From a5616dd194aecfef294d70ad9294d20ac68a49d4 Mon Sep 17 00:00:00 2001 From: Darryl James Date: Sat, 7 Sep 2024 20:12:38 +0900 Subject: [PATCH] Optimize `wake_on_collision_ended` when large number of collisions are occurring (#508) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Objective - With a large number of colliding bodies, I was seeing `wake_on_collision_ended` use almost as much time as `run_substep_schedule` in some cases - I narrowed the slowness down to evaluation of [this if condition](https://github.com/Jondolf/avian/blob/2f83cc15bd804ad82ecd7e080e337d052d1c3472/src/dynamics/sleeping/mod.rs#L280-L285) - Evaluating this once takes around 3μs on my laptop which isn't a huge deal on its own, but when there's 2500 colliders it started to add up to multiple milliseconds ## Solution - In `wake_on_collision_ended`, skip the body of the first for loop (over `sleeping`) if the following both hold true: - The body does not already have a `Sleeping` component - TimeSleeping is 0.0 I believe this is ok because the side-effects of that loop is to remove the `Sleeping` component and to reset `TimeSleeping` to zero, which is exactly the condition it now tests for. Therefore, if the entity is already in that state, there's no point in executing the [relatively costly if statement here](https://github.com/Jondolf/avian/blob/2f83cc15bd804ad82ecd7e080e337d052d1c3472/src/dynamics/sleeping/mod.rs#L280-L285) (as mentioned above). - Since it now also already knows if the `Sleeping` component is present, I gated the `commands.entity(entity).remove::();` calls so that it only adds that command if the component is present - This should save a few lookups on Bevy's end, but isn't strictly necessary for this PR - It may also be worth gating the `sleeping` query on `Without` but I wasn't sure how correct that was - If we think it's worth it, we could add that too so that bodies with sleeping disabled aren't even considered here ## Results Large numbers of colliders (here, 2500) showed a near 1000x improvement in execution time of `wake_on_collision_ended`, going from multiple milliseconds to a few microseconds when none of the bodies are sleeping: 2500 ~~Performance regressed for small numbers of colliders (here, 100), however this is regression at the microseconds level (3.5μs to 15.5μs median), so I posit that this is a worthy tradeoff:~~ (removed; I had the traces backwards) --- ## Changelog - SleepingPlugin's `wake_on_collision_ended` system - `sleeping` query now includes `Has` - first loop in system body skips anything that isn't already sleeping or about to sleep - don't remove `Sleeping` component if `Has`resolved to false --- src/dynamics/sleeping/mod.rs | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/src/dynamics/sleeping/mod.rs b/src/dynamics/sleeping/mod.rs index 44e79a54..423f0823 100644 --- a/src/dynamics/sleeping/mod.rs +++ b/src/dynamics/sleeping/mod.rs @@ -265,10 +265,18 @@ fn wake_on_collision_ended( moved_bodies: Query, (Changed, Without)>, colliders: Query<(&ColliderParent, Ref)>, collisions: Res, - mut sleeping: Query<(Entity, &mut TimeSleeping)>, + mut sleeping: Query<(Entity, &mut TimeSleeping, Has)>, ) { // Wake up bodies when a body they're colliding with moves. - for (entity, mut time_sleeping) in &mut sleeping { + for (entity, mut time_sleeping, is_sleeping) in &mut sleeping { + // Skip anything that isn't currently sleeping and already has a time_sleeping of zero. + // We can't gate the sleeping query using With here because must also reset + // non-zero time_sleeping to 0 when a colliding body moves. + let must_check = is_sleeping || time_sleeping.0 > 0.0; + if !must_check { + continue; + } + // Here we could use CollidingEntities, but it'd be empty if the ContactReportingPlugin was disabled. let mut colliding_entities = collisions.collisions_with_entity(entity).map(|c| { if entity == c.entity1 { @@ -283,7 +291,9 @@ fn wake_on_collision_ended( || moved_bodies.get(p.get()).is_ok_and(|pos| pos.is_changed()) }) }) { - commands.entity(entity).remove::(); + if is_sleeping { + commands.entity(entity).remove::(); + } time_sleeping.0 = 0.0; } } @@ -293,12 +303,16 @@ fn wake_on_collision_ended( if contacts.during_current_frame || !contacts.during_previous_frame { continue; } - if let Ok((_, mut time_sleeping)) = sleeping.get_mut(contacts.entity1) { - commands.entity(contacts.entity1).remove::(); + if let Ok((_, mut time_sleeping, is_sleeping)) = sleeping.get_mut(contacts.entity1) { + if is_sleeping { + commands.entity(contacts.entity1).remove::(); + } time_sleeping.0 = 0.0; } - if let Ok((_, mut time_sleeping)) = sleeping.get_mut(contacts.entity2) { - commands.entity(contacts.entity2).remove::(); + if let Ok((_, mut time_sleeping, is_sleeping)) = sleeping.get_mut(contacts.entity2) { + if is_sleeping { + commands.entity(contacts.entity2).remove::(); + } time_sleeping.0 = 0.0; } }