Rapier context as a Component #545

Vrixyz · 2024-06-27T15:33:17Z

Alternative to support multiple worlds: #328 + lays out the foundation to split RapierContext in multiple elements #502, still grouped in an entity.

By using SystemParams for a DefaultRapierContext, the migration for most users is painless.
Users developing libraries should go through a more involved migration by supporting multiple worlds.

Solved blockers

Fixed by #563

These crashes have been observed, analysis within Vrixyz#26.

Collision panics

Details: Rapier plugin is in PostUpdate schedule, using TimestepMode::Interpolated.

log

thread 'Compute Task Pool (0)' panicked at /home/cornchip/Programming/rust/bevy_rapier/bevy_rapier3d/../src/pipeline/events.rs:71:14:
Internal error: entity not found for collision event.
stack backtrace:
   0: rust_begin_unwind
             at /rustc/524d806c62a82ecc0cf8634b94997ae506f4d6f9/library/std/src/panicking.rs:661:5
   1: core::panicking::panic_fmt
             at /rustc/524d806c62a82ecc0cf8634b94997ae506f4d6f9/library/core/src/panicking.rs:74:14
   2: core::panicking::panic_display
             at /rustc/524d806c62a82ecc0cf8634b94997ae506f4d6f9/library/core/src/panicking.rs:264:5
   3: core::option::expect_failed
             at /rustc/524d806c62a82ecc0cf8634b94997ae506f4d6f9/library/core/src/option.rs:2023:5
   4: core::option::Option<T>::expect
             at /rustc/524d806c62a82ecc0cf8634b94997ae506f4d6f9/library/core/src/option.rs:926:21
   5: bevy_rapier3d::pipeline::events::EventQueue::collider2entity
             at /home/cornchip/Programming/rust/bevy_rapier/bevy_rapier3d/../src/pipeline/events.rs:67:9
   6: <bevy_rapier3d::pipeline::events::EventQueue as rapier3d::pipeline::event_handler::EventHandler>::handle_collision_event
   7: rapier3d::geometry::narrow_phase::NarrowPhase::remove_collider
             at /home/cornchip/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rapier3d-0.21.0/src/geometry/narrow_phase.rs:347:21
   8: rapier3d::geometry::narrow_phase::NarrowPhase::handle_user_changes
             at /home/cornchip/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rapier3d-0.21.0/src/geometry/narrow_phase.rs:302:17
   9: rapier3d::pipeline::physics_pipeline::PhysicsPipeline::detect_collisions
             at /home/cornchip/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rapier3d-0.21.0/src/pipeline/physics_pipeline.rs:129:13
  10: rapier3d::pipeline::physics_pipeline::PhysicsPipeline::step
             at /home/cornchip/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rapier3d-0.21.0/src/pipeline/physics_pipeline.rs:478:9
  11: bevy_rapier3d::plugin::context::RapierContext::step_simulation
             at /home/cornchip/Programming/rust/bevy_rapier/bevy_rapier3d/../src/plugin/context/mod.rs:292:25
  12: bevy_rapier3d::plugin::systems::step_simulation
             at /home/cornchip/Programming/rust/bevy_rapier/bevy_rapier3d/../src/plugin/systems/mod.rs:51:13
  13: core::ops::function::FnMut::call_mut
             at /rustc/524d806c62a82ecc0cf8634b94997ae506f4d6f9/library/core/src/ops/function.rs:166:5
  14: core::ops::function::impls::<impl core::ops::function::FnMut<A> for &mut F>::call_mut
             at /rustc/524d806c62a82ecc0cf8634b94997ae506f4d6f9/library/core/src/ops/function.rs:294:13
  15: <Func as bevy_ecs::system::function_system::SystemParamFunction<fn(F0,F1,F2,F3,F4,F5,F6) .> Out>>::run::call_inner
             at /home/cornchip/.cargo/registry/src/index.crates.io-6f17d22bba15001f/bevy_ecs-0.14.0/src/system/function_system.rs:710:21
  16: <Func as bevy_ecs::system::function_system::SystemParamFunction<fn(F0,F1,F2,F3,F4,F5,F6) .> Out>>::run
             at /home/cornchip/.cargo/registry/src/index.crates.io-6f17d22bba15001f/bevy_ecs-0.14.0/src/system/function_system.rs:713:17
  17: <bevy_ecs::system::function_system::FunctionSystem<Marker,F> as bevy_ecs::system::system::System>::run_unsafe
             at /home/cornchip/.cargo/registry/src/index.crates.io-6f17d22bba15001f/bevy_ecs-0.14.0/src/system/function_system.rs:534:19
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
Encountered a panic in system `bevy_rapier3d::plugin::systems::step_simulation<cosmos_core::physics::collision_handling::CosmosPhysicsFilter>`!
Encountered a panic in system `bevy_app::main_schedule::Main::run_main`!

Different occurence

thread 'Compute Task Pool (1)' panicked at /home/cornchip/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rapier3d-0.21.0/src/dynamics/solver/interaction_groups.rs:440:60:
index out of bounds: the len is 1 but the index is 2
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Encountered a panic in system `bevy_rapier3d::plugin::systems::step_simulation<cosmos_core::physics::collision_handling::CosmosPhysicsFilter>`!
Encountered a panic in system `bevy_app::main_schedule::Main::run_main`!

Strengths

rather low impact on existing users, check the examples.
using a RapierContext as component avoids maintaining our own indices for worlds, and groups up the dependencies of a RapierContext in the same entity. I believe it will be most helpful when ECS relations will be a thing.

Weaknesses

⚠️ “unwrap” heavy code, relying on those components being added correctly: (bevy relations please 🥺🌈) ; most should probably be if let else.
- RapierContextEntityLink on every rapier entities
  - Rigidbodies
  - joints
  - Colliders
  - Others ?
The root “physics object (linked to by RapierContextEntityLink) ; should always have:
- RapierContext -> its acces is simplified with RapierContextAccess
- RapierConfiguration -> as its usage is more advanced, I think it's fine to keep it in queries.
- RenderToSimulationTime -> same as RapierConfiguration
- Maybe I’ve forgotten some..?
⚠️ performance: Configuration::physics_pipeline_active being in the RapierContext entity leads to iteration to all entities for each worlds even though it’s not necessary 🙁 (writeback for example)
- I imagine performance when disabled is not what we should thrive to optimize, but still a pity.
⚠️ Change detection, tracking all these accesses to RapierContext is not trivial, and we should evaluate regressions.
For now all systems share all these things, which I consider low priority to specialize for each RapierContext (them being components would be interesting but not trivial):
- Their schedule
- The timestep mode
- The physics hook
- DebugRenderContext
  - might be easy to migrate to a component ; it does involve a pipeline creation though.
  - So probably keep the pipeline as a resource, but have the enabled flag as a component on each RapierContext.
Testbed uses DefaultRapierContextAccess, I think that’s OK.
there's not a tremendous amount of tests, I noticed I broke events very stealthily so I added a regression test Added 'realistic' test for events #544

Performance analysis

Overall, This pull request doesn't seem to impact significantly the performance based on selected benchmarks:

Comparison with adapted benchmark with baseline from: #551 :

Cubes "master"

     Running benches\cubes.rs (C:\Users\thier\Documents\cargo_target_dir\release\deps\cubes-c309aaa524b91fbb.exe)
Timer precision: 100 ns
cubes                     fastest       │ slowest       │ median        │ mean          │ samples │ iters
├─ cubes_10x10__30_steps  207.6 ms      │ 286.1 ms      │ 216.3 ms      │ 218.1 ms      │ 100     │ 100
├─ cubes_3x3_30_steps     5.451 ms      │ 8.012 ms      │ 5.68 ms       │ 5.851 ms      │ 100     │ 100
╰─ cubes_5x5_30_steps     19.01 ms      │ 22.84 ms      │ 19.67 ms      │ 19.98 ms      │ 100     │ 100

Cubes "This PR"

     Running benches\cubes.rs (C:\Users\thier\Documents\cargo_target_dir\release\deps\cubes-c309aaa524b91fbb.exe)
Timer precision: 100 ns
cubes                    fastest       │ slowest       │ median        │ mean          │ samples │ iters
├─ cubes_10x10_30_steps  206.4 ms      │ 234.1 ms      │ 213.5 ms      │ 215.3 ms      │ 100     │ 100
├─ cubes_3x3_30_steps    5.777 ms      │ 7.041 ms      │ 5.98 ms       │ 6.065 ms      │ 100     │ 100
╰─ cubes_5x5_30_steps    19.4 ms       │ 24.01 ms      │ 19.9 ms       │ 20.4 ms       │ 100     │ 100

many_pyramids3 "master"

     Running benches\many_pyramids3.rs (C:\Users\thier\Documents\cargo_target_dir\release\deps\many_pyramids3-22265334d52714c0.exe)
Timer precision: 100 ns
many_pyramids3               fastest       │ slowest       │ median        │ mean          │ samples │ iters
├─ pyramid_1_with_height_2   20.81 ms      │ 21.44 ms      │ 21.32 ms      │ 21.24 ms      │ 5       │ 25
├─ pyramid_1_with_height_20  1.072 s       │ 1.074 s       │ 1.073 s       │ 1.073 s       │ 2       │ 4
╰─ pyramid_2_with_height_20  4.262 s       │ 4.262 s       │ 4.262 s       │ 4.262 s       │ 1       │ 1

many_pyramids3 "This PR"

     Running benches\many_pyramids3.rs (C:\Users\thier\Documents\cargo_target_dir\release\deps\many_pyramids3-22265334d52714c0.exe)
Timer precision: 100 ns
many_pyramids3               fastest       │ slowest       │ median        │ mean          │ samples │ iters
├─ pyramid_1_with_height_2   21.05 ms      │ 21.98 ms      │ 21.26 ms      │ 21.46 ms      │ 5       │ 25
├─ pyramid_1_with_height_20  1.078 s       │ 1.08 s        │ 1.079 s       │ 1.079 s       │ 2       │ 4
╰─ pyramid_2_with_height_20  4.312 s       │ 4.312 s       │ 4.312 s       │ 4.312 s       │ 1       │ 1

custom_benches "master" 59477d9

average total time: 0.023415528 ms
average rapier step time: 0.00576001 ms
average bevy overhead: 0.017655518 ms
total time: 23.47802734375 ms
average total time: 116.73869 ms
average rapier step time: 116.542145 ms
average bevy overhead: 0.19654846 ms
total time: 11673.889404296875 ms

custom_benches "This PR" deb0a06

average total time: 0.023138916 ms
average rapier step time: 0.0056398925 ms
average bevy overhead: 0.017499024 ms
total time: 23.954345703125 ms
average total time: 115.698875 ms
average rapier step time: 115.48543 ms
average bevy overhead: 0.21344757 ms
total time: 11570.1279296875 ms

…be its own PR

example from dimforge#328 Co-authored-by: Anthony Tornetta <[email protected]>

Co-authored-by: Anthony Tornetta <[email protected]>

src/plugin/systems/multiple_rapier_contexts.rs

src/plugin/systems/remove.rs

src/plugin/systems/writeback.rs

bevy_rapier3d/examples/multi_world3.rs

src/plugin/context/mod.rs

src/plugin/plugin.rs

src/plugin/systems/mod.rs

src/plugin/systems/rigid_body.rs

src/plugin/context/systemparams/rapier_context_access.rs

src/plugin/context/mod.rs

src/plugin/context/systemparams/rapier_context_access.rs

.github/workflows/main.yml

Vrixyz · 2024-07-04T13:27:16Z

I'm reaching a point where I'm confident with the pull request, feedback is most welcome :)

bevy_rapier3d/examples/ray_casting3.rs

src/plugin/context/mod.rs

src/plugin/context/systemparams/rapier_context_access.rs

src/plugin/systems/joint.rs

AnthonyTornetta · 2024-07-05T17:46:23Z

src/plugin/systems/multiple_rapier_contexts.rs

+        }
+        // Verify link is correctly updated for children.
+        let new_rapier_context = world.spawn(RapierContext::default()).id();
+        // FIXME: We need to wait 1 frame when creating a world.


Is this still a problem?

I... believe so. I'll try to track down why exactly.

I'd love a test for that specifically ; I'd love to update this test to be able to test exactly a new RapierContext in the same frame, but I'm not sure on where to insert the user systems exactly for that. Last seems wrong.

src/plugin/systems/writeback.rs

src/plugin/systems/rigid_body.rs

AnthonyTornetta

Looks great! I'm looking forward to this becoming the standard API!

src/plugin/plugin.rs

AnthonyTornetta · 2024-07-08T22:13:42Z

Could we add an easier way to change the fields of RapierContext during its initialization (for the single default-world approach)? Currently I have to write a system that at startup changes those fields, but that seems a bit clunky. Maybe provide the RapierContextInitialization enum with more options?

src/plugin/systems/collider.rs

AnthonyTornetta · 2024-07-08T00:32:51Z

src/plugin/systems/multiple_rapier_contexts.rs

+) {
+    for (entity, new_physics_world) in &q_changed_worlds {
+        let context = q_context.get(new_physics_world.0);
+        if new_physics_world.is_added() {


This caused errors for me, where the rapier link wasn't propagating down to children. Not sure why this check is here.

My initial mental model was that this function is responsible of reacting to rapier context change ; addition is handled within init_rigid_bodies and similar functions.

You're correct though, the initialization is lacking the children propagation, I'll see what I can do.

I think keeping a similar logic is easier to reason about 🤔, rather than making an big system responsible for both initializing and update to link changes.

bevy_rapier3d/examples/multi_world3.rs

Vrixyz · 2024-07-09T15:59:47Z

src/plugin/plugin.rs

+    pub struct TestMarker;
+
+    #[test]
+    pub fn hierarchy_link_propagation() {


This whole test could be in another PR to explore ; I think that's an interesting approach to fully master the system ordering. @AnthonyTornetta, this is the direction I'm taking to address your feedbacks on state inconsistencies (mainly hierarchy propagation, not currently tested but I'll probably add it in that way).

It's full of println! ; I quite like those because it helps debugging the tests ; and it's captured anyway. I'm glad to refactor this with specific instructions if need be. @sebcrozet for info.

src/plugin/plugin.rs

src/plugin/systems/multiple_rapier_contexts.rs

bevy_rapier3d/examples/multi_world3.rs

sebcrozet

This is looking great! Thank you @Vrixyz, @AnthonyTornetta and @Aceeri for the work and reviews!

CHANGELOG.md

sebcrozet · 2024-08-09T09:40:32Z

CHANGELOG.md

+    - `ResMut<mut RapierContext>` -> `DefaultRapierContextAccessMut`
+    - `Res<RapierContext>` -> `DefaultRapierContextAccess`


This is just a suggestion. Bevy has these Read<> and Write<> structs. Perhaps, to make the API look more idiomatic the access structs could be named as follow?

Suggested change

- `ResMut<mut RapierContext>` -> `DefaultRapierContextAccessMut`

- `Res<RapierContext>` -> `DefaultRapierContextAccess`

- `ResMut<mut RapierContext>` -> `WriteDefaultRapierContext`

- `Res<RapierContext>` -> `ReadDefaultRapierContext`

TL;DR: ok :)

I tend to prefer suffixes to prefix, so when you're searching for a term, the alphabetical order comes better grouped.

That said, my initial naming does a bad job at putting Default first. For this reason, I might prefer a RapierContextDefaultRead ; but that would probably mean renaming DefaultRapierContext to RapierContextDefault, which might sound weird.

For the idiomatic way, I wanted to keep close to Res / ResMut, which is very idiomatic. I believe Read/Write is used for individual components, which is not exaclty the case here. I'm not sure I've seen it in SystemParam. But I'm not sure.

In any way, I don't feel too opinionated about that, so I'll just implement your suggestion 😅.

CHANGELOG.md

src/pipeline/events.rs

src/plugin/context/mod.rs

src/plugin/plugin.rs

src/plugin/systems/rigid_body.rs

Vrixyz · 2024-08-12T15:36:24Z

interesting to note differences of system ordering compared to master (from #576)

sebcrozet

Looking fantastic, thanks!

This reverts commit bab3431.

Vrixyz and others added 8 commits June 25, 2024 10:15

working rapierContext as component, but not ideal API.

d4ff57e

systemparam exploration

3cb7676

RapierContextEntityLink to know which RapierContext is referred to

a01395e

more rapierContext conf + broken events -> added a test which should …

4e7c36c

…be its own PR

fix events test

4952ea5

better support for multi world

7a4ba5e

multiple worlds working + example

24fd036

example from dimforge#328 Co-authored-by: Anthony Tornetta <[email protected]>

progress multi world with change

f900dac

Co-authored-by: Anthony Tornetta <[email protected]>

Vrixyz marked this pull request as draft June 27, 2024 15:33

AnthonyTornetta reviewed Jun 27, 2024

View reviewed changes

Vrixyz commented Jun 28, 2024

View reviewed changes

src/plugin/context/systemparams/rapier_context_access.rs Outdated Show resolved Hide resolved

Vrixyz commented Jun 28, 2024

View reviewed changes

src/plugin/context/mod.rs Outdated Show resolved Hide resolved

cargo clippy + pr feedbacks

d171fe4

Vrixyz commented Jul 1, 2024

View reviewed changes

src/plugin/context/systemparams/rapier_context_access.rs Outdated Show resolved Hide resolved

Vrixyz added 2 commits July 2, 2024 09:35

less unwraps

8dd49ee

ci without debug symbols

2b262ea

Vrixyz commented Jul 2, 2024

View reviewed changes

.github/workflows/main.yml Outdated Show resolved Hide resolved

Vrixyz added 6 commits July 2, 2024 16:25

more tests + logs + better changelog

f849cd2

more error logs in case of no rapier context

df3578e

Merge remote-tracking branch 'upstream' into RapierContext_Component

ac16f38

fix conflict

941b52e

fix unwrap

5669a8c

fix warning

0b9ca86

Vrixyz requested a review from sebcrozet July 4, 2024 13:25

Vrixyz marked this pull request as ready for review July 4, 2024 13:25

Vrixyz requested a review from AnthonyTornetta July 4, 2024 13:28

AnthonyTornetta suggested changes Jul 5, 2024

View reviewed changes

address pr feedback

f4643ef

AnthonyTornetta suggested changes Jul 5, 2024

View reviewed changes

src/plugin/plugin.rs Outdated Show resolved Hide resolved

Vrixyz added 4 commits July 8, 2024 11:14

Merge branch 'master' into RapierContext_Component

0e46610

fixed compilation

75c2195

fix doc comment

ccd2be7

fixed joints3 example

9142888

AnthonyTornetta suggested changes Jul 8, 2024

View reviewed changes

experiment on stepping to test system ordering and consistent ecs state

ba0cd76

Vrixyz commented Jul 9, 2024

View reviewed changes

AnthonyTornetta suggested changes Jul 9, 2024

View reviewed changes

src/plugin/plugin.rs Show resolved Hide resolved

Vrixyz added 3 commits July 10, 2024 09:13

fix compilation + warning

6d10fe6

better error handling when rapier context is missing

d0b07a0

add a name for rapier context entity

2676c35

AnthonyTornetta suggested changes Jul 12, 2024

View reviewed changes

src/plugin/systems/multiple_rapier_contexts.rs Show resolved Hide resolved

AnthonyTornetta suggested changes Jul 12, 2024

View reviewed changes

bevy_rapier3d/examples/multi_world3.rs Outdated Show resolved Hide resolved

Vrixyz added 3 commits July 23, 2024 10:44

Merge branch 'master' into RapierContext_Component

40be1f1

rename an init function

deb0a06

apply pr suggestion

cf165bd

sebcrozet approved these changes Aug 9, 2024

View reviewed changes

Vrixyz added 5 commits August 12, 2024 15:31

apply PR suggestions

aeab8c9

Merge branch 'master' into RapierContext_Component

07eefbb

rename rapier context system params

4cccde1

fix clippy (+ random typo)

cbfe138

cargo fmt

5dc5b2d

sebcrozet self-requested a review August 30, 2024 14:31

sebcrozet approved these changes Sep 6, 2024

View reviewed changes

Merge remote-tracking branch 'upstream' into RapierContext_Component

d0961d0

Vrixyz merged commit bab3431 into dimforge:master Sep 9, 2024
4 checks passed

jeyum2 added a commit to jeyum2/bevy_rapier that referenced this pull request Sep 10, 2024

Revert "Rapier context as a Component (dimforge#545)"

efb71e5

This reverts commit bab3431.

AnthonyTornetta mentioned this pull request Oct 20, 2024

Multiple worlds/pipelines #328

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rapier context as a Component #545

Rapier context as a Component #545

Vrixyz commented Jun 27, 2024 •

edited

Loading

Vrixyz commented Jul 4, 2024

AnthonyTornetta Jul 5, 2024

Vrixyz Jul 5, 2024

Vrixyz Jul 5, 2024 •

edited

Loading

AnthonyTornetta left a comment

AnthonyTornetta commented Jul 8, 2024

AnthonyTornetta Jul 8, 2024

Vrixyz Jul 9, 2024

Vrixyz Jul 9, 2024

sebcrozet left a comment

sebcrozet Aug 9, 2024

Vrixyz Aug 12, 2024

Vrixyz commented Aug 12, 2024

sebcrozet left a comment

		- `ResMut<mut RapierContext>` -> `DefaultRapierContextAccessMut`
		- `Res<RapierContext>` -> `DefaultRapierContextAccess`

Rapier context as a Component #545

Rapier context as a Component #545

Conversation

Vrixyz commented Jun 27, 2024 • edited Loading

Fixed by #563

Collision panics

Strengths

Weaknesses

Performance analysis

Vrixyz commented Jul 4, 2024

AnthonyTornetta Jul 5, 2024

Choose a reason for hiding this comment

Vrixyz Jul 5, 2024

Choose a reason for hiding this comment

Vrixyz Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

AnthonyTornetta left a comment

Choose a reason for hiding this comment

AnthonyTornetta commented Jul 8, 2024

AnthonyTornetta Jul 8, 2024

Choose a reason for hiding this comment

Vrixyz Jul 9, 2024

Choose a reason for hiding this comment

Vrixyz Jul 9, 2024

Choose a reason for hiding this comment

sebcrozet left a comment

Choose a reason for hiding this comment

sebcrozet Aug 9, 2024

Choose a reason for hiding this comment

Vrixyz Aug 12, 2024

Choose a reason for hiding this comment

Vrixyz commented Aug 12, 2024

sebcrozet left a comment

Choose a reason for hiding this comment

Vrixyz commented Jun 27, 2024 •

edited

Loading

Vrixyz Jul 5, 2024 •

edited

Loading