Skip to content

Fix deactivated camera still being used in render world #15946

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Oct 19, 2024
Merged

Fix deactivated camera still being used in render world #15946

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
86be404
Fix deactivated camera still being used in render world
rafalh Oct 16, 2024
e0414a2
Simplify removing camera components in render world
rafalh Oct 16, 2024
91cfef5
Remove more components for deactivated camera in render world
rafalh Oct 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions crates/bevy_render/src/camera/camera.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1054,8 +1054,18 @@ pub fn extract_cameras(
) in query.iter()
{
if !camera.is_active {
commands.entity(render_entity).remove::<(
ExtractedCamera,
ExtractedView,
RenderVisibleEntities,
TemporalJitter,
RenderLayers,
Projection,
GpuCulling,
Comment on lines +1058 to +1064
Copy link
Contributor

@wilk10 wilk10 Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we actually need to remove all of those components?
Wouldn't removing ExtractedView be enough to halt all processing for this camera?

Copy link
Contributor Author

@rafalh rafalh Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

)>();
continue;
}

let color_grading = color_grading.unwrap_or(&ColorGrading::default()).clone();

if let (
Expand Down