fix: rendering corruption related to opaque mesh binning when restoring a minized window - #25670
Conversation
|
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 ✨ |
|
I also removed the ambiguous I also use panic! instead of error! for the original error, since it completely corrupts rendering and thus should never happen generally. |
| /// After calling this method one or more times, you must call | ||
| /// [`Self::sort_added_entities`] to ensure the [`Self::added_entities`] | ||
| /// list is sorted. | ||
| pub fn add_entity(&mut self, pair: (Entity, MainEntity)) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Yes i think a followup PR is a good fit for this actually.
it) Move RenderVisibleEntities Note comment to extract_cameras.
JMS55
added
C-Bug
JMS55
left a comment
JMS55
left a comment
There was a problem hiding this comment.
Can you try an example where you toggle camera.is_active on and off, and make sure that rendering does not break?
|
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 |
|
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. |
|
I'm not surprised this it's broken, unfortunately we don't have automated tests for it... |
|
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 |
|
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. |
|
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 |
|
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. 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 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 Or just be simple, do not remove either I dont know if this is kinda going out of scope of this PR. |
JMS55
added
S-Needs-Design
|
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 |
…ng a minized window (bevyengine#25670) # Objective Fixes bevyengine#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 bevyengine#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
…ng a minized window (bevyengine#25670) # Objective Fixes bevyengine#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 bevyengine#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
…led meshes - extract_meshes_for_gpu_building: add Changed<RenderLayers> and RemovedComponents<RenderLayers> to the change detection so that a pure render-layer change on a (GPU-culled) mesh actually flows through extraction and reaches RenderGpuCulledEntities::update(). Without this, the changed_layers list is only ever populated when the mesh is re-extracted for some other reason. - collect_gpu_culled_meshes: trigger the full flush on ExtractedView::is_added() instead of RenderVisibleEntities::is_added(). ExtractedView is removed while the camera is inactive or has a zero-sized render target and re-added when the camera comes back, so this one trigger covers camera spawn, camera reactivation, and minimized window restoration. RenderVisibleEntities therefore keeps persisting on the camera entity, restoring the bevyengine#25670 invariant, so the removal in extract_cameras is reverted. - Remove RenderVisibleEntitiesClass::add_entity(): its only callers on main are exactly the call sites rewritten here, so it would be dead code after this PR; sort_added_entities' doc is updated accordingly. (Its removal was deferred to a follow-up PR during the bevyengine#25670 review, so drop this hunk if maintainers prefer that.) Also use `!=` instead of `.ne()` and rename the misnamed `render_mesh_instance_gpu_queues` parameter. Reword the flush comments; document the remaining subview-level limitation for shadow maps; fix the extract_cameras comments.
With the flush keyed on `ExtractedView::is_added()`, removing `RenderVisibleEntities` in `extract_cameras` is redundant for the `is_active` fix. Keeping it restores the bevyengine#25670 invariant (`RenderVisibleEntities` must persist once it has been created) and makes the inactive and minimized paths symmetric. Also document why the zero-size branch must not remove it.
Its only callers were already converted to direct `added_entities` pushes, leaving it as dead code; update `sort_added_entities`' doc accordingly. (Its removal was deferred to a follow-up PR during the bevyengine#25670 review.)
Its only callers were already converted to direct `added_entities` pushes, leaving it as dead code; update `sort_added_entities`' doc accordingly. (Its removal was deferred to a follow-up PR during the bevyengine#25670 review.)
… rendering anything (bevyengine#25690) # Objective This is a followup pr for bevyengine#25670 I have found issues related to toggling `is_active` at runtime causes the rendering to stop working, and this pr fixes that. While fixing on that, ive found another issues when using `NoCpuCulling` on Mesh entity, if the camera is spawned after the entity got collected for rendering, then the camera wont render that entity. The reason why `NoCpuCulling` on Mesh causes this but not on camera, is the gpu mesh collect pass looks for `ViewVisibility` changes, and cpu culling system will trigger the change detection even if the camera is tagged with `NoCpuCulling`. But it wont trigger it when mesh have `NoCpuCulling `. ## Solution For the `is_active` issue, I removed `RenderVisibleEntities` from the render camera when its inactive, but dont remove it when the window is minized. This behaviour matches what bevy other places does, which most places dont care if window is minized. For the second issue, I check to see is `RenderVisibleEntities` added this frame in `collect_gpu_culled_meshes` and do a full table flush when `RenderVisibleEntities` is confirmed to be new. This means a freshly spawned camera will pick up previously registered mesh, and is_active toggling camera will still do the same. Be aware that this is solely for `Mesh3d` tagged with `NoCpuCulling` since thats purpose of this function, any `Mesh3d` not tagged with `NoCpuCulling` will still go through the cpu collect pass instead. ## Testing I used the following functions to toggle is_active field and use gpu culling path, put it in 3d_scene and ssao example. Live test it, the rendering stays the same when toggling is_active at runtime. But the issue remains on bevy/main. ```rust fn use_no_cpu_culling(add: On<Add<Mesh3d>>, mut commands: Commands) { commands .entity(add.entity) .insert(bevy::camera::visibility::NoCpuCulling); } fn set_active(query: Query<&mut Camera>, keyboard: Res<ButtonInput<KeyCode>>) { if keyboard.just_pressed(KeyCode::KeyP) { for mut cam in query { cam.is_active = !cam.is_active; } } } ``` Here is the full code snippet i used for testing both issues. The camera spawning will be delayed by 2sec. On main you will not see anything rendered, but with this pr, rendering is normal. ```rust //! A simple 3D scene with light shining over a cube sitting on a plane. use bevy::{camera::visibility::NoCpuCulling, prelude::*}; fn main() { App::new() .add_plugins(DefaultPlugins) .add_systems(Startup, (scene.spawn(), spawn_cam)) .add_systems(Update, set_active) .add_observer(obs_test) .run(); } /// set up a simple 3D scene fn scene() -> impl SceneList { bsn_list! [ ( #CircularBase Mesh3d(asset_value(Circle::new(4.0))) MeshMaterial3d::<StandardMaterial>(asset_value(Color::WHITE)) Transform::from_rotation(Quat::from_rotation_x(-std::f32::consts::FRAC_PI_2)) ), ( #Cube Mesh3d(asset_value(Cuboid::new(1.0, 1.0, 1.0))) MeshMaterial3d::<StandardMaterial>(asset_value(Color::srgb_u8(124, 144, 255))) Transform::from_xyz(0.0, 0.5, 0.0) ), ( PointLight { shadow_maps_enabled: true, } Transform::from_xyz(4.0, 8.0, 4.0) ), // ( // Camera3d // Transform::from_xyz(-2.5, 4.5, 9.0).looking_at(Vec3::ZERO, Vec3::Y) // ) ] } fn obs_test(add: On<Add<Mesh3d>>, mut commands: Commands) { commands.entity(add.entity).insert(NoCpuCulling); } fn spawn_cam(mut commands: Commands) { commands.delayed().secs(2.0).spawn(( Camera3d::default(), Transform::from_xyz(-2.5, 4.5, 9.0).looking_at(Vec3::ZERO, Vec3::Y), )); } fn set_active(query: Query<&mut Camera>, keyboard: Res<ButtonInput<KeyCode>>) { if keyboard.just_pressed(KeyCode::KeyP) { for mut cam in query { cam.is_active = !cam.is_active; } } } ``` --------- Co-authored-by: Luo Zhihao <luo.zhihao.b@gmail.com>
6 participants
Objective
Fixes #25649
Solution
added_entitiesinsideRenderVisibleEntitiesClassis about to newly added entities, and binning relies on that invariant, but the cpu culling system which usesRenderVisibleEntitieswill get wiped during camera inactive and size eq to 0, 0 time. ButRenderVisibleEntitiesrelies on persistent data to determineadded_entitiesandremoved_entities.So the fix to this is to not remove
RenderVisibleEntitiesduring 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