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

Remove Pos and Rot in favor of bevy's Transform #16

Closed
Jondolf opened this issue Mar 16, 2023 · 6 comments
Closed

Remove Pos and Rot in favor of bevy's Transform #16

Jondolf opened this issue Mar 16, 2023 · 6 comments

Comments

@Jondolf
Copy link
Owner

Jondolf commented Mar 16, 2023

Currently, bevy_xpbd uses the components Pos and Rot for describing the position and orientation of bodies. At the end of each iteration of the main physics loop, these are synchronized with bevy's Transforms.

I'd like to discuss if we should remove the separate components and move to just using bevy's own Transforms.

Advantages

  • More familiar and convenient for bevy users
  • More complete APIs
  • Reduces code and maintenance overhead (for example, no custom Rot implementations for 2D and 3D, all rotations use Quats)
  • Less components and nicer queries (Transform and PrevTransform instead of Pos, PrevPos, Rot, and PrevRot)
  • No need for synchronization step
  • If/when bevy gets an editor eventually, it would make more sense to manipulate Transforms directly
  • It's what most physics engines seem to do (like bevy_rapier)

Disadvantages

  • Transform hierarchies, scaling and shearing can cause serious problems (see comments below)
  • Parallellism might be harder, since two systems can't access the same Transform at once. Separate systems with Pos and Rot can run simultaneously.
  • No f64 version of Transform unless we make a custom one (adding f64 as a crate feature: Add bevy_xpbd_2d_f64 and bevy_xpbd_3d_f64 crates #7)
  • In 2D, Pos uses Vec2 and Rot uses a scalar value. Transforms just have a Vec3 and a Quat, which can be considered less ergonomic for 2D. However, bevy users are used to working with them, and they might provide more freedom, so this may not be an issue.
  • Migration pain (not a problem for us now, but could be in the future if this crate becomes more popular)
  • Something else?

Conclusion

Personally, at least from an API standpoint, I think it makes sense to move to using just Transforms, as it would generally be much more convenient and familiar for bevy users, and it would also probably reduce the engine's code and complexity. Using Transforms for physics can cause many problems as well though, which is why I'd like to hear what others think.

@Aceeri
Copy link
Contributor

Aceeri commented Mar 16, 2023

More familiar and convenient for bevy users

I agree here, it would be nicer/simpler for quick usecases, but I think it ends up falling apart more because things that directly change Transform probably aren't compatible with the physics engine aside from scenes. I think something that might make sense is to have the Pos/Rot take the Transform when it is initially created, which would give us the benefits of being able to set Transforms for initialization.

Reduces code and maintenance (for example, no custom Rot implementations for 2D and 3D, all rotations use Quats)

We could just move Rot to being a Quat similar to Transform, it'd effectively be the same thing.

Less components and nicer queries (Transform and PrevTransform instead of Pos, PrevPos, Rot, and PrevRot)

Tbh I don't expect users to ever really use PrevPos/Rot so I'm not sure we need to worry much about those. One benefit I can see with splitting Pos/Rot is if we want to add more traditional particle physics in the original PBD sense for things like cloth/ropes

It's what most physics engines seem to do (like bevy_rapier)

Transform also gives some false senses of usability/security with some things, e.g. should we allow shearing of GlobalTransform to also affect physics? (imo probably not, this has caused issues in bevy rapier here: dimforge/bevy_rapier#244)

However, I do think adding a Scale component or similar in the future might be interesting (especially if we can make it interact nicely in the physics world, potentially by getting the velocity the contact point including the change in scale).

No f64 version of Transform unless we make a custom one (adding f64 as a crate feature: #7)

This is also partially why I'm more partial to our own positioning, at least with the mentioned part of the paper in this PR: (#7).

One big thing is there is no way to opt out of the transform hierarchy and having a parent-child relationship with physics entities doesn't make a whole lot of sense as they would be better explained with a fixed joint constraint (which we could do automatically to be fair, but I think that adds some implicitness and I'd rather error/warn here if we were to go this route).

Overall I think I'm more on the side of keep them separated for the sake of being more explicit when you are interacting with physics entities.

@Aceeri
Copy link
Contributor

Aceeri commented Mar 16, 2023

dimforge/bevy_rapier#219 (comment)
Some discussion on this from @HackerFoo

@HackerFoo
Copy link

Physics uses isometries (no scale or shear), and these must be global (from the physics engine's point of view), so it's not a good match for Transform. Implementing From / Into makes it easy to convert between transform types.

@HackerFoo
Copy link

HackerFoo commented Mar 16, 2023

Putting the rotation and translation together in a struct can make the math a lot cleaner, though. I use two types of isometries: one with a Quat rotation for positions and one with a Vec3 torque (cross product) for forces, which accumulate properly.

Maybe the second one should be called something else.

@johanhelsing
Copy link
Contributor

johanhelsing commented Mar 17, 2023

No f64 version of Transform unless we make a custom one

in my opinion, this alone is reason enough not to use Transforms.

From my previous attempts with Transforms my original bevy_xpbd and 2D, it was pretty painful / non-ergonomic to use from the outside as well.

I think something that might make sense is to have the Pos/Rot take the Transform when it is initially created, which would give us the benefits of being able to set Transforms for initialization.

Implementing From / Into makes it easy to convert between transform types.

Yeah, I think this could work quite nicely with the builder methods on RigidBodyBundle:

I also think it's nice to be able to run a simulation that doesn't sync back to the bevy transforms. Could easily integrate with crates like seldom_pixel for instance, or physics simulations that don't really need to render every frame (or at all)

On the topic of granularity of components: In the version of bevy_xpbd I use in cargo space (derived from the same source as this repo), I use the presence of components to communicate what a physics entity can and cannot do. e.g.:

  • if it doesn't have mass or any kind of velocity (angular or linear), it's a static
  • if it doesn't have angular velocity, its rotation is locked (ideal for particles)
  • if it doesn't have a collider, it won't be included in the penetration constraints (also done here)
  • means a lot of queries don't have overlap, so more systems can run in parallel
  • and easy to iterate over dynamics vs statics based on query filters
  • can (optionally) optimize for axis-aligned non-rotating physics

@Jondolf
Copy link
Owner Author

Jondolf commented Mar 17, 2023

I agree with everyone's points here, I think we should stick with Pos and Rot as separate components. This is more explicit and provides flexible and non-overlapping queries, while avoiding the Transform problems related to hierarchy, shear, scale, f64 etc. The primary gains of moving to Transform would probably just be familiarity and ergonomics, and even that's debatable.

I think this was still a worthwile discussion to have, at least for future reference. I'll close this now :)

Edit: The components are now called Position and Rotation. After #96, Transform can also be directly used for positioning and moving bodies, so in a way we get the best of both worlds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants