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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Fix dependency of shadow mapping on the optional PrepassPlugin #7878

Closed
wants to merge 9 commits into from
Closed

[Merged by Bors] - Fix dependency of shadow mapping on the optional PrepassPlugin #7878

wants to merge 9 commits into from

Conversation

geieredgar
Copy link
Contributor

@geieredgar geieredgar commented Mar 2, 2023 •
edited
Loading

Objective

Unfortunately, there are three issues with my changes introduced by #7784.

  1. The changes left some dead code. This is already taken care of here: [Merged by Bors] - Remove dead code after #7784 #7875.
  2. Disabling prepass causes failures because the shadow mapping relies on the PrepassPlugin now.
  3. Custom materials use the prepass.wgsl shader, but this does not always define a fragment entry point.

This PR fixes 2. and 3. and resolves #7879.

Solution

  • Add a regression test with disabled prepass.
  • Split PrepassPlugin into two plugins:
    • PrepassPipelinePlugin contains the part that is required for the shadow mapping to work and is unconditionally added.
    • PrepassPlugin now only adds the systems and resources required for the "real" prepasses.
  • Add a noop fragment entry point to prepass.wgsl, used if NORMAL_PASS is not defined.

@JMS55 JMS55 added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen labels Mar 2, 2023
@JMS55 JMS55 added this to the 0.10 milestone Mar 2, 2023
@geieredgar geieredgar mentioned this pull request Mar 2, 2023
Closed
@geieredgar
Copy link
Contributor Author

While investigating #7879, I have found a third issue: When using custom materials, the default prepass.wgsl shader could be used. But prepass.wgsl only defined an entry point for the fragment shader if NORMAL_PASS was defined. I added an entrypoint for the other cases that simply does nothing. This will produce the same shadows like before #7784. So a custom material should override the prepass_fragment_shader method to be able to obtain better shadows like the StandardMaterial.

superdump
superdump requested changes Mar 3, 2023
View reviewed changes
crates/bevy_pbr/src/prepass/mod.rs Outdated Show resolved Hide resolved
@geieredgar @superdump
Update crates/bevy_pbr/src/prepass/mod.rs
672486d 
Co-authored-by: Robert Swain <robert.swain@gmail.com>
@james7132 james7132 requested a review from superdump March 3, 2023 11:38
@robtfm robtfm self-requested a review March 3, 2023 13:18
robtfm
robtfm reviewed Mar 3, 2023
View reviewed changes
Copy link
Contributor

@robtfm robtfm left a comment

Choose a reason for hiding this comment

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

This does work, but I don't think adding the dummy fragment shader should be necessary. instead we can just not add the FragmentState in PrepassPipeline::specialize() when the material_fragment_shader is None.

StandardMaterial returns a handle, so that will behave correctly. Custom Materials that return None would not add a frag shader, which would be more efficient.

0.10 is looming though, so if there isn't time then this is fine as is

@geieredgar
Copy link
Contributor Author

Ok, I adjusted the fragment state condition and its comment, reverted the fragment entry point commit and tested that the shader_prepass example still works. I think this PR is ready to be merged.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 3, 2023
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Mar 3, 2023
@geieredgar
Fix dependency of shadow mapping on the optional PrepassPlugin (#7878)
cb0db07 
# Objective

Unfortunately, there are three issues with my changes introduced by #7784.

1.  The changes left some dead code. This is already taken care of here: #7875.
2. Disabling prepass causes failures because the shadow mapping relies on the `PrepassPlugin` now.
3. Custom materials use the `prepass.wgsl` shader, but this does not always define a fragment entry point.

This PR fixes 2. and 3. and resolves #7879.

## Solution

- Add a regression test with disabled prepass.
- Split `PrepassPlugin` into two plugins:
  - `PrepassPipelinePlugin` contains the part that is required for the shadow mapping to work and is unconditionally added.
  - `PrepassPlugin` now only adds the systems and resources required for the "real" prepasses.
- Add a noop fragment entry point to `prepass.wgsl`, used if `NORMAL_PASS` is not defined.


Co-authored-by: Edgar Geier <geieredgar@gmail.com>
@bors
Copy link
Contributor

bors bot commented Mar 3, 2023

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Fix dependency of shadow mapping on the optional PrepassPlugin [Merged by Bors] - Fix dependency of shadow mapping on the optional PrepassPlugin Mar 3, 2023
@bors bors bot closed this Mar 3, 2023
@geieredgar geieredgar mentioned this pull request Mar 3, 2023
Merged
Shfty pushed a commit to shfty-rust/bevy that referenced this pull request Mar 19, 2023
@geieredgar @Shfty
Fix dependency of shadow mapping on the optional PrepassPlugin (bev…
9d18245 
…yengine#7878)

# Objective

Unfortunately, there are three issues with my changes introduced by bevyengine#7784.

1.  The changes left some dead code. This is already taken care of here: bevyengine#7875.
2. Disabling prepass causes failures because the shadow mapping relies on the `PrepassPlugin` now.
3. Custom materials use the `prepass.wgsl` shader, but this does not always define a fragment entry point.

This PR fixes 2. and 3. and resolves bevyengine#7879.

## Solution

- Add a regression test with disabled prepass.
- Split `PrepassPlugin` into two plugins:
  - `PrepassPipelinePlugin` contains the part that is required for the shadow mapping to work and is unconditionally added.
  - `PrepassPlugin` now only adds the systems and resources required for the "real" prepasses.
- Add a noop fragment entry point to `prepass.wgsl`, used if `NORMAL_PASS` is not defined.


Co-authored-by: Edgar Geier <geieredgar@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@IceSentry IceSentry IceSentry approved these changes

@robtfm robtfm robtfm approved these changes

@superdump superdump Awaiting requested review from superdump

Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
0.10
Development

Successfully merging this pull request may close these issues.

shader_prepass example panics
6 participants
@geieredgar @alice-i-cecile @superdump @IceSentry @robtfm @JMS55