[Merged by Bors] - use better set inheritance in render systems #7524
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jakobhellermann
commented
Feb 6, 2023
Comment on lines
+83
to
+88
| .configure_sets( | ||
| ( | ||
| PrepareAssetLabel::PreAssetPrepare, | ||
| PrepareAssetLabel::AssetPrepare, | ||
| PrepareAssetLabel::PostAssetPrepare, | ||
| ) |
There was a problem hiding this comment.
This only needs to be configured once. But because this is in RenderAssetPlugin<A>, it will be configured for every render asset A. I don't think there is a better place to put it, because the RenderAssetPlugin should work without e.g. the PbrPlugin.
Is configure_sets idempotent? Or should there be a if !initialized { configure(); initialized = true } is some way?
There was a problem hiding this comment.
configure_sets should be idempotent. If it's not, that's a bug.
alice-i-cecile
added
A-Rendering
alice-i-cecile
approved these changes
Feb 6, 2023
hymm
approved these changes
Feb 6, 2023
alice-i-cecile
added
the
S-Ready-For-Final-Review
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Feb 6, 2023
# Objective Some render systems that have system set used as a label so that they can be referenced from somewhere else. The 1:1 translation from `add_system_to_stage(Prepare, prepare_lights.label(PrepareLights))` is `add_system(prepare_lights.in_set(Prepare).in_set(PrepareLights)`, but configuring the `PrepareLights` set to be in `Prepare` would match the intention better (there are no systems in `PrepareLights` outside of `Prepare`) and it is easier for visualization tools to deal with. # Solution - replace ```rust prepare_lights in PrepareLights prepare_lights in Prepare ``` with ```rs prepare_lights in PrepareLights PrepareLights in Prepare ``` **Before**  **After** 
|
Pull request successfully merged into main. Build succeeded:
|
bors
bot
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
Some render systems that have system set used as a label so that they can be referenced from somewhere else.
The 1:1 translation from
add_system_to_stage(Prepare, prepare_lights.label(PrepareLights))isadd_system(prepare_lights.in_set(Prepare).in_set(PrepareLights), but configuring thePrepareLightsset to be inPreparewould match the intention better (there are no systems inPrepareLightsoutside ofPrepare) and it is easier for visualization tools to deal with.Solution
with
Before
After
