Allow avoidance of exclusive World's ownership from Testbed

[Vrixyz]

Referencing #131

Current state

I succeeded in compiling:

• balls2d official nphysics example, which is working fine.
• my target repository compiled and is now working

I'm opening the pull request to show my progress, and discuss about the shortcomings.

TODO List

• make specs and nphysics compatible for external body additions
  • propagate possible world modifications after callback
• allow to avoid heavy Arc<Mutex<nphysics::World>> if not required by (library) user.
  • clean files (split WorldOwner trait and impl from Testbed)
  • optional: attempt to play with implicit From<nphysics::World> for no impact on current examples
• keep worldOwner parameter to callback for convenience
• optimize locks with RwLock instead of Mutex
• make all examples compile and working - Allow avoidance of exclusive World's ownership from Testbed #133 (comment) means there are no source code modifications needed
• Fix CI: examples sensor2 and bodystatus2 use callback whose signature changed.
• 🚧 test on Web assembly target. (I don't know how to test it)
• 🚧 update documentation. (i'm not sure what to add)
• make the same changes to testbed3d : @sebcrozet volunteered to do it

Tasks this PR will certainly not cover

They should be copied over to #131 to keep track of them.

• avoid boxing and prefer impl : as commented, I couldn't make it work.
• alternate simpler example without Dispatcher and its need of Mutex. I did not succeed in applying this idea

Failed attempts

This section is only for reference, to help understand why some choices have been made.

Very first attempt

Within Testbed, I tried to replace World by &mut World, no chance in compiling ever after.

Second attempt

Within Testbed, I tried to add a "WorldOwner", which objective was to own a reference to a &mut World, then have a retrieve function to access it.

I lost myself again in complex lifetime annotations.

Third attempt

I started small, read some articles (again)
Then, I used RefCell<World> to store a mutable world inside Testbed.
Once compiling and at least 1 official example working, I used Rc<RefCell<World>.

Balls2d compiled successfully, but not my reproduction repository : indeed, specs::System needs its resources to be sync::Send to be sent beetween threads.

So I tried to Arc<RefCell<World>>, but I quickly realized I had no choice but to use Arc<RefCell<World>>, since RefCell is not sync::Send.

Minor mistakes

I thought I had a deadlock, but it was actually lack of gravity 🙄; then graphicsManager was not called when modifying nphysics::World, so it leads to a crash when attempting to modify color when moving an object. I fixed it by updating graphics nodes after the update loop.

[Ralith]

I don't think specs requires shared ownership, much less explicit locking, of world state.

[Vrixyz]

For clarification, the World I am putting in Mutex is the one from nphysics, not the one from specs.

The errors I had concerning sync::Send were about putting the nphysics::World into specs::System.

I'll update the description to make it more explicit.

That being said, you might be right, I didn't find any mentions of sync::Send in specs either, but the error messages I had were pretty clear...

For now I focus on making my use case work, then I'll try to optimize the data scheme used.

Thanks for your feedback.

[Ralith]

You probably encountered the requirements on DispatchBuilder::with. These requirements do not apply to all specs systems; indeed you don't need to use dispatchers at all. specs supports mutable data that isn't behind mutexes in many ways, one of which is by storing it as a resource in the specs world.

[Vrixyz]

That must be it ! Thanks 🙏 Dropping dispatcher for systems requiring nphysics::World might be a solution indeed then :)

Applying Rc<RefCell<nphysics::World>> to Testbed is still a requirement though from my understanding.

I’ll make examples from all these informations to help visualizing impacts. (When I have time)

[Vrixyz]

After more testing, this Pull Request kinda works, except for graphics updates.

I added this snipper to handle new additions to nphysics::Wold, just before drawing.

for co in physics_world.colliders() {
    if self.graphics.body_nodes_mut(physics_world, co.data().body()).is_none() {
        self.graphics
            .add(window, co.handle(), &physics_world);
        }
}

I think that will not handle modifications to existing 2d bodies, but that's a start, I suggest I focus my efforts on supporting new bodies additions during runtime before going in edge cases like modification, deletion, etc.

This Pull request is working at the moment for my (very limited use case).

• I'll try to lay out a clear checklist below of what is needed for this Pull request to be accepted (if at all: The end choice of merging my pull request is of course not mine to make)

This checklist is at top level description for readability purposes.

[Vrixyz]

I moved that outside of the running check (if self.running != RunMode::Stop {), because I thought this belonged more in the "draw" logic than the update. It allows me to easily share the same mutex for graphics update and graphics rendering.

[sebcrozet]

This was inside of the running check because the self.graphics.draw(...) is actually not very well named. All it does is to synchronize the positions of the graphics objects with the position of the physics colliders. Performing this update is not necessary if the simulation is not running.

[sebcrozet]

Thank you for attempting this! The approach seems sound overall, besides a few details that could be improved to avoid the user having to know about the Arc<Mutex<...>> in most examples.

One thing that will have to be tested (but I can do this test if you want) is that it still works on WASM.

Don't feel forced to do the same for the 3D testbed, especially since you don't use it.

[sebcrozet]

This should be avoided for several reasons:

1. It means all the testbed examples will have to be modified.
2. This complicates the example with something that is useful only in some corner-use-cases.
3. I don't want users looking at the examples to think using an Arc<Mutex<...>>> is always necessary. (Someone quickly skimming through the examples just to see what they look like will notice the use of Arc and Mutex so they may end up concluding too quickly).

An alternative is to keep the Testbed::new(world) as it is, and have a method from the testbed to retrieve the shared world: testbed.world().

[Vrixyz]

My alternative take on this was to make different instantiaters to pass world to Testbed, from my WIP local branch:

/// This trait is designed to allow choosing implementation of underlying storing of World: shared between threads or owned only by Testbed.
pub trait WorldOwner {
    fn get<'a: 'b, 'b>(&'a mut self) -> Box<DerefMut<Target = World<f32>> + 'b>;
}

pub struct WorldOwnerShared {
    world: Arc<Mutex<World<f32>>>,
}

impl WorldOwnerShared {
    pub fn new(w: World<f32>) -> WorldOwnerShared {
        WorldOwnerShared {world: Arc::new(Mutex::new(w))}
    }
}

impl WorldOwner for WorldOwnerShared {
    fn get<'a: 'b, 'b>(&'a mut self) -> Box<DerefMut<Target = World<f32>> + 'b> {
        Box::new(self.world.lock().unwrap())
    }
}
// TODO: simpler WorldOwnerExclusive implementation not requiring mutex

that would impact my use case example as

let mut testbed = Testbed::new(Testbed::WorldOwnerShared(world)));

And existing simpler examples as

let mut testbed = Testbed::new(Testbed::WorldOwnerExclusive(world)));

Maybe the world itself could implement WorldOwner, so the existing examples wouldn't be impacted at all ? 💃

Edit: a From<nphysics::World> to implement for both WorldOwners might seem an even better idea, I'll play with the idea

Your suggestion makes it difficult or overkill to share TestBed with specs::System, in order to retrieve nphysics::World. Please tell me if my lead is incorrect so I don't spend time in a incorrect direction.

All this discussion is underlying to the goal of objective

allow to avoid heavy Arc<Mutexnphysics::World> if not required by (library) user."

from the TODO List

[sebcrozet]

Your suggestion makes it difficult or overkill to share TestBed with specs::System, in order to retrieve nphysics::World.

There is no need to share the testbed with specs. What I was thinking of would mean your example would look like that:

    // Initialize the testbed first.
    let mut testbed = Testbed::new(physics_world);

    let ecs_world = std::cell::RefCell::new(specs::World::new());

    // In the following, we have testbed.world() -> Arc<Mutex<World<f32>>
    ecs_world
        .borrow_mut()
        .add_resource(testbed.world());
    let dispatcher = DispatcherBuilder::new()
        .with(DummySystem::new(), "dummy_system", &[])
        .build();

    let dispatcher = std::cell::RefCell::new(dispatcher);

);

Basically, the testbed is responsible for creating the Arc internally.

---

If think your solution based on WorldOwner is better than what I proposed above. The testbed could be parameterized by: Testbed<W: WorldOwner>, and instead of returning Box<DerefMut<Target = World<f32>> + 'b>>, the .get method you propose could return impl DerefMut<Target = World<f32>> + 'b> to avoid boxing.

As you proposed, both World, Arc<RwLock<World>> and Arc<Mutex<World>> should implement this WorldOwner trait so no modification of any testbed example is required (even when user callbacks are used since they would become Fn(&mut T, &mut GraphicsManager, f32).

I don't think you will be able to make a solution based on From<nphysics::World> without requiring the user to add explicit type annotation for the testbed though.

[Vrixyz]

What I thought about bypasses modifications of existing examples 🎉 :

pub fn set_world(&mut self, world: World<f32>) {
    self.set_world_owner(Box::new(WorldOwnerExclusive::from(world)));
}

/// Lower level setter allowing for shared World behaviour
pub fn set_world_owner(&mut self, world: Box<WorldOwner>) ...

I tried experiencing with Into and From for World and Arc<RwLock>, but didn't succeed in making the situation better, I'll stop there my experiments on set_world API modifications and consider it OK enough.

[Vrixyz]

I did try to make get return a impl WorldOwned, but I failed at making it compile again, so I'll let this as an exercise to whoever is more confortable with that.

[sebcrozet]

This was inside of the running check because the self.graphics.draw(...) is actually not very well named. All it does is to synchronize the positions of the graphics objects with the position of the physics colliders. Performing this update is not necessary if the simulation is not running.

[sebcrozet]

I am not very fond of this, though I understand why this can be necessary to avoid dead-locks if, e.g., the user locks the physics world while inside of the callback. Maybe this could at least take &mut Testbed as its first argument, so the user can call testbed.world() to retrieve the shared world (see my first comment for the definition of testbed.world()).

[Vrixyz]

ok, or pass the self.world without locking it, to let the user choose between using the parameter or handling himself the dependency

[sebcrozet]

Yes, that will work too!

[Vrixyz]

For reference: passing &mut Testbed in callback caused cannot reborrow mutably error and that's why I just plain removed it.

passing the self.world needs Testbed to be generic over its WorldOwner real type. I wanted to avoid adding complexity, but as you suggested it in other comments, I'll look into it.

[sebcrozet]

Is there a reason why you used a Mutex instead of a RwLock? The latter should be more flexible by allowing multiple simultaneous read-only locks.

[Vrixyz]

@sebcrozet You wrote somewhere that webasm should be tested.

I'm not very comfortable with webassembly and rust, so if you have resources on how to make it work with nphysics, I may follow some guide.

Until then, I think this Pull Request is good for "final" reviews.

Unchecked items from the top TODO List will need more guidance for me to check them. That is, if they are strictly needed, that is for you to decide.

Concerning CI, I'm not sure why it's failing, I had no problem on mac nor on windows, making cargo build from nightly and 1.28.
I suspect some caching or incorrect dependency is in play.

[sebcrozet]

@Vrixyz Thanks, this PR seems pretty good and almost ready now! I appreciate that your changes don't impact the examples, except for those that fail to compile on travis.

The reason why some examples fail to compile on travis come from the fact the sensor2 and body_status2 register callbacks that need to be slightly modified to account for the change of parameter type. For example: https://github.com/sebcrozet/nphysics/blob/master/examples2d/sensor2.rs#L91 should be modified to call the .getor .get_mut when needed on the world owner.

For testing on WebAssembly, you may follow that tutorial on the user guide: http://nphysics.org/wasm_compatibility/ For testing, you can do this on the examples2d repository, and ensure only one of the examples remain on the Cargo.toml so it is the only one being compiled.

I will take some time next week to finalize this PR (by performing similar changes to the 3D testbed) and merge it when CI succeeds. If you want, I can do the WebAssembly tests next week as well.

[Vrixyz]

Thanks, I'll attempt to fix 2d examples, it sound simple to fix.

Once done, I'll make another go at the webassembly testing.

After that I'll stop touching this PR, I'll keep you updated, that should be soon enough.

[Vrixyz]

There's definitely something I'm missing for understanding and testing webasm : when I try to load target/deploy/index.html, I have a blank page. Nervermind.

I'm stopping here, CI is green 🎉

[sebcrozet]

I've tested with wasm32-unknown-unknown and it works as expected. I can finally merge this. Thanks @Vrixyz!