State your state

- Address a few bugs with rubidium
 - Ignore EBOs when restoring state
 - ElementBuffer deals in raw gl handles
 - Document all state changes/restores/resets

Co-authored-by: PepperCode1 <44146161+peppercode1@users.noreply.github.com>

src/main/java/com/jozufozu/flywheel/backend/gl/GlStateTracker.java

@@ -39,7 +39,7 @@ public class GlStateTracker {
 		return new State(BUFFERS.clone(), vao, program);
 	}
 
-	public static record State(int[] buffers, int vao, int program) {
+	public record State(int[] buffers, int vao, int program) {
 		public void restore() {
 			if (vao != GlStateTracker.vao) {
 				GlStateManager._glBindVertexArray(vao);
@@ -48,7 +48,7 @@ public class GlStateTracker {
 			GlBufferType[] values = GlBufferType.values();
 
 			for (int i = 0; i < values.length; i++) {
-				if (buffers[i] != GlStateTracker.BUFFERS[i]) {
+				if (buffers[i] != GlStateTracker.BUFFERS[i] && values[i] != GlBufferType.ELEMENT_ARRAY_BUFFER) {
 					GlStateManager._glBindBuffer(values[i].glEnum, buffers[i]);
 				}
 			}

src/main/java/com/jozufozu/flywheel/backend/instancing/instancing/GPUInstancer.java

@@ -45,6 +45,7 @@ public class GPUInstancer<D extends InstanceData> extends AbstractInstancer<D> {
 	public void render() {
 		if (invalid()) return;
 
+		// XXX VAO is bound and not reset or restored
 		vao.bind();
 
 		renderSetup();
@@ -68,12 +69,15 @@ public class GPUInstancer<D extends InstanceData> extends AbstractInstancer<D> {
 
 		vao = new GlVertexArray();
 
+		// XXX Callback seems unnecessary. Remove and extract code to run after alloc call?
 		model = modelAllocator.alloc(modelData, arenaModel -> {
+			// XXX VAO is bound and not reset or restored
 			vao.bind();
 
 			arenaModel.setupState(vao);
 		});
 
+		// XXX VAO is already guaranteed to be bound in model callback
 		vao.bind();
 		vao.enableArrays(model.getAttributeCount() + instanceFormat.getAttributeCount());
 
@@ -108,6 +112,7 @@ public class GPUInstancer<D extends InstanceData> extends AbstractInstancer<D> {
 			removeDeletedInstances();
 		}
 
+		// XXX ARRAY_BUFFER is bound and reset
 		instanceVBO.bind();
 		if (!realloc()) {
 

src/main/java/com/jozufozu/flywheel/backend/instancing/instancing/InstancedMaterialGroup.java

@@ -68,15 +68,23 @@ public class InstancedMaterialGroup<P extends WorldProgram> implements MaterialG
 		return vertexCount;
 	}
 
+	// XXX Overriden in CrumblingGroup
+	// XXX Runs inside of restore state
 	public void render(Matrix4f viewProjection, double camX, double camY, double camZ, RenderLayer layer) {
 		type.setupRenderState();
-		Textures.bindActiveTextures();
+		Textures.bindActiveTextures(); // XXX Changes active unit and bound textures
-		renderAll(viewProjection, camX, camY, camZ, layer);
+		renderAll(viewProjection, camX, camY, camZ, layer); // XXX May change ARRAY_BUFFER binding (reset or not reset), VAO binding (not reset), shader binding (not reset), call Model.createEBO
 		type.clearRenderState();
+		// XXX Should texture bindings be reset or restored?
+		// XXX Should the active unit be reset or restored?
+		// XXX Should the VAO binding be reset or restored?
+		// XXX Should the ARRAY_BUFFER binding be reset or restored?
+		// XXX Should the shader binding be reset or restored?
 	}
 
+	// XXX Internal GL state changes are inconsistent; sometimes bindings are reset to 0, sometimes not
 	protected void renderAll(Matrix4f viewProjection, double camX, double camY, double camZ, RenderLayer layer) {
-		initializeInstancers();
+		initializeInstancers(); // XXX May change ARRAY_BUFFER binding (reset or not reset), VAO binding (not reset), call Model.createEBO
 
 		vertexCount = 0;
 		instanceCount = 0;
@@ -88,6 +96,7 @@ public class InstancedMaterialGroup<P extends WorldProgram> implements MaterialG
 			P program = owner.context.getProgram(ProgramContext.create(entry.getKey()
 					.getProgramSpec(), Formats.POS_TEX_NORMAL, layer));
 
+			// XXX Shader is bound and not reset or restored
 			program.bind();
 			program.uploadViewProjection(viewProjection);
 			program.uploadCameraPos(camX, camY, camZ);
@@ -95,7 +104,7 @@ public class InstancedMaterialGroup<P extends WorldProgram> implements MaterialG
 			setup(program);
 
 			for (GPUInstancer<?> instancer : material.getAllInstancers()) {
-				instancer.render();
+				instancer.render(); // XXX May change VAO binding (not reset), ARRAY_BUFFER binding (reset)
 				vertexCount += instancer.getVertexCount();
 				instanceCount += instancer.getInstanceCount();
 			}
@@ -103,19 +112,19 @@ public class InstancedMaterialGroup<P extends WorldProgram> implements MaterialG
 	}
 
 	private void initializeInstancers() {
-		ModelAllocator allocator = getModelAllocator();
+		ModelAllocator allocator = getModelAllocator(); // XXX May change ARRAY_BUFFER binding (not reset)
 
 		// initialize all uninitialized instancers...
 		for (InstancedMaterial<?> material : materials.values()) {
 			for (GPUInstancer<?> instancer : material.uninitialized) {
-				instancer.init(allocator);
+				instancer.init(allocator); // XXX May change VAO binding (not reset), ARRAY_BUFFER binding (not reset), call Model.createEBO
 			}
 			material.uninitialized.clear();
 		}
 
 		if (allocator instanceof ModelPool pool) {
 			// ...and then flush the model arena in case anything was marked for upload
-			pool.flush();
+			pool.flush(); // XXX May change ARRAY_BUFFER binding (reset)
 		}
 	}
 

src/main/java/com/jozufozu/flywheel/backend/instancing/instancing/InstancingEngine.java

@@ -74,6 +74,7 @@ public class InstancingEngine<P extends WorldProgram> implements Engine {
 	 */
 	@Override
 	public void render(TaskEngine taskEngine, RenderLayerEvent event) {
+		// XXX Restore state
 		GlStateTracker.State restoreState = GlStateTracker.getRestoreState();
 
 		double camX;

src/main/java/com/jozufozu/flywheel/backend/model/ElementBuffer.java

@@ -1,25 +1,29 @@
 package com.jozufozu.flywheel.backend.model;
 
-import com.jozufozu.flywheel.backend.gl.GlNumericType;
+import com.jozufozu.flywheel.backend.gl.buffer.GlBufferType;
-import com.jozufozu.flywheel.backend.gl.buffer.GlBuffer;
+import com.mojang.blaze3d.vertex.VertexFormat;
 
 public class ElementBuffer {
 
-	private final GlBuffer buffer;
+	protected final int elementCount;
-	public final int elementCount;
+	protected final VertexFormat.IndexType eboIndexType;
-	public final GlNumericType eboIndexType;
+	private final int glBuffer;
 
-	public ElementBuffer(GlBuffer backing, int elementCount, GlNumericType indexType) {
+	public ElementBuffer(int backing, int elementCount, VertexFormat.IndexType indexType) {
-		this.buffer = backing;
-		this.eboIndexType = indexType;
 		this.elementCount = elementCount;
+		this.eboIndexType = indexType;
+		this.glBuffer = backing;
 	}
 
 	public void bind() {
-		buffer.bind();
+		GlBufferType.ELEMENT_ARRAY_BUFFER.bind(glBuffer);
 	}
 
-	public void unbind() {
+	public int getElementCount() {
-		buffer.unbind();
+		return elementCount;
+	}
+
+	public VertexFormat.IndexType getEboIndexType() {
+		return eboIndexType;
 	}
 }

src/main/java/com/jozufozu/flywheel/backend/model/IndexedModel.java

@@ -52,27 +52,26 @@ public class IndexedModel implements BufferedModel {
 	 * The VBO/VAO should be bound externally.
 	 */
 	public void setupState(GlVertexArray vao) {
+		// XXX ARRAY_BUFFER is bound and not reset or restored
 		vbo.bind();
 		vao.enableArrays(getAttributeCount());
 		vao.bindAttributes(0, getType().getLayout());
+		ebo.bind();
 	}
 
 	@Override
 	public void drawCall() {
-		ebo.bind();
+		GL20.glDrawElements(primitiveMode.glEnum, ebo.getElementCount(), ebo.getEboIndexType().asGLType, 0);
-		GL20.glDrawElements(primitiveMode.glEnum, ebo.elementCount, ebo.eboIndexType.getGlEnum(), 0);
 	}
 
 	/**
-		 * Draws many instances of this model, assuming the appropriate state is already bound.
+	 * Draws many instances of this model, assuming the appropriate state is already bound.
-		 */
+	 */
 	@Override
 	public void drawInstances(int instanceCount) {
 		if (!valid()) return;
 
-		ebo.bind();
+		GL31.glDrawElementsInstanced(primitiveMode.glEnum, ebo.getElementCount(), ebo.getEboIndexType().asGLType, 0, instanceCount);
-
-		GL31.glDrawElementsInstanced(primitiveMode.glEnum, ebo.elementCount, ebo.eboIndexType.getGlEnum(), 0, instanceCount);
 	}
 
 	public boolean isDeleted() {

src/main/java/com/jozufozu/flywheel/backend/model/ModelPool.java

@@ -42,6 +42,7 @@ public class ModelPool implements ModelAllocator {
 
 		vbo = new MappedGlBuffer(GlBufferType.ARRAY_BUFFER);
 
+		// XXX ARRAY_BUFFER is bound and not reset or restored
 		vbo.bind();
 		vbo.setGrowthMargin(stride * 64);
 	}
@@ -68,6 +69,7 @@ public class ModelPool implements ModelAllocator {
 		if (dirty) {
 			if (anyToRemove) processDeletions();
 
+			// XXX ARRAY_BUFFER is bound and reset
 			vbo.bind();
 			if (realloc()) {
 				uploadAll();
@@ -182,25 +184,25 @@ public class ModelPool implements ModelAllocator {
 
 		@Override
 		public void setupState(GlVertexArray vao) {
+			// XXX ARRAY_BUFFER is bound and not reset or restored
 			vbo.bind();
 			vao.enableArrays(getAttributeCount());
 			vao.bindAttributes(0, vertexType.getLayout());
+			ebo.bind();
 		}
 
 		@Override
 		public void drawCall() {
-			GL32.glDrawElementsBaseVertex(GlPrimitive.TRIANGLES.glEnum, ebo.elementCount, ebo.eboIndexType.getGlEnum(), 0, first);
+			GL32.glDrawElementsBaseVertex(GlPrimitive.TRIANGLES.glEnum, ebo.getElementCount(), ebo.getEboIndexType().asGLType, 0, first);
 		}
 
 		@Override
 		public void drawInstances(int instanceCount) {
 			if (!valid()) return;
 
-			ebo.bind();
-
 			//Backend.log.info(StringUtil.args("drawElementsInstancedBaseVertex", GlPrimitive.TRIANGLES, ebo.elementCount, ebo.eboIndexType, 0, instanceCount, first));
 
-			GL32.glDrawElementsInstancedBaseVertex(GlPrimitive.TRIANGLES.glEnum, ebo.elementCount, ebo.eboIndexType.getGlEnum(), 0, instanceCount, first);
+			GL32.glDrawElementsInstancedBaseVertex(GlPrimitive.TRIANGLES.glEnum, ebo.getElementCount(), ebo.getEboIndexType().asGLType, 0, instanceCount, first);
 		}
 
 		@Override

src/main/java/com/jozufozu/flywheel/backend/model/VBOModel.java

@@ -58,6 +58,7 @@ public class VBOModel implements BufferedModel {
 	 * The VBO/VAO should be bound externally.
 	 */
 	public void setupState(GlVertexArray vao) {
+		// XXX ARRAY_BUFFER is bound and not reset or restored
 		vbo.bind();
 		vao.enableArrays(getAttributeCount());
 		vao.bindAttributes(0, getLayout());

src/main/java/com/jozufozu/flywheel/core/QuadConverter.java

@@ -1,43 +1,32 @@
 package com.jozufozu.flywheel.core;
 
-import java.nio.Buffer;
+import org.jetbrains.annotations.NotNull;
-import java.nio.ByteBuffer;
+import org.jetbrains.annotations.Nullable;
-import java.nio.ByteOrder;
+import org.lwjgl.opengl.GL32;
-import java.util.EnumMap;
+import org.lwjgl.opengl.GL32C;
-import java.util.Map;
-
-import javax.annotation.Nonnull;
-import javax.annotation.Nullable;
-
-import org.lwjgl.system.MemoryStack;
 import org.lwjgl.system.MemoryUtil;
 
 import com.jozufozu.flywheel.backend.gl.GlNumericType;
-import com.jozufozu.flywheel.backend.gl.buffer.GlBuffer;
 import com.jozufozu.flywheel.backend.gl.buffer.GlBufferType;
-import com.jozufozu.flywheel.backend.gl.buffer.MappedGlBuffer;
+import com.jozufozu.flywheel.backend.gl.buffer.GlBufferUsage;
 import com.jozufozu.flywheel.backend.model.ElementBuffer;
 import com.jozufozu.flywheel.event.ReloadRenderersEvent;
+import com.mojang.blaze3d.vertex.VertexFormat;
 
-import net.minecraftforge.api.distmarker.Dist;
+import it.unimi.dsi.fastutil.ints.Int2ReferenceArrayMap;
-import net.minecraftforge.eventbus.api.EventPriority;
+import it.unimi.dsi.fastutil.ints.Int2ReferenceMap;
-import net.minecraftforge.eventbus.api.SubscribeEvent;
-import net.minecraftforge.fml.common.Mod;
 
 /**
  * A class to manage EBOs that index quads as triangles.
  */
-@Mod.EventBusSubscriber(Dist.CLIENT)
 public class QuadConverter {
 
-	public static final int STARTING_CAPACITY = 42; // 255 / 6 = 42
-
 	private static QuadConverter INSTANCE;
 
-	@Nonnull
+	@NotNull
 	public static QuadConverter getInstance() {
 		if (INSTANCE == null) {
-			INSTANCE = new QuadConverter(STARTING_CAPACITY);
+			INSTANCE = new QuadConverter();
 		}
 
 		return INSTANCE;
@@ -48,130 +37,78 @@ public class QuadConverter {
 		return INSTANCE;
 	}
 
-	Map<GlNumericType, GlBuffer> ebos;
+	private final Int2ReferenceMap<ElementBuffer> cache = new Int2ReferenceArrayMap<>();
-	int[] capacities;
+	private final int ebo;
+	private int quadCapacity;
 
-	public QuadConverter(int initialCapacity) {
+	public QuadConverter() {
-		this.ebos = new EnumMap<>(GlNumericType.class);
+		this.ebo = GL32.glGenBuffers();
-		initCapacities();
+		this.quadCapacity = 0;
-
-		fillBuffer(initialCapacity);
 	}
 
 	public ElementBuffer quads2Tris(int quads) {
-		int indexCount = quads * 6;
+		if (quads > quadCapacity) {
-		GlNumericType type = getSmallestIndexType(indexCount);
+			grow(quads * 2);
-
-		if (quads > getCapacity(type)) {
-			fillBuffer(quads, indexCount, type);
 		}
 
-		return new ElementBuffer(getBuffer(type), indexCount, type);
+		return cache.computeIfAbsent(quads, this::createElementBuffer);
 	}
 
-	private void initCapacities() {
+	@NotNull
-		this.capacities = new int[GlNumericType.values().length];
+	private ElementBuffer createElementBuffer(int quads) {
+		return new ElementBuffer(ebo, quads * 6, VertexFormat.IndexType.INT);
 	}
 
-	private int getCapacity(GlNumericType type) {
+	private void grow(int quads) {
-		return capacities[type.ordinal()];
+		int byteSize = quads * 6 * GlNumericType.UINT.getByteWidth();
-	}
+		final long ptr = MemoryUtil.nmemAlloc(byteSize);
 
-	private void updateCapacity(GlNumericType type, int capacity) {
+		fillBuffer(ptr, quads);
-		if (getCapacity(type) < capacity) {
+
-			capacities[type.ordinal()] = capacity;
+		// XXX ARRAY_BUFFER is bound and reset
-		}
+		final var bufferType = GlBufferType.ARRAY_BUFFER;
+		final int oldBuffer = bufferType.getBoundBuffer();
+		bufferType.bind(ebo);
+		GL32C.nglBufferData(bufferType.glEnum, byteSize, ptr, GlBufferUsage.STATIC_DRAW.glEnum);
+		bufferType.bind(oldBuffer);
+
+		MemoryUtil.nmemFree(ptr);
+
+		this.quadCapacity = quads;
 	}
 
 	public void delete() {
-		ebos.values()
+		GL32.glDeleteBuffers(ebo);
-				.forEach(GlBuffer::delete);
+		this.quadCapacity = 0;
-		ebos.clear();
-		initCapacities();
 	}
 
-	private void fillBuffer(int quads) {
+	private void fillBuffer(long ptr, int quads) {
-		int indexCount = quads * 6;
+		int numVertices = 4 * quads;
+		int baseVertex = 0;
+		while (baseVertex < numVertices) {
+			writeQuadIndicesUnsafe(ptr, baseVertex);
 
-		fillBuffer(quads, indexCount, getSmallestIndexType(indexCount));
+			baseVertex += 4;
-	}
+			ptr += 6 * 4;
-
-	private void fillBuffer(int quads, int indexCount, GlNumericType type) {
-		MemoryStack stack = MemoryStack.stackPush();
-		int bytes = indexCount * type.getByteWidth();
-
-		ByteBuffer indices;
-		if (bytes > stack.getSize()) {
-			indices = MemoryUtil.memAlloc(bytes); // not enough space on the preallocated stack
-		} else {
-			stack.push();
-			indices = stack.malloc(bytes);
 		}
-
-		indices.order(ByteOrder.nativeOrder());
-
-		fillBuffer(indices, type, quads);
-
-		GlBuffer buffer = getBuffer(type);
-
-		buffer.bind();
-		buffer.upload(indices);
-		buffer.unbind();
-
-		if (bytes > stack.getSize()) {
-			MemoryUtil.memFree(indices);
-		} else {
-			stack.pop();
-		}
-
-		updateCapacity(type, quads);
 	}
 
-	private void fillBuffer(ByteBuffer indices, GlNumericType type, int quads) {
+	private void writeQuadIndicesUnsafe(long ptr, int baseVertex) {
-		for (int i = 0, max = 4 * quads; i < max; i += 4) {
+		// triangle a
-			// triangle a
+		MemoryUtil.memPutInt(ptr, baseVertex);
-			type.castAndBuffer(indices, i);
+		MemoryUtil.memPutInt(ptr + 4, baseVertex + 1);
-			type.castAndBuffer(indices, i + 1);
+		MemoryUtil.memPutInt(ptr + 8, baseVertex + 2);
-			type.castAndBuffer(indices, i + 2);
+		// triangle b
-			// triangle b
+		MemoryUtil.memPutInt(ptr + 12, baseVertex);
-			type.castAndBuffer(indices, i);
+		MemoryUtil.memPutInt(ptr + 16, baseVertex + 2);
-			type.castAndBuffer(indices, i + 2);
+		MemoryUtil.memPutInt(ptr + 20, baseVertex + 3);
-			type.castAndBuffer(indices, i + 3);
-		}
-		((Buffer) indices).flip();
 	}
 
-	private GlBuffer getBuffer(GlNumericType type) {
+	// make sure this gets reset first, so it has a chance to repopulate
-		return ebos.computeIfAbsent(type, $ -> new MappedGlBuffer(GlBufferType.ELEMENT_ARRAY_BUFFER));
-	}
-
-	/**
-	 * Given the needed number of indices, what is the smallest bit width type that can index everything? <br>
-	 *
-	 * <pre>
-	 * | indexCount   | type  |
-	 * |--------------|-------|
-	 * | [0, 255)     | byte  |
-	 * | [256, 65536)	| short	|
-	 * | [65537, )	| int	|
-	 * </pre>
-	 */
-	private static GlNumericType getSmallestIndexType(int indexCount) {
-//		indexCount = indexCount >>> 8;
-//		if (indexCount == 0) {
-//			return GlNumericType.UBYTE;
-//		}
-//		indexCount = indexCount >>> 8;
-//		if (indexCount == 0) {
-//			return GlNumericType.USHORT;
-//		}
-
-		return GlNumericType.UINT;
-	}
-
-	// make sure this gets reset first so it has a chance to repopulate
-	@SubscribeEvent(priority = EventPriority.HIGHEST)
 	public static void onRendererReload(ReloadRenderersEvent event) {
-		if (INSTANCE != null) INSTANCE.delete();
+		if (INSTANCE != null) {
+			INSTANCE.delete();
+			INSTANCE = null;
+		}
 	}
 }

src/main/java/com/jozufozu/flywheel/core/crumbling/CrumblingGroup.java

@@ -18,6 +18,7 @@ public class CrumblingGroup<P extends CrumblingProgram> extends InstancedMateria
 		super(owner, type);
 	}
 
+	// XXX See notes of overriden method
 	@Override
 	public void render(Matrix4f viewProjection, double camX, double camY, double camZ, RenderLayer layer) {
 		type.setupRenderState();

src/main/java/com/jozufozu/flywheel/core/crumbling/CrumblingRenderer.java

@@ -65,6 +65,7 @@ public class CrumblingRenderer {
 
 		Vec3 cameraPos = camera.getPosition();
 
+		// XXX Restore state
 		GlStateTracker.State restoreState = GlStateTracker.getRestoreState();
 		CrumblingRenderer.renderBreaking(activeStages, new RenderLayerEvent(level, null, stack, null, cameraPos.x, cameraPos.y, cameraPos.z));
 		restoreState.restore();
@@ -87,6 +88,7 @@ public class CrumblingRenderer {
 
 				instanceManager.beginFrame(SerialTaskEngine.INSTANCE, info);
 
+				// XXX Each call applies another restore state even though we are already inside of a restore state
 				materials.render(SerialTaskEngine.INSTANCE, event);
 
 				instanceManager.invalidate();
@@ -94,6 +96,9 @@ public class CrumblingRenderer {
 
 		}
 
+		// XXX Inconsistent GL state cleanup
+		// If texture binding and active unit need to be restored, store them in variables before GL state is changed
+		// instead of guessing that unit 0 and crumbling tex 0 are correct
 		GlTextureUnit.T0.makeActive();
 		AbstractTexture breaking = textureManager.getTexture(ModelBakery.BREAKING_LOCATIONS.get(0));
 		if (breaking != null) RenderSystem.bindTexture(breaking.getId());

src/main/java/com/jozufozu/flywheel/core/model/Model.java

@@ -46,6 +46,7 @@ public interface Model {
 		return Formats.POS_TEX_NORMAL;
 	}
 
+	// XXX Since this is public API (technically) we cannot make assumptions about what GL state this method can use or modify unless a contract is established.
 	/**
 	 * Create an element buffer object that indexes the vertices of this model.
 	 *