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

Make setup of Opaque3dPrepass and AlphaMask3dPrepass phase items consistent with others #8408

Merged
merged 3 commits into from
Jun 12, 2023

Conversation

Sixmorphugus
Copy link
Contributor

@Sixmorphugus Sixmorphugus commented Apr 16, 2023

Objective

When browsing the bevy source code to try and learn about bevy_core_pipeline, I noticed that the DrawFunctions resources, sort_phase_systems and texture preparation for the Opaque3d and AlphaMask3d phase items are all set up in bevy_core_pipeline, while the Opaque3dPrepass and AlphaMask3dPrepass phase items are only declared in bevy_core_pipeline, and actually registered properly with the renderer in bevy_pbr.

This means that, if I am trying to make crate that replaces bevy_pbr, I need to make sure I manually fix this unfinished setup the same way that bevy_pbr does. Worse, it means that if I try to use the PrepassNode bevy_core_pipeline adds without fixing this, the engine will simply crash because the DrawFunctions<T> resources cannot be accessed.

The only advantage I can think of for bevy doing it this way is an ambiguous performance save due to the prepass render phases not being present unless you are using prepass materials with PBR.

Solution

I have moved the registration of DrawFunctions<T>, sort_phase_system::<T>, camera RenderPhase extraction, and texture preparation for prepass's phase items into bevy_core_pipeline alongside the equivalent code that sets up the Opaque3d, AlphaMask3d and Transparent3d phase items.

Am open to tweaking this to improve the performance impact of prepass things being around if the app doesn't use them if needed.

I've tested that the shader_prepass example still works with this change.

@github-actions
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@Sixmorphugus
Copy link
Contributor Author

Fixing unused imports rn

@IceSentry IceSentry added A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior labels Apr 16, 2023
@IceSentry IceSentry self-requested a review April 16, 2023 23:19
Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

LGTM

@nicopap nicopap added the C-Code-Quality A section of code that is hard to understand or change label Apr 20, 2023
Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

Actually, apply_normal_mapping is used in pbr_prepass.wgsl at line 98, you should update that file to use the new argument list.

The reason it compiles is that the shader_prepass.rs example doesn't have a normal map, so the #ifdef removes the code from the file, and there is no opportunity to create a shader compilation error since the code is never compiled.

…viously done in bevy_pbr) consistent with Opaque3d and AlphaMask3d phase items (done in bevy_core_pipeline) to make it easier to use bevy_core_pipeline's render phases without bevy_pbr
@Sixmorphugus
Copy link
Contributor Author

Sixmorphugus commented Apr 21, 2023

Actually, apply_normal_mapping is used in pbr_prepass.wgsl at line 98, you should update that file to use the new argument list.

The reason it compiles is that the shader_prepass.rs example doesn't have a normal map, so the #ifdef removes the code from the file, and there is no opportunity to create a shader compilation error since the code is never compiled.

I'm not sure what you mean, I've pulled changes just now and the argument list hasn't changed in pbr_functions.wgsl.
I also never touched any .wgsl files in these changes and am unsure why any would be affected.
What new argument list should it be using?

@nicopap
Copy link
Contributor

nicopap commented Apr 21, 2023

I mistook this PR for #8106 😅 How embarrassing. Everything is good

Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

LGTM, sorry for not reviewing this sooner.

@nicopap nicopap 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 Jun 11, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 12, 2023
Merged via the queue into bevyengine:main with commit a78c4d7 Jun 12, 2023
1 check passed
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-Bug An unexpected or incorrect behavior C-Code-Quality A section of code that is hard to understand or change 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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants