fix: rendering corruption related to opaque mesh binning when restoring a minized window

[CodingDaniel1]

Objective

Fixes #25649

Solution

added_entities inside RenderVisibleEntitiesClass is about to newly added entities, and binning relies on that invariant, but the cpu culling system which uses RenderVisibleEntities will get wiped during camera inactive and size eq to 0, 0 time. But RenderVisibleEntities relies on persistent data to determine added_entities and removed_entities.

So the fix to this is to not remove RenderVisibleEntities during camera inactive time.

Testing

• Did you test these changes? If so, how?
  I tested the issue binaries from Restoring a minimized window corrupts rendering after duplicate binning #25649 and it all worked fine when restoring minizing windows no matter what.

• Are there any parts that need more testing?
  Definitely, im new to bevy cpu culling side of things, i dont really know what this change will affect other code, but i dont see any issues right now tho.

• How can other people (reviewers) test your changes? Is there anything specific they need to know?
  Just run any examples related to cpu culling and test to see if anything goes wrong.

• If relevant, what platforms did you test these changes on, and are there any important ones you can't test?
  I have only tested this on windows10

[github-actions]

Welcome, new contributor!

Please make sure you've read our contributing guide, as well as our policy regarding AI usage, and we look forward to reviewing your pull request shortly ✨

[CodingDaniel1]

I also removed the ambiguous RenderVisibleEntitiesClass::add_entity method since added_entities field is public and the cpu and gpu culling path uses different method to push new entity to it.

I also use panic! instead of error! for the original error, since it completely corrupts rendering and thus should never happen generally.

[arovge]

Removing this method + the doc changes may be better suited to a followup PR. I think there is a benefit to the doc comment here pointing you to sort_added_entities() rather than usages needing to discover the doc comment on sort_added_entities() to know sorting must happen after adding an entity to the list

Maybe a followup PR could explore sorting entities inside of add_entity() and adding a add_entity_without_sorting() method to bring more value to a add_entity() method?

Either way, definitely something to explore/change outside of this PR

[CodingDaniel1]

Yes i think a followup PR is a good fit for this actually.

[JMS55]

Can you try an example where you toggle camera.is_active on and off, and make sure that rendering does not break?

[CodingDaniel1]

Seems like cpu culling is messed up when only changing is_active field on Camera.

Im going to convert this to draft first. then come up with the fix

[CodingDaniel1]

Wait has anyone ever tested is_active field on the camera? Even without this pr, theres still issue when toggling is_active.

Ive found that if you just toggle is_active off and on, previously visible meshes will be hidden until you look away and look at them again.

[JMS55]

I'm not surprised this it's broken, unfortunately we don't have automated tests for it...

[CodingDaniel1]

Okay seems like if i reverted the change this pr made, the is_active field is working properly for cpu culled meshes, but if its tagged with NoCpuCulling then it has the same issue i described above

[CodingDaniel1]

Do i try to fix it in this PR or in a follow up PR, since the Gpu culling path reassembles the issue i described above, and this pr made cpu culling path did that as well.

[CodingDaniel1]

Looking at different snapshot captured in renderdoc, seems like after camera is_active field being toggled on and off, majority of the passes just skip doing work at all. Including compute passes that cull meshes and build indirect parameters. and raster passes like prepass and main pass

[CodingDaniel1]

I understand the issue now.

Basically multiple systems rely on is_active field on camera to determine whether the camera is being rendered or not. extract_core_3d_camera_phases is one of them, it removes the corresponding render phase from ViewBinnedRenderPhases<Opaque3d> when the camera is inactive or just simply gone from the world. But the extract_camera system actually treats a camera inactive when either the field is false, or the viewport size is (0, 0).

A possible fix came to my mind initially is to replace is_active with a getter function which takes viewport size into account. Which means changing every places use the is_active field to some function like is_active_and_valid_viewport. This also means a complete rebin operation will happen for a window minized restore operation, which is not wanted i guess.

Another fix to this issue is much more complex and involves refactoring existing systems. The idea is to keep what we have now, keep removing the RenderVisibleEntities when Camera::is_active is false, but not removing it when window is minized. Then we need to remove all states RenderGpuCulledEntities already had. Just like removing RenderVisibleEntities on the render camera thats responsible for cpu culling.

Or just be simple, do not remove either RenderVisibleEntities and RenderGpuCulledEntities, just dont remove the binned phase even the camera is inactive. Which means if we want no overhead for an inactive camera, the best thing to do is to despawn it entirely, otherwise it will still have related retained rendering data

I dont know if this is kinda going out of scope of this PR.

[CodingDaniel1]

I need to mention if this got merged, the cpu culling path will trigger the issues gpu culling path already does, which means needs a followup pr