Skip bloom rendering if HDR is disabled#15856
Closed
akimakinai wants to merge 1 commit intobevyengine:mainfrom
Closed
Skip bloom rendering if HDR is disabled#15856akimakinai wants to merge 1 commit intobevyengine:mainfrom
akimakinai wants to merge 1 commit intobevyengine:mainfrom
Conversation
alice-i-cecile
added
C-Bug
alice-i-cecile
requested review from
IceSentry,
JMS55,
Trashtalk217 and
tychedelia
alice-i-cecile
added
the
S-Needs-Review
IceSentry
approved these changes
Oct 11, 2024
tychedelia
approved these changes
Oct 11, 2024
Member
tychedelia
left a comment
tychedelia
left a comment
There was a problem hiding this comment.
This looks like a good check to have anyway, but I'm concerned about the component sticking around. Minimally we should be more careful in the future about using SyncToRenderWorld to guarantee cleanup.
13 tasks
github-merge-queue bot
pushed a commit
that referenced
this pull request
Oct 15, 2024
# Objective Ensure that components that are conditionally extracted do not linger in the render world when not extracted from the main world. ## Solution If the `ExtractComponent` returns `None`, we'll remove the render world component. I think this is the most sensible behavior here. In the future if there really is a use case for keeping the previous render component around, we could add a `Option<Self::Out>` parameter for the previous render component to the method, or something similar. I think that this follows the principle of least surprise here relative to what `None` would suggest and the way that render nodes are typically written. The alternative would be to add an `enabled` field to pretty much every camera settings component, or duplicate the extraction condition as #15856 does. ## Testing `transmission` no longer crashes. ## Migration Guide Components that implement `ExtractComponent` and return `None` will cause the extracted component to be removed from the render world.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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
camera.hdrwhile havingBloomcomponent, which can be reproduced withtransmissionexample where you can toggle it withHkey. Several examples appear broken following recent changes #15570 (comment)Bloomcomponent is conditionally extracted here, so before retained rendering world the component existed in rendering world only when HDR is enabled.Solution
!camera.hdr.It seems that conditional extraction is no longer a valid pattern, because
extract_componentreturningNonewill keep the previous value if it existed.Testing
transmissionexample no longer crashes when toggling HDR