Skip to content

Commit

Permalink
Optimize wake_on_collision_ended when large number of collisions ar…
Browse files Browse the repository at this point in the history
…e occurring (#508)

# 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::<Sleeping>();` 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<SleepingDisabled>` 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:
<img width="569" alt="2500" src="https://github.com/user-attachments/assets/9785ddfb-0270-474a-bfda-c83a830f011e">

~~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<Sleeping>`
    - first loop in system body skips anything that isn't already sleeping or about to sleep
    - don't remove `Sleeping` component if `Has<Sleeping>`resolved to false
  • Loading branch information
datael committed Sep 7, 2024
1 parent 2f83cc1 commit a5616dd
Showing 1 changed file with 21 additions and 7 deletions.
28 changes: 21 additions & 7 deletions src/dynamics/sleeping/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -265,10 +265,18 @@ fn wake_on_collision_ended(
moved_bodies: Query<Ref<Position>, (Changed<Position>, Without<Sleeping>)>,
colliders: Query<(&ColliderParent, Ref<ColliderTransform>)>,
collisions: Res<Collisions>,
mut sleeping: Query<(Entity, &mut TimeSleeping)>,
mut sleeping: Query<(Entity, &mut TimeSleeping, Has<Sleeping>)>,
) {
// 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<Sleeping> 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 {
Expand All @@ -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::<Sleeping>();
if is_sleeping {
commands.entity(entity).remove::<Sleeping>();
}
time_sleeping.0 = 0.0;
}
}
Expand All @@ -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::<Sleeping>();
if let Ok((_, mut time_sleeping, is_sleeping)) = sleeping.get_mut(contacts.entity1) {
if is_sleeping {
commands.entity(contacts.entity1).remove::<Sleeping>();
}
time_sleeping.0 = 0.0;
}
if let Ok((_, mut time_sleeping)) = sleeping.get_mut(contacts.entity2) {
commands.entity(contacts.entity2).remove::<Sleeping>();
if let Ok((_, mut time_sleeping, is_sleeping)) = sleeping.get_mut(contacts.entity2) {
if is_sleeping {
commands.entity(contacts.entity2).remove::<Sleeping>();
}
time_sleeping.0 = 0.0;
}
}
Expand Down

0 comments on commit a5616dd

Please sign in to comment.