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

bevy_pbr: Reuse the mesh.wgsl vertex stage for shadow mapping and remove depth.wgsl #4303

Closed

Conversation

superdump
Copy link
Contributor

Objective

  • Reduce fragmentation of vertex stage code which can easily lead to bugs and mistakes as depth.wgsl is 'missed'

Solution

  • Refactor the View struct out of mesh_view_bind_group.wgsl into view_struct.wgsl and define the bevy_pbr::view_struct import. Import this into mesh_view_bind_group.wgsl as it is used there for bindings.
  • Remove depth.wgsl and rework mesh.wgsl to reproduce the same vertex stage by excluding unneeded code with the help of a SHADOW_MAPPING shader def

Changelog

  • Changed: Shadow mapping passes now use the same vertex shader file as main passes to ease maintenance

Migration Guide

  • Use MESH_SHADER_HANDLE with the SHADOW_MAPPING shader def for the vertex stage of shadow mapping passes instead of SHADOW_SHADER_HANDLE

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 23, 2022
entry_point: "vertex".into(),
shader_defs: vec![],
shader_defs: vec!["SHADOW_MAPPING".to_string()],
Copy link
Member

Choose a reason for hiding this comment

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

For this to be correct in the general case, we need to inherit the shaderdefs of the specialized mesh pipeline in addition to this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The point was that with SHADOW_MAPPING defined, only code that is either without any other shader defs or that has shader defs that are configured for shadow passes are included.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a ‘by convention’ maintenance burden, but I think it’s pretty manageable because so little is needed for shadow mapping. Just clip position.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess your comment was about, for example, VERTEX_TANGENTS. My point is that if SHADOW_MAPPING is set, no other shader defs are part of what remains.

Copy link
Member

Choose a reason for hiding this comment

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

This is likely going to be needed when we merge in skinning and morph target support. Where they are needed in the shadow vertex shader to be correct. Any other user made specialized define would also likely need to be reflected here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I realised what you meant when I looked over the mesh skinning PR last night. I'll have to figure out how to hook that up then.

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change and removed S-Needs-Triage This issue needs to be labelled labels Mar 23, 2022
@superdump
Copy link
Contributor Author

I'm going to close this because I want to move in the opposite direction to a more specialised depth pass. The problem with reusing the mesh vertex stage is that it needs to load all vertex attributes. For non-animated geometry for depth-only passes, only the position attribute is needed. I have learned since making this PR that it is a good idea to have separate vertex buffers for the position attribute and for everything else so that depth passes do not even read the unused vertex attribute data, have better cache usage, and improve performance.

@superdump superdump closed this Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants