Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiple worlds/pipelines #328

Open
wants to merge 98 commits into
base: master
Choose a base branch
from

Conversation

AnthonyTornetta
Copy link

@AnthonyTornetta AnthonyTornetta commented Feb 14, 2023

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 AnthonyTornetta changed the title Multiple worlds Multiple worlds/pipelines Feb 22, 2023
@AnthonyTornetta
Copy link
Author

This has been updated to bevy 0.10

@AnthonyTornetta
Copy link
Author

Updated to bevy 0.13

Copy link
Contributor

@Vrixyz Vrixyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work and updating it! I'd like to see bevy_rapier support multiple physics worlds, it's not a minor change though so I'd like to discuss the approach.

My 3 main concerns are :

  • developer experience for users not interested in multi-world, ideally they wouldn't be impacted.
  • performance: adding a component for each rigidbody (PhysicsWorld) seems alright ; and I didn't see anything shocking concerning iterations, still I'd like to take some time to verify with perf measurements.
  • How could this be supported well in correlation with Split up RapierContext #502

This PR

You chose to move the functionality of RapierContext to a new RapierWorld ; then change RapierContext to have multiple Worlds.

I'd like to explore if it's possible to not alter the existing API ?

No breaking changes ?

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 !

Comment on lines +222 to +223
collision_events: &mut self.collision_events_to_send,
contact_force_events: &mut self.contact_force_events_to_send,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, the change to store the RwLock is not stricly necessary, just useful to avoid the RwLock::new within the simulation step ?

Alright to me 👍 ; Would it make sense to add a PhysicsWorld id to these events ? Maybe as a follow up.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also agree that adding a PhysicsWorld field to those events would be a good idea, even though you could just query it in the event handlers. Since it's just a usize there would be basically no overhead and make it more convenient for some use cases.

true,
QueryFilter::only_dynamic(),
)
.expect("Default world should exist.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This user facing API change is unfortunate, I'd like to discuss if alternatives could be achievable, more details in main PR comment feed.

Comment on lines +24 to +37
debug-render-2d = [
"bevy/bevy_core_pipeline",
"bevy/bevy_sprite",
"bevy/bevy_gizmos",
"rapier3d/debug-render",
"bevy/bevy_asset",
]
debug-render-3d = [
"bevy/bevy_core_pipeline",
"bevy/bevy_pbr",
"bevy/bevy_gizmos",
"rapier3d/debug-render",
"bevy/bevy_asset",
]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: These changes are welcome ❤️, but out of scope and adds up to a lot of noise for an already non trivial Pull Request. It's not blocking but an ideal situation would be to isolate them in another PR, or not include them.

Comment on lines +1808 to +1816
// fn find_world(context: &mut RapierContext) -> &mut RapierWorld {
// for (_, world) in context.worlds.iter_mut() {
// if let Some(handle) = item_finder(world) {
// return Some((world, handle));
// }
// }

// None
// }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed anymore ?

@Vrixyz Vrixyz added D-Difficult Needs strong technical background, domain knowledge, or impacts are high, needs testing... P-High arbitrary important item labels May 20, 2024
@AnthonyTornetta
Copy link
Author

AnthonyTornetta commented May 21, 2024

@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!

@Vrixyz Vrixyz mentioned this pull request May 21, 2024
@Vrixyz Vrixyz added enhancement New feature or request A-Integration very bevy specific labels May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Integration very bevy specific D-Difficult Needs strong technical background, domain knowledge, or impacts are high, needs testing... enhancement New feature or request P-High arbitrary important item
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Multiple pipelines?
3 participants