Support custom vertex formats

[bonsairobo]

I don't think this code is very good. I just wanted to hack this functionality in as easily as I could.

I encountered a little confusion and some obstacles to make this work:

1. I don't understand the purpose of the VertexBufferDescriptors resource
2. I don't understand the purpose of PipelineLayout::sync_vertex_buffer_descriptors
3. The pipeline layout only gets constructed by the PipelineCompiler in the DRAW stage, but we want to use the layout during the RENDER_RESOURCE stage. So when an entity is created with a Mesh and a RenderPipelines, the layout isn't ready yet to create the vertex buffer. I just added the LoadingMeshes resource to fix this, but I don't think that's the right final solution.
4. There is an alignment bug with mapping GPU index buffers. I just hacked around this in the example code by duplicating indices.
5. It's not awesome that I iterate over that query to get the VertexBufferDescriptors every frame even in frames where vertex buffers don't need to be created. Not sure what the best way to handle this is.
6. I think it's kinda weird that the actual PipelineDescriptor we need is stored inside of the PipelineCompiler. Maybe there is a better place for it.

Also, what's the bar for writing tests? I'm happy to add some. Not sure if there are any particular test patterns I should follow.

[bonsairobo]

Oops. I accidentally transitioned out of draft mode. I put it back for now. I will eventually learn how to use Github.

[bonsairobo]

I want to capture some of the ideas that came up in Discord for how to improve the structure of this change.

1. I think the mesh_resource_provider_system wants to be a render graph node system. This would allow us to move the pipeline compilation into another node so that the mesh system can take a dependency on it.
2. We should have some kind of mapping from descriptor to set of pipelines. Right now my patch just does a linear search through all of the pipelines to find the ones that are compatible with a descriptor.

[multun]

Can you rebase this on master? Format checking was enabled, and the CI doesn't yet check for it at the commit you're at

[bonsairobo]

I implemented the CompilePipelinesNode and MeshNode like I talked about in my last comment. I'm not totally familiar with how SystemNode is supposed to work, but all the examples seem to work still, and I managed to delete the hacky set of loading meshes. The only odd thing was that the Node::update implementations are just empty. Kinda smells to me.

I also went ahead and deleted the VertexBufferDesciptors resource, since it wasn't really being used for anything anymore.

Instead of implementing the map from descriptor to pipelines, I figured that could maybe happen in a separate PR. It's only an issue for scaling to many pipelines, which is probably not a huge concern yet.

[cart]

Sorry for the delay. Just finished a first pass over the code. First, lets answer some questions:

I don't understand the purpose of the VertexBufferDescriptors resource

Its there to provide consistent VertexBufferDescriptors across pipelines, even when shaders don't list identical vertex attributes. Your changes result in multiple copies of mesh buffers for each pipeline layout used.

For example, a pipeline with the layout [Vertex_Position, Vertex_Normal] will have different buffers than a pipeline that has the layout [Vertex_Position].

Given how expensive syncing buffers is (and the potential for shaders to have many different permutations of attributes), it made sense to me to allow configuring shared VertexBufferDescriptors that give shaders a common set of attributes.

I don't understand the purpose of PipelineLayout::sync_vertex_buffer_descriptors

See the above rationale. It allows pipelines to be configured to share mesh buffers. I think this is a valuable property worth preserving.

The pipeline layout only gets constructed by the PipelineCompiler in the DRAW stage, but we want to use the layout during the RENDER_RESOURCE stage. So when an entity is created with a Mesh and a RenderPipelines, the layout isn't ready yet to create the vertex buffer. I just added the LoadingMeshes resource to fix this, but I don't think that's the right final solution.

I think you ended up not using LoadingMeshes and opted to add pipeline compilation to the render graph. I think i'd like pipeline compilation to remain a system, as it isn't involved in the creation of command encoders.

There is an alignment bug with mapping GPU index buffers. I just hacked around this in the example code by duplicating indices.

Dang. We should work around that, but I think i would prefer it if we worked around it at a higher level (ex: in our mesh allocation code). I'd prefer it if the backend buffer logic was "what you ask for is what you get".

It's not awesome that I iterate over that query to get the VertexBufferDescriptors every frame even in frames where vertex buffers don't need to be created. Not sure what the best way to handle this is.

Yeah I'd also prefer it if we didn't do this. Lets solve this after we decide the best way to handle "mesh buffer sharing".

I think it's kinda weird that the actual PipelineDescriptor we need is stored inside of the PipelineCompiler. Maybe there is a better place for it.

We need to store the mapping between "original descriptor + specialization" to "specialized descriptor" somewhere. Given that the PipelineCompiler is what operates on that data, it made sense to me to store it there. I'm open to suggestions if you have better ideas.

I think the mesh_resource_provider_system wants to be a render graph node system. This would allow us to move the pipeline compilation into another node so that the mesh system can take a dependency on it.

I know I'm the one that suggested that mesh_resource_provider_system gets moved to a graph node, but I'm (regrettably) having second thoughts about that. In general the purpose of the render graph is to generate command buffers in parallel. mesh_resource_provider_system doesn't need to generate command buffers and fits quite nicely into normal system constraints. I think I'm actually going to push for things to move out of the graph that dont relate to command buffer encoder generation. Same goes for the pipeline compiler. It doesn't benefit from living in the graph. Adding it only makes things more complicated and adds more boilerplate.

We should have some kind of mapping from descriptor to set of pipelines. Right now my patch just does a linear search through all of the pipelines to find the ones that are compatible with a descriptor.

Yeah the current solution is quite expensive. We should work out ways to optimize it.

Summary / High Level Thoughts

• I think the "seamless" approach you implemented here is really nice UX. Users don't need to think about vertex layouts at all and it "just works".
• I'm not convinced that just relying on reflected pipeline layouts is the right call. The cost of duplicated mesh buffers seems too high. I'm ok with adding a manual step somewhere if it allows us to share buffers across shaders. Given that most people only care about vertex position/uv/normal (and sometimes color), I think we can make those defaults work just as seamlessly as your current implementation.
• I'm pretty sure I don't want to move mesh buffer + pipeline compilation to the graph. It doesnt seem like we gain anything and theres an increased complexity cost. If operation ordering is an issue, we should be able to resolve that in the ECS schedule.

@aclysma and @lachlansneff: you are both pretty deep into the bevy renderer at this point, so I'd love to hear your thoughts here too.

[cart]

Oops didnt answer all of the questions. I'll edit the message above to fill them in.

[bonsairobo]

Hey @cart thanks for the detailed feedback. I think I agree with all of your points, and I wasn't aware that there was an intent to share vertex buffers between pipelines with different descriptors, but that makes sense.

In that case, I think the existing shader reflection logic that generates VertexBufferDescriptors will not work for us. It assumes that a vertex buffer layout will match the shader's attribute binding order, which will not always be true if we want to use the same buffer with many shaders. In fact, I think we don't even want to generate a VertexBufferDescriptor during shader reflection. At most, we should just generate something like a ShaderVertexLayout which only specifies the attributes names and sizes. Then at a later stage, we could link the ShaderVertexLayout to a VertexBufferLayout.

And concerning the graph nodes, I was mostly just trying it out because it seemed to fit the dependency model, but after seeing how the other graph nodes are used, I agree that it's just causing more boilerplate for this particular use case.

[aclysma]

Some general thoughts not necessarily related to this PR (most of which you have probably already though through for yourself!):

• I think for longer term/PBR, we're going to want to split vertex data into a few buffers. (I think we are guaranteed support for 4 buffers bound at once.) For example a depth pre-pass may only need position, and shading PBR will need to add normals, tangents, UV. A shader can consume both buffers at once to have all 4 values. We will likely want to "bless" a few standard layouts for specific features like PBR and avoid creating a buffer for every possible permutation of inputs being read.
  • We can probably get away with a single vertex buffer that includes every possible value to start with
• Pipeline setup can allow using vertex streams that include more information than shader actually uses, which avoids needing multiple copies of the same data (at the cost of the "extra" data making what we actually care about sparser.).
  • I think that's what you're already doing with sync_vertex_buffer_descriptors
• There will always be one-offs for certain systems like imgui or debug draw, so definitely great to have custom layouts well-supported
• Almost everything should be hashed, shared, and reference counted. Sharing resources allows for batching. The expense of reference counting (which usually requires allocation) doesn't matter because of how comparatively expensive it is to allocate GPU resources.
  • I noticed some strings that should probably be hashed. Would be nice to have an engine-primitive "HashedString" that retains the string name in debug only.
  • One of the downsides of reflection is that it kind of hides these sharing opportunities - you don’t necessarily “see” it when you make a pipeline that is not sharing something, but could be if it were designed for it. (Not arguing against it, just think it's worth mentioning.)
• I generally agree that anything that doesn’t need to be in the render graph should be done somewhere else

[cart]

Closing in favor of the direction we're taking in #599 (and future follow up prs)