Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(rendering): factor out MutableChunkMesh #4998

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

keturn
Copy link
Member

@keturn keturn commented Feb 18, 2022

One of the things that made working on ChunkMeshWorker for #4987 scary was the concern that a ChunkMesh's data might be updated while it was in the middle of updating the mesh for OpenGL.

Upon closer examination, it looks like in practice that a new instance of ChunkMesh is created each time the chunk is dirtied. That means Chunk.mesh could be a reference to a safer, non-mutable interface.

I experimented with this (see attached 269f823), moving the methods vertexElements, updateMesh and disposeData to a new MutableMesh interface.

Outstanding before merging

There are a few things that need to be addressed:

  • LodChunkProvider does not follow the same approach of generating new instances, and instead calls updateMesh and discardData on its RenderableChunk. Should that change? Or is the LodChunkProvider code redundant with the ChunkMeshWorker code?
  • ChunkMesh.updateMaterial sounds like it might mutate the chunk (it starts with update), but it really only updates the material. In fact, I see only one implementation of this, and it looks like it could be a static method.
  • ChunkMeshTypeHandler operates directly on vertexElements that an immutable ChunkMesh does not provide access to.
    • that type handler looks like it is for EntityBasedRenderableChunk Allow entities to be rendered like chunks #4614 for FallingBlocks kinds of things. I think we can provide for this, introduce some private methods for serialization, if we decide that's the way to go.

@github-actions github-actions bot added the Type: Refactoring Request for or implementation of pure and automatic refactorings, e.g. renaming, to improve clarity label Feb 18, 2022
@keturn
Copy link
Member Author

keturn commented Feb 18, 2022

An alternate structure might be, instead of having a MutableChunkMesh and a ChunkMesh interface and one is descended from the other, to have some kind of builder object. The builder object wouldn't have a render method at all, and then once the vertex elements are all set, you call a method that returns a new immutable renderable ChunkMesh.

I think that's a cleaner separation than subclassing.

@pollend
Copy link
Member

pollend commented Feb 19, 2022

the generated mesh has to be scheduled back on the main thread to push it to opengl. all opengl calls are sequential and the operation is unbound after the mesh is submitted. The worst that can happen is you schedule it at a bad time and cause a pipeline stall. I would worry about this more if this was like vulkan where that kind of behavior can be a problem. opengl is "fairly safe in this regards".

@@ -44,7 +45,7 @@ protected PersistedData serializeNonNull(ChunkMesh value, PersistedDataSerialize
directBuffer.rewind();
asBuffers.add(directBuffer);
}
ChunkMesh result = new ChunkMeshImpl();
MutableChunkMesh result = new ChunkMeshImpl();
for (ChunkMesh.RenderType renderType : ChunkMesh.RenderType.values()) {
result.getVertexElements(renderType).buffer.replace(asBuffers.remove(0));
Copy link
Member

@pollend pollend Feb 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this probably should be changed. these are raw buffers for opengl so they follow the endiness of the machine if two machines have different endiness then it will be garbled. network order is little endian I believe

Base automatically changed from test/chunkmeshworker to refactor/migrate-chunk-mesh-generation-flowable May 7, 2022 17:11
Base automatically changed from refactor/migrate-chunk-mesh-generation-flowable to develop May 13, 2022 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Refactoring Request for or implementation of pure and automatic refactorings, e.g. renaming, to improve clarity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants