Skip to content

Use RenderStartup for occlusion_culling example. #20184

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

Merged

Conversation

andriyDev
Copy link
Contributor

Objective

Solution

  • This situation is very weird. We are trying to get features of the renderer back into the main world. We use an ArcMutex to communicate between the two worlds.
  • We now insert the resource into both worlds, but only with Option::None.
  • The RenderStartup init system initializes the resource with the feature values populated.

This does result in a "regression" where there is a single frame where the update_status_text system doesn't react properly, since it doesn't have the features from the render world yet.

Testing

  • Ran the occlusion_culling example and it worked!

@andriyDev andriyDev added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 18, 2025
@andriyDev andriyDev force-pushed the startup-occlusion-culling-ex branch from ed16ddc to 866a6af Compare July 18, 2025 07:23
@andriyDev andriyDev requested review from IceSentry and atlv24 July 18, 2025 07:50
Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

I think this makes much more sense than finish. Running the example, I find the same behavior.

I thought I could improve readback_indirect_parameters with the following diff, but it might be better as a follow up PR

Click here to reveal diff
diff --git a/examples/3d/occlusion_culling.rs b/examples/3d/occlusion_culling.rs
index ee0c4029b9..8e133dd789 100644
--- a/examples/3d/occlusion_culling.rs
+++ b/examples/3d/occlusion_culling.rs
@@ -618,23 +618,17 @@ fn readback_indirect_parameters(
     };
 
     // Read the GPU buffers back.
-    let saved_indirect_parameters_0 = (**saved_indirect_parameters).clone();
-    let saved_indirect_parameters_1 = (**saved_indirect_parameters).clone();
+    let saved_indirect_parameters_clone = (**saved_indirect_parameters).clone();
+
     readback_buffer::<IndirectParametersIndexed>(data_buffer, move |indirect_parameters| {
-        saved_indirect_parameters_0
-            .lock()
-            .unwrap()
-            .as_mut()
-            .unwrap()
-            .data = indirect_parameters.to_vec();
+        let mut saved_indirect_parameters = saved_indirect_parameters_clone.lock().unwrap();
+        saved_indirect_parameters.as_mut().unwrap().data = indirect_parameters.to_vec();
     });
+    let saved_indirect_parameters_clone = (**saved_indirect_parameters).clone();
+
     readback_buffer::<u32>(batch_sets_buffer, move |indirect_parameters_count| {
-        saved_indirect_parameters_1
-            .lock()
-            .unwrap()
-            .as_mut()
-            .unwrap()
-            .count = indirect_parameters_count[0];
+        let mut saved_indirect_parameters = saved_indirect_parameters_clone.lock().unwrap();
+        saved_indirect_parameters.as_mut().unwrap().count = indirect_parameters_count[0];
     });
 }

@@ -620,10 +621,20 @@ fn readback_indirect_parameters(
let saved_indirect_parameters_0 = (**saved_indirect_parameters).clone();
let saved_indirect_parameters_1 = (**saved_indirect_parameters).clone();
readback_buffer::<IndirectParametersIndexed>(data_buffer, move |indirect_parameters| {
saved_indirect_parameters_0.lock().unwrap().data = indirect_parameters.to_vec();
saved_indirect_parameters_0
.lock()
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a fan of those massive chains, but not sure how to solve it either 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One "solution" is to do what I did initially: rather than making the struct hold on Option, just default the inner struct (with false for the supported features), and then just reassign it later. I didn't want to do that though since it isn't very rusty and we now have a resource that has "wrong" values.

Another alternative I considered was initializing the resource in RenderStartup and then clone it over to the main world in ExtractSchedule. That would mean extraction is "going the other way" which is weird, and the main world would have to live with the resource being missing for a frame.

This example feels a little overcomplicated, but it's also hard to show users the effect of occlusion culling, so alas, we live with what we have.

Copy link
Contributor

@nicopap nicopap Jul 18, 2025

Choose a reason for hiding this comment

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

On a purely esthetic level, it's possible to get rid of them with an assignment, I gave a patch in the expandable in my review (just click on > Click here to reveal diff)

@andriyDev andriyDev added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 18, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jul 21, 2025
Merged via the queue into bevyengine:main with commit e45a77a Jul 21, 2025
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants