-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Remove components if not extracted #15948
Conversation
Hm, in some cases, we also need to remove components inserted from |
Instead, I went ahead to add the config component ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rafalh are you able to review this PR for me in turn? :) This is closely related to #15946.
@alice-i-cecile I tried but I don't think I understand enough about the render graph to fully review this. I left some comments through. Most of changes looks okay to me.
crates/bevy_pbr/src/render/light.rs
Outdated
.remove::<( | ||
ExtractedDirectionalLight, | ||
RenderCascadesVisibleEntities, | ||
LightViewEntities, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LightViewEntities
is added in an observer and not in this system, so it's not clear at the first glance why we remove it here. Maybe we should remove it in an observer too to be consistent? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added OnRemove
version of the observer that removes LightViewEntities
.
@@ -342,6 +346,7 @@ impl ViewNode for DepthOfFieldNode { | |||
_: &mut RenderGraphContext, | |||
render_context: &mut RenderContext<'w>, | |||
( | |||
_depth_of_field, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure why do we need this and it seems like a hack to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, changed to remove components in view query too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for leaving the breadcrumbs to improve this later.
Objective
(Camera is done in Fix deactivated camera still being used in render world #15946)
Solution
SyncComponentPlugin
for DOF, TAA, and SSAO(According to the documentation, this plugin "needs to be added for manual extraction implementations." We may need to check this is done.)
Testing
Modified example locally to add toggles if not exist.
depth_of_field
exampleCamera.is_active
and TAA componentCamera.is_active
inmany_lights
example (no crash/glitch even without this PR)Camera.is_active
inskybox
(no crash/glitch even without this PR)Visibility
ofDirectionalLight
inlighting
exampleCamera.is_active
and SSAO component inssao
exampleCamera.is_active
(nop without Fix deactivated camera still being used in render world #15946 because UI defaults to some camera even ifDefaultCameraView
is not there)