Fix deactivated camera still being used in render world #15946
Conversation
Switch to retained render world causes the extracted cameras in render world to not be removed until camera in main world is despawned. When extracting data from main world inactive cameras are skipped. Therefore camera that was active and became inactive has a retained `ExtractedCamera` component from previous frames (when it was active) and is processed the same way as if it were active (there is no `active` field on `ExtractedCamera`). This breakes switching between cameras in `render_primitives` example. Fix it by removing `ExtractedCamera` and related components from inactive cameras. Note that despawning inactive camera seems to be bad option because they are spawned using `SyncToRenderWorld` component. Fixes bevyengine#15822
rafalh
force-pushed
the
deactivate-camera-fix
branch
from
cfd614c to
86be404
Compare
rafalh
force-pushed
the
deactivate-camera-fix
branch
from
240a657 to
91cfef5
Compare
alice-i-cecile
added
C-Bug
alice-i-cecile
added
the
S-Needs-Review
|
@akimakinai can you review this for me? |
| ExtractedCamera, | ||
| ExtractedView, | ||
| RenderVisibleEntities, | ||
| TemporalJitter, | ||
| RenderLayers, | ||
| Projection, | ||
| GpuCulling, |
There was a problem hiding this comment.
In my opinion this is going to be extremely difficult to maintain. The moment that a new component that should be listed here for removal is added, it almost for sure won't.
Is this the only solution? Is there some equivalent of "required components" that automatically removes other components, if one is removed?
Another question (just to understand): what happens when a camera that is made inactive is re-activated? Is there already a solution to re-add these components automatically on reactivation?
There was a problem hiding this comment.
In my opinion this is going to be extremely difficult to maintain. The moment that a new component that should be listed here for removal is added, it almost for sure won't.
Is this the only solution? Is there some equivalent of "required components" that automatically removes other components, if one is removed?
Yes, this is not ideal. However, since insertion and removal are handled within this single function, it's not unmanageable for now, I guess. We need better solution, but it might be too late for 0.15 unfortunately.
Another question (just to understand): what happens when a camera that is made inactive is re-activated? Is there already a solution to re-add these components automatically on reactivation?
Unless we hit this branch and continue, commands.insert below inserts these components every frame (redundantly).
"Re-insert every frame" was seemingly intended to be a temporary but working solution until we get to redesign extraction to benefit more from retained render world (per Next Steps in original PR), but unfortunately issues like this were found lately that already require removing components. So hopefully this is all temporary.
There was a problem hiding this comment.
Do we actually need to remove all of those components?
Wouldn't removing ExtractedView be enough to halt all processing for this camera?
There was a problem hiding this comment.
@MiniaczQ Removing ExtractedView alone wouldn't fix all the problems. In this PR I was focusing on render_primitives example and I started from from trying to fix warnings about camera ambiguity from sort_cameras system. This system uses only ExtractedCamera component so it would still spam the logs.
Could less components be removed? Probably. That's what I initially implemented although I don't know if the set I proposed was minimal (it was pretty much arbitrary on my side).
For me it makes sense to remove all components that the system inserts and it's easier to reason about than look through all the systems and check which components are important and should be removed and which could be left.
But I agree it's not perfect and could be hard to synchronize. I hope that in the future a better more ergonomic and hopefully generic solution will be found. But for 0.15 I think it is good enough.
There was a problem hiding this comment.
Oof. Agree with all the comments here, but hope this is at least good motivation to take a look at how this should work at the beginning of 0.16.
alice-i-cecile
added
S-Ready-For-Final-Review
| ExtractedCamera, | ||
| ExtractedView, | ||
| RenderVisibleEntities, | ||
| TemporalJitter, | ||
| RenderLayers, | ||
| Projection, | ||
| GpuCulling, |
There was a problem hiding this comment.
Oof. Agree with all the comments here, but hope this is at least good motivation to take a look at how this should work at the beginning of 0.16.
# Objective - Fixes #15871 (Camera is done in #15946) ## Solution - Do the same as #15904 for other extraction systems - Added missing `SyncComponentPlugin` for DOF, TAA, and SSAO (According to the [documentation](https://dev-docs.bevyengine.org/bevy/render/sync_component/struct.SyncComponentPlugin.html), 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. - [x] DOF - toggling DOF component and perspective in `depth_of_field` example - [x] TAA - toggling `Camera.is_active` and TAA component - [x] clusters - not entirely sure, toggling `Camera.is_active` in `many_lights` example (no crash/glitch even without this PR) - [x] previous_view - toggling `Camera.is_active` in `skybox` (no crash/glitch even without this PR) - [x] lights - toggling `Visibility` of `DirectionalLight` in `lighting` example - [x] SSAO - toggling `Camera.is_active` and SSAO component in `ssao` example - [x] default UI camera view - toggling `Camera.is_active` (nop without #15946 because UI defaults to some camera even if `DefaultCameraView` is not there) - [x] volumetric fog - toggling existence of volumetric light. Looks like optimization, no change in behavior/visuals
Objective
Switch to retained render world causes the extracted cameras in render world to not be removed until camera in main world is despawned. When extracting data from main world inactive cameras are skipped. Therefore camera that was active and became inactive has a retained
ExtractedCameracomponent from previous frames (when it was active) and is processed the same way as if it were active (there is noactivefield onExtractedCamera). This breakes switching between cameras inrender_primitivesexample.Fixes #15822
Solution
Fix it by removing
ExtractedCameraand related components from inactive cameras.Note that despawning inactive camera seems to be bad option because they are spawned using
SyncToRenderWorldcomponent.Testing
Switching camera in
render_primitivesexample now works correctly.