Mesh overhaul with custom vertex attributes #592

[julhe]

Pull request draft for #592

[fu5ha]

based on my above comments, this would end up as something like

for pipeline in render_pipelines.pipelines {
    let pipeline_descriptor = pipeline_descriptors.get(pipeline.descriptor).unwrap();
    if let Some(layout) = pipelin_descriptor.layout {
        for (slot, vertex_buffer_descriptor) in layout.vertex_buffer_descriptors.iter().enumerate() {
            let handle = render_resource_context
                .get_asset_resource(*handle, vertex_buffer_descriptor.name)
                .expect("mesh does not provide required vertex attribute {} for pipeline {:?}", vertex_buffer_descriptor.name, pipeline_descriptor);
            println!("setup vertex buffer {}", slot);
            render_pipelines.bindings.set_vertex_buffer(slot as u8, handle);
        }
    }
}

[julhe]

Gonna fix the rough edges now and propose this PR. ⌨️

[julhe]

#592 (comment)

[fu5ha]

Mostly looking good to me. Has this been tested with sprite/2d pipelines?

[julhe]

Has this been tested with sprite/2d pipelines?

Works all for me🙂

[julhe]

Quick summary

• You can straight forward throw any attribute data at Mesh now and it will be mapped to the named binding slot. No dealing with vertices structs anymore. That also means any code that bridges between separate <-> interleaved vertex data is gone, therefore the reduced code size.
• That also means that VertexBufferAttributes don't need to be defined upfront. All necessary info (stride, shader_location) is implicit.
• Shader attribute naming is now mostly irrelevant. Attributes that start with "I_" will be used for instancing.

Uncool things

• No support for interleaved buffers at all. This is cool for desktop GPUs (and any CPU) but might be a bottleneck on mobile GPUs. Change default vertex layout to seperate buffers #592 (comment)
• No way of using half for vertex attributes. Would be nice to have, since many attributes (normal, vertex-colors, uv's, ...) can be stored as half just fine. (This wasn't possible before either, but now it would be harder to add).
• When a vertex buffer is required by a shader but not defined by the mesh, this is only noticed on the very last step. This could be hard to debug in bigger projects.

[julhe]

Okay, from my POV there’s nothing to add to this anymore. 🙂

Some post PR things: While this approach will work for now, over long term, we will hit some issues:

1. As Change default vertex layout to seperate buffers #592 (comment) mention, there will be issues regarding max. bound buffers. from wgpu limitations when animations come into place.
2. Unbound attribute handling.
3. Mobile GPUs still preferer interleaved data.
4. Support for half or other formats

My idea would be to decouple all these things from mesh.rs and instead add something like a VertexDataToGPUAdapter which could:

• Bundle attributes into interleaved buffers
• Convert attributes to half or other formats
• Undefined attribute handling
• Allow for custom attribute encoding, like converting normals into octahedrons for reduced bandwidth.

This would probably sit somewhere at render_pipeline.rs and would be user configureable. The API would be something like Res<VertexDataToGPUConfig> and could have functions like:

• fn set_attribute_post_processing(attribute_name: &str, on_attribute_post: Fn) (handles type conversion + attribute post proccessing)
• fn set_attribute_prefered_interleaved_buffer(attribute_name: &str, buffer_id: usize)
• fn set_attribute_missing_behaviour(attribute_name: &str, on_attribute_missing: Fn)

[julhe]

Summary (Dev version)

• mesh.rs packs now all attributes into one buffer via attributes_to_vertex_buffer_data().
• mesh.rs also provides dummy buffer (VERTEX_FALLBACK_BUFFER_ID) for undefined attributes, so the pipeline can be rendered in any case.
• render_pipeline can now specializes on individual buffer descriptions.
• reflect_layout returns each attribute as a single buffer. (This shall be subject of change later)
• Removed any old code like VertexAttribute, Vertex, AsVertexBufferDescriptor
• RenderResourceContext uses u64 now.
• mesh.attributes is a HashMap now.

Future stuff

• Add API to customize attribute data befor passing to buffer to allow:
  • Float -> Half/NormU16/... conversion
  • Attribute Packing (is that even still a thing?)
  • normal compression
  • (custom initialization of undefinied attributes ?)
• Allow for multiple attribute buffers.
• Replace buffer names with hashes for tighter structs.
• Allow for VertexAttributeValues to be unloaded after beeing send to GPU to save CPU memory.
• Allocated one big fallback buffer instead of speperate ones for each mesh.
• Provide example.

[cart]

In general this looks good to me. Most of my feedback is superficial. Allocating a "fallback buffer" for each Mesh feels like overkill, but its not a big deal and we could always allocate "shared fallback buffers" of varying sizes if/when it becomes a problem. We might need to revisit it when we re-add support for multiple vertex buffers (ex: for instancing).

[cart]

Another followup I'd like is a "custom mesh with custom attributes" example that proves that we support arbitrary layouts with custom shaders.

[julhe]

In general this looks good to me. Most of my feedback is superficial. Allocating a "fallback buffer" for each Mesh feels like overkill, but its not a big deal and we could always allocate "shared fallback buffers" of varying sizes if/when it becomes a problem.

Agree. I'm thinking of a generic buffer that expands size by the largest allocated mesh. Will add this to the followup.

[cart]

Looks like sorting attributes breaks the shader_custom_material and shader_defs examples. Maybe something was reliant on that order?

[cart]

Gave the new fixes a try with various permutations of attributes in custom shaders and they all worked. I think this is good to go!

[J-F-Liu]

After update to latest bevy code, there is a big performance degradation caused by this PR.

[cart]

@J-F-Liu that's probably related to #764. I have a PR out that should fix it

[J-F-Liu]

My frame rate dropped from 28 to 7, I check out mesh-allocation-fix and have a try, the frame rate rises to 25.
Btw, Mesh:: attribute(&mut self, should remove mut.

[julhe]

@J-F-Liu sounds interesting, can you tell some context? (or even provide some source code?)

[J-F-Liu]

@julhe There are 5431 mesh objects rendered as PrimitiveTopology::LineList, plus mouse movement screen ray interaction checking.

[cart]

I'm guessing the extra allocations for the VertexBufferDescriptor in each entity's PipelineSpecialization are to blame for the slight drop in performance (and maybe also the act of re-computing it).

If so that would be resolved by the followups called out here: #768 (comment)

[cart]

I think the approach we're using now is the right path forward. We just need to remove some of the hot-spots introduced by some slightly naive choices made for simplicity (and to meet the 0.3 deadline that I have arbitrarily set 😄 ).