Advanced vertex format support

[trevex]

As described in https://github.com/attackgoat/screen-13/issues/54, this draft PR tries to illustrate the basic idea.

Fixes https://github.com/attackgoat/screen-13/issues/54

[trevex]

I added an implementation of the derive macro. Currently the user has to provide the number of locations a type requires (if it requires more than 1), e.g. https://github.com/trevex/screen-13/blob/vertex-layout/src/driver/vertex.rs#L141

This could be automated and checked for correctness if more information about the formats is available. For example if the following were to exist: https://android.googlesource.com/platform/external/vulkan-validation-layers/+/HEAD/layers/vk_format_utils.cpp#40 I will check if there is a possibility to upstream this to ash as it would be the best place for this kind of information.

Currently still missing from this PR:

• More documentation and a proper example (or maybe updating examples)
• VertexLayout implementation for DerivedVertexLayout (will require some thought to validate the layout as much as possible, but above info about formats would be nice to check if a subsequent location to a format exceeding 16 bytes (e.g. 4x f64 components) is unassigned)

[attackgoat]

I'm watching this with interest however I do have concern about the macro-based design. For users who decide they need advanced vertex layouts, they should provide bindings/attributes directly as the data is pretty simple. If they manually provide erroneous data I would prefer they rely on validation layers for debugging.

One tiny issue is that two Vec's of similar data is confusing where one Vec of bindings with a child Vec of attributes might be "clearer". I think Khronos split the lists like this to be memory efficient as opposed to object-oriented. If automatic layout was not available I would suggest the Vec-contains-Vecs approach, but as this is a secondary option for advanced users two-Vecs seem fine. It matches the Vulkan SDK documentation and numerous C++ examples that way.

I'll throw together my thoughts into a different branch and see if that makes sense.

[trevex]

Hmm, interesting, if I understand you correctly, I agree that Vec-contains-Vec would make VertexInputState nicer to use and allow to properly compose multiple bindings, so I would agree it is a worthwhile change in general and certainly brings in part of the benefits this PR tries to implement, but the VertexLayout trait of this PR could still be of value on top of that:

The idea of the VertexLayout-trait is to do the "matching" of the vertex-layout (available bindings and attributes) during pipeline creation with shader inputs. So the same Vertex can be used by both the shadow-mapping pass and a PBR pass, but the first will only use the position attribute, while the PBR pass would use additional attributes, without needing to specify correct locations or use two different manual VertexInputState. It would also make it easy to create the "same" pipeline for different vertex layouts.

I will leave the validation out for now, as you said the validation layers already provide this information. So the scope of this PR will be to ease the creation of VertexInputState by deriving Vertex and matching the fields with the shader input.

Let me know what you think.

[trevex]

Okay I finished the basic implementation, but it is not yet used by GraphicPipeline, the following test illustrates probably best what I am trying to accomplish: https://github.com/trevex/screen-13/blob/vertex-layout/src/driver/vertex.rs#L179

Creating the GraphicPipeline with an explicity layout that is "matched" against shader input, might then look as follows:

let pipeline = Arc::new(GraphicPipeline::create(
        &event_loop.device,
        GraphicPipelineInfo::with_explicit_vertex_layout([
            MyVertex::layout(vk::VertexInputRate::VERTEX), // explicit `VertexInputState` can be used as `VertexLayout` as well
            MyInstanceData::layout(vk::VertexInputRate::INSTANCE),
        ]),
        [
            Shader::new_vertex(
                // ...
            ),
            Shader::new_fragment(
                // ...
            ),
        ],
    )?);

I will hold off on further changes to this PR for now to hear your thoughts.

[attackgoat]

Thanks - I'll look at what you did! Here is my idea as to how to implement this; I just give users the ability to specify those Vulkan structs.

Unfortunately my example has got some sort of problem; I see the data come out correctly in RenderDoc but it fails to transform properly and instead of -1, 0, -1, 1 (etc) vertices I get 0, -1.875, 0, 1 and so the upper-right triangle (the one with 64 bit positions) does not show up. The lower-right one (32 bit positions) does:

I will try this example on your branch and see if I can reproduce the issue or a fix.

[attackgoat]

Okay as for my branch - I fixed the issue in the example. It was just that we need to use dvec2 (and such types) when handling 64-bit data, whoops.

I'm going to merge #57 for the base functionality desired by this PR/issue; but I don't want to stop the creativity or throw this effort away. I think it provides value on top of the "manual" way for some cases; such as the one you mention where one model might have a shadow pass and PBR pass which use the same buffer but different layouts. The vsm_omni.rs example does run into this issue, however from previous projects I had gotten used to keeping a separate shadow buffer of just position data.

I think this vsm_omni.rs example really emphasizes there are many ways to get these jobs done; and a macro-based vertex struct probably works for some folks. Screen-13 currently works directly off byte arrays because it's agnostic of the users' opinion on how to solve problems.

This is where contrib/macros can really help out for those interested in the type of use-case it solves.

• Derivable macro applied to vertex types
• Can be applied to ShaderBuilder types to set vertex layout
• Comes with opinions on binding/location/stride issues which make certain use-cases "automatic"

[trevex]

Ok, let me rework the code then into a contrib/... -crate. I am considering including some form of validation then as an incompatibility such as lacking dvec or a used location after an attribute with a format exceeding 16 bytes could easily be checked (among other things).

The current names of this PR (mainly VertexLayout-trait) somewhat clash with ShaderBuiilder::vertex_layout, which basically accepts VertexInputState. I would suggest ShaderBuilder::vertex_layout is renamed to ShaderBuilder::vertex_input_state to more closely mirror the Vulkan API. WDYT?

This would also save me the time to think of new alternative names.

[attackgoat]

Good change; the concept is "vertex layout" but the Vulkan term is "vertex input". I dropped "state" from the user-accessible function because the rest of the code generally doesn't surface helper words and also a conflict with the #[Builder] macro.

[trevex]

PR was automatically closed when I force pushed current master to start porting code from scratch to contrib/.

[trevex]

@attackgoat Okay, integrating the code from contrib with Screen 13 was slightly more tricky than expected and it would be great to hear your thoughts as I am a new rustacean.

The current implementation extends ShaderBuilder and adds with_vertex_layout method which builds the shader with the specified layout. I had to make some relevant types and fields public, which I believe is acceptable specifically as the Shader fields are useful to other use-cases around automating things using the reflected shader.

I added a triangle example to illustrate the usage: https://github.com/trevex/screen-13/blob/vertex-layout/contrib/screen-13-macros/examples/triangle.rs

I also tried defining VertexLayout in Screen 13 directly with a vertex_layout method added to ShaderBuilder, but due to limitations how traits are allowed to be implemented that would essentially push most of the code, except the derive macro in-tree. At that point you might as well add the derive macro as well and hide it behind a feature flag.

As mentioned in the beginning it would be great to get some feedback. Maybe there is another way to integrate all-together.

[trevex]

Thanks for the feedback.

I now fixed all the to dos I still left as comments in the code, so the macro should handle the following cases as well:

• Padding fields are properly handled to calculate attribute offsets (align(N) also works, but bytemuck::Pod requires explicit padding)
• Formats exceeding 16 bytes and a single location are now properly incremented by 2 (not sure if anyone will every need this but potentially useful for instancing data with double precision)

I believe the code is in a good state now, so I will work on the following next:

• Proper "advanced" example using instancing
• Additional documentation
• I might extend the tests further while I am at it