bevy_rapier icon indicating copy to clipboard operation
bevy_rapier copied to clipboard

Multiple worlds/pipelines

Open AnthonyTornetta opened this issue 2 years ago • 9 comments

This closes #221 by allowing multiple worlds to be used in parallel.

Reasoning

Currently, bevy_rapier restricts its usage to a single world per game, which is limiting in many situations, despite rapier itself having no restriction on the amount of worlds it supports. This PR changes RapierContext to instead store a list of RapierWorlds which hold what RapierContext used to hold. Each world is addressable be a unique WorldId, which can be attached to any entity via the PhysicsWorld component. The physics world simply stores the world's id.

Goal

The goal of this PR was to add support for any number of worlds to bevy_rapier with minimal breaking changes to projects already using this that are content with only one world. The only changes that will be made to such projects are as follows:

  • raycast calls will now need the DEFAULT_WORLD_ID provided as the first argument and you will need to call .expect("The default world should always exist") before using their result.
  • If you provided custom arguments to the RapierContext, you will instead need to do:
RapierContext::new(RapierWorld {
  your custom stuff,
  ..Default::default()
})
  • Similarly, to access those items you now have to do context.(get_world/get_world_mut)(DEFAULT_WORLD_ID).expect("Default world should exist").field_you_wanted
  • Some other things may need to be adjusted to first get the world, but these were the main ones that would affect the most projects.

Example

I have provided a 3d example of the multiple worlds in action in multi_world3.rs. image

Each of those platform/block pairs is in its own physics world, where they do not interact with each other.

A GIF of a cube changing its world every 3 seconds and moving between platforms in different worlds found in change_world3.rs: change_world3

PhysicsWorld

This is a component that may be attached to any entity where you want to specify its world. If this component is omitted, the entity will be added to the DEFAULT_WORLD_ID world, which is present in every game. If this component is attached, the entity will be added to the specified world. If the component is modified, the entity should switch the world that it is in to reflect the id it was changed to.

To add or remove a world, just use RapierContext::add_world and RapierContext::remove_world respectively.

AnthonyTornetta avatar Feb 14 '23 23:02 AnthonyTornetta

This has been updated to bevy 0.10

AnthonyTornetta avatar Mar 07 '23 23:03 AnthonyTornetta

Thanks for adding this, it will be immensily useful for my game!

If you can forgive some bikeshedding, could we rename BodyWorld to something else? For me it has a connection to rigidbody, even though it also applies to just colliders. Also, it's not immediately clear that this is related to physics specifically. I propose a name like PhysicsWorld to make it more generic and directly link it to the fact that it's a physics primitive.

What do you think?

Alainx277 avatar Mar 21 '23 12:03 Alainx277

@Alainx277 This was actually something I pondered when naming it to BodyWorld. At the time I thought it was good enough, but did lack some clarity. But, since you noticed it too, I do agree that calling the property PhysicsWorld would be more clear and definitely indicate it has to do with the physics engine -- I can't believe I didn't think of it haha! Thanks for the feedback!

AnthonyTornetta avatar Mar 22 '23 01:03 AnthonyTornetta

Updated to bevy 0.13

AnthonyTornetta avatar Feb 25 '24 01:02 AnthonyTornetta

@Vrixyz

Thanks for the feedback!

Developer experience for users not interested in multi-world, ideally they wouldn't be impacted.

This is a real issue that should absolutely be addressed. For the most part these changes did avoid impacting a large portion of users, but there are some methods that have to have their method headers change in a multi-world context. You noted the raycast method, but many of the public methods in the RapierContext struct had to have a WorldId added to their argument list.

There are a couple solutions to this that I have considered:

  1. Omitting the world id parameter, and instead applying those methods through every world.

This has a couple implications. If you are operating in an environment where you know which world you want to work with, you could do context.get_world(world_id).method(...) instead of context.method(...). The biggest con with this approach is the overhead of having to remember to do context.get_world before every physics method, which could be very easy to forget. By making it a parameter in the context-level methods, it would be impossible to forget.

This can also lead to hard-to-notice bugs. Take, for example, performing a raycast. If I have multiple worlds and do a raycast, it almost never makes sense to raycast in every single world and return the first hit or a list of hits throughout all my worlds. Every raycast should realistically only be done in the context of one physics world. As such, not changing the publicly facing API at all, would lead to a less sensible API and some hidden footguns. Ultimately I decided not to go with this approach to avoid these footguns.

  1. Compiler flags to statically change method signatures based on wanted features

Compiler feature flags could definitely be used to not change the public API for those that don't want multiple worlds, and make the current changes usable for those that do want multiple worlds.

One problem with this is that it could result in writing two APIs for one library. This could lead to some harder-to-maintain code and result in two sets of tests having to be created for every method. The biggest problem with this, however, is that plugins that rely on bevy_rapier may then be incompatible with those that rely on them but opt to use multiple worlds. Due to the public API for methods being different for single/multi worlds, most plugins would likely be built for apps that don't enable the multiple worlds feature. Any library that then wanted to support both multi and single world versions of the plugin, would then have to include their own feature flags and flag different sections of their own code. While this approach is effective in preventing non-libraries from having the changing their code, it could result in libraries having to potentially double their physics-related code and add new feature flags.

  1. Multiple Resources

If we add a new Resource for multiworld usecase: a RapierContextMultipleWorlds (effectively this branch RapierContext)

  • RapierContext would be incompatible with RapierContextMultipleWorlds: configurable on the plugin or check if one or the other already exist.
  • internal systems which need RapierContext would need to target either one depending on their availability For practicality, I think we can keep the PhysicsWorld identifier on entities

I might be overlooking technical details so let me know if that's unreasonable, thanks again !

This suffers from the same problems as the compiler flag approach and has more problems on its own. We would have to double the API and testing surface area (perhaps even more with this approach), and it would result in libraries having to double their code. The biggest additional flaw with this approach is the accidental usage of the wrong version of the RapierContext. You have to always be aware if you should be using RapierContext or RapierContextMultipleWorlds, which could be especially difficult for those working on multiple projects at once. This approach would create very easy to shoot footguns both for libraries and end-users.

  1. Changing the public API for everyone

This is the approach I ultimately went with, but it (as you have noticed) isn't without its flaws. Most methods in the RapierContext struct will have to be changed to return a Result that would indicate if the world no longer exists. We could, of course, make it panic instead, but I thought that would be a very scary change. In addition, the method signatures would have to be changed to support the WorldId specifier. This is a substantial API change for those that extensively used RapierContext methods.

One potential pro of changing the public API is that it will make it more well-known that bevy_rapier supports multiple physics worlds simultaneously.

I decided to go with this approach because I thought the cons of this approach were less severe than the cons of the previous approaches. This approaches avoids the footguns of not changing the public API at all, and avoids the problems that come with only changing it for some. If you have any other viewpoints about these methods, I would absolutely love to hear your opinions.

Overall

Overall, I think changing the public API isn't the worst possible outcome. Bevy users are used to public APIs changing every 3 months with engine updates, so I don't think it be a huge strain on them. In addition, it avoids the problems of making 2 APIs for two groups of users while letting them know that multiple worlds are now supported. Of course, changing public APIs is never ideal, but I don't think it's possible to not changing it without causing worse consequences for libraries and for the future.

Of course, if you (or anyone else) has a different opinion on this, I would love to hear your side.

Performance & perf measurements

I haven't done any official or reliable performance measurements comparing the before + after changes, but that should be done & measured before this gets officially committed into the main branch. As a personal anecdote (and therefore meaningless), I haven't noticed any degradation in performance for my game that has been using this version of bevy_rapier for over a year.

How could this be supported well in correlation with #502

Doing #502 work within this already huge PR would create an even bigger PR, so I would definitely prefer not to do it within this PR specifically. But, for after this is merged in, it would heavily depend on how you wanted to approach splitting bevy_rapier up into multiple plugins. I'm not very familiar with bevy_xpd's architecture, but I can make a few guesses. I noticed that xpd relies on different resources for different steps of the physics pipeline, each of which is created by separate plugins.

I assume this would be the approach used if rapier does split up into multiple plugins. This approach would be more difficult with multiple worlds, due to each world needing to have their own instances of the steps, but I don't think it would be too difficult. Rather than treating these as resources, where only one instance can be used, we could instead have rapier take a generic type for each step of the physics pipeline we want to control via plugins and treat them as components. Then, instead of adding worlds to some resource, an entity could be created to represent the world, where each physics step was added as a component to that entity. This would be a massive architecture change, but I think using entities in an ECS is never a bad idea, and would put more power into the end users' hands.

Of course, this approach isn't perfect. Invalid states could be more easily represented (by the improper addition/removal of components on these world entities), but I think it would still be a step in a better direction. I would be interested to further discuss this and any other ideas/directions you (or others) have!

AnthonyTornetta avatar May 21 '24 00:05 AnthonyTornetta

Of course https://github.com/dimforge/bevy_rapier/issues/502 is outside the scope of this PR, yes 😄

I believe RapierContext as Component is the best way forward ; it could be made easier for the users via a SystemParam similar to how bevy_egui supports multiple windows (https://github.com/mvlabat/bevy_egui/blob/6021f22a0e69ed802844a259d4b43d94007027ec/src/lib.rs#L341).

Thanks again for the PR, detailed response and your patience, it helps with understanding the impacts !

ThierryBerger avatar May 29 '24 13:05 ThierryBerger

I've made some updates while updating this branch to not have any conflicts, two of which will change the public API:

  • WorldId ID to events (as mentioned above):
    • The WorldId is now included in the CollisionEvent enum. This will cause the public API to change once more, but since it's pretty easy to ignore this field I don't think it's a huge problem.
  • Changed where gravity is defined
    • The integration rework proved problematic for this PR, since the RapierContext resource has been restructured. One of the things that I had to move to make understandable was the gravity field. This is now stored in each world, rather than in the RapierContext. As such, you'll have to do either RapierWorld::set_gravity or RapierWorld::with_gravity, the latter being a builder-like method that returns self.

AnthonyTornetta avatar May 29 '24 18:05 AnthonyTornetta

Thanks again for your ongoing discussion on this topic !

For information, I'm starting to study bevy_rapier API in order to offer a few improvements, mainly RapierContext usage, but I'd love to support your use-case with those API improvements.

The exact details of these improvements are still being worked on, and I'd like to familiarize myself with the design implications and understand in practice the changes necessary for supporting multiple worlds, so I'll be exploring on my own branch from master.

For multi-world feature, I will probably transition RapierContext to be a Component, and explore SystemParams to offer good ergonomics for single world users. In parallel, I will study bevy_xpbd to compare approaches.

I think this particular Pull Request is unlikely to be merged as-is, but thanks again for the work which will prove handy to compare approaches, and the discussions to find a good path forward ! I'm only stating my own feeling, and to manage expectations, I look forward to further discussions on this not trivial feature 😄.

ThierryBerger avatar Jun 24 '24 08:06 ThierryBerger

I agree that moving forward with the "componentization" of RapierContext is a good choice. Given that is the direction you're going, I agree that merging these changes in would be pointless, given that many would have to be redone anyway. I would be happy to help out with this architectural shift if you would like any, and if you have questions you want to ask me outside of github, I'm in the dimforge discord server and my username is Cornchip.

I will continue to keep this branch updated for now, since I am using it in my own project. Thanks for taking interest in supporting multiple worlds! I'm sure a lot of people (besides me) will appreciate it.

AnthonyTornetta avatar Jun 25 '24 14:06 AnthonyTornetta

Closing in favor of #545

AnthonyTornetta avatar Oct 20 '24 19:10 AnthonyTornetta