Skip to content
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

Possible Performance Bug on Physics::step, specifically Query::update_incremental #466

Closed
cybersoulK opened this issue Mar 18, 2023 · 5 comments · Fixed by dimforge/parry#185
Labels
C-Bug Something isn't working

Comments

@cybersoulK
Copy link

in rapier3d 0.17.2, i was having a bug that slows down my game state from 120fps + to 30 fps (the same effect as a memory leak, it keeps lowering down)

When i set the step with Some(&mut self.query_pipeline), instead of manually updating the query_pipeline.

It could be possibly because the ::step uses Query::update_incremental instead of Query::update, and there might be a bug related to update_incremental.

@cybersoulK cybersoulK changed the title Possible Interval Bug on Step, specifically Query::update_incremental Possible Performance Bug on Physics::Step, specifically Query::update_incremental Mar 18, 2023
@cybersoulK cybersoulK changed the title Possible Performance Bug on Physics::Step, specifically Query::update_incremental Possible Performance Bug on Physics::step, specifically Query::update_incremental Mar 18, 2023
@sebcrozet sebcrozet added the C-Bug Something isn't working label Mar 26, 2023
@LPGhatguy
Copy link

We're also running into this issue. In our case, PhysicsPipeline::step doesn't get any slower, but all of our spatial queries get slower over time. In particular, QueryPipeline::cast_ray_and_get_normal grows to be ~12x slower over the course of about a minute running our game.

When we downgrade the game to rapier3d 0.15.0 with the same game, this issue disappears.

Could this be related to dimforge/parry#136? Both issues came about around the same time for us.

@LPGhatguy
Copy link

I was able to fix this regression by changing PhysicsPipeline::step to call QueryPipeline::update instead of QueryPipeline::update_incremental. I think this narrows the issue down to Parry 0.13's new QueryPipeline::update_incremental method.

I've published this change to a fork available here: https://github.com/SecondHalfGames/rapier/tree/update-not-incremental

index 358efed..9301408 100644
--- a/src/pipeline/physics_pipeline.rs
+++ b/src/pipeline/physics_pipeline.rs
@@ -489,7 +489,7 @@ impl PhysicsPipeline {
         );

         if let Some(queries) = query_pipeline.as_deref_mut() {
-            queries.update_incremental(colliders, &modified_colliders, &removed_colliders, false);
+            queries.update(bodies, colliders);
         }

         self.clear_modified_colliders(colliders, &mut modified_colliders);

@LPGhatguy
Copy link

More context as we've investigated this: using update instead of update_incremental is not a perfect fix. It appears that using update instead causes the QVBH to become corrupt. Raycasts and shapecasts stop colliding with a lot of the geometry in our scene with this patch.

Here is our kinematic character controller interacting with a table in our game, using rapier3d 0.17.2:

cyrtanthus-desktop_0faIMXrekH.mp4

And here is the exact same code and scene, but on rapier3d master + our patch above:

cyrtanthus-desktop_qmGWdxCJaH.mp4

As you can see, something about using QueryPipeline::update instead of QueryPipeline::update_incremental breaks the query pipeline. The first way we noticed this manifesting is that this patch also causes about half our zombies to fall through the floor.

We're in kind of a tough spot with this issue. rapier3d 0.15.0 works but is missing a few features that we need. rapier3d 0.17.2 has this performance regression, which is a show-stopper for us. Our patch fixes the performance issues but creates new problems.

Is there any information (or funding) we can provide to help track down this issue sooner than later? Thanks!

@LPGhatguy
Copy link

Last update on this! Passing None to PhysicsPipeline::step and updating the query pipeline myself works fine. No performance regressions or weird issues.

@sebcrozet
Copy link
Member

This definitely indicates a bug in QueryPipeline::update_incremental.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants