TheGrimsey / oxidized_navigation

A runtime Nav-Mesh generation plugin for Bevy Engine in Rust.
Apache License 2.0
169 stars 12 forks source link

Make oxidized_navigation generic over Parry3d colliders instead of requiring bevy_rapier3d #14

Closed Elabajaba closed 1 year ago

Elabajaba commented 1 year ago

This adds support for bevy_xpbd_3d, and should allow end users to easily use their own (Parry3d compatible) colliders.

This changes plugin creation to OxidizedNavigationPlugin::<Collider>::new(NavMeshSettings { ... }),, as OxidizedNavigationPlugin now stores a PhantomData<C>, where C: Collider + Component.

In order for users to implement their own Parry3d wrapper components, they just need to implement the Collider trait (should probably should be renamed to OxidizedCollider or something) from /src/colliders/mod.rs.

Todo:

TheGrimsey commented 1 year ago

I like this solution. 👍 Very nice stuff.

should probably should be renamed to OxidizedCollider

I agree with this.

Elabajaba commented 1 year ago

Xpbd (currently) only uses bevy's transform for the initial collider creation, and not for if it moves: https://github.com/Jondolf/bevy_xpbd/pull/96

Xpbd and my parry example also don't automatically scale the collider based on bevy's transform scale. Xpbd will probably add that eventually, but I'm not sure if it's worth adding that complexity to the parry example.

I still haven't gotten around to fixing the docs or commenting the parry example.

I'm not sure if we want to add a builtin parry3d wrapper component?

TheGrimsey commented 1 year ago

Xpbd (currently) only uses bevy's transform for the initial collider creation, and not for if it moves: Jondolf/bevy_xpbd#96

Already merged by the time I read this :P Wonderful.

Xpbd and my parry example also don't automatically scale the collider based on bevy's transform scale. Xpbd will probably add that eventually, but I'm not sure if it's worth adding that complexity to the parry example.

The current expectation (because this is how it's worked with Rapier) is that the collider is scaled. The transform scale is ignored when building the nav-mesh. It's possible this is not ideal.

I'm not sure if we want to add a builtin parry3d wrapper component?

This may be a good idea.

Could then possibly generate from a parry collider without needing a rapier or Xpbd, which would allow fewer dependencies when generating from just meshes. Although even that is possibly too much.

What I would be interested in though is a private placeholder component for when you aren't using any collision & just meshes. Though that may be out of scope for this as it ties more into meshes in #9

Elabajaba commented 1 year ago

I'm not sure if we want to add a builtin parry3d wrapper component?

This may be a good idea.

I figure anyone who's using raw parry3d colliders probably has additional data associated with them for eg. spatial queries and if/when transform propagation takes place for them (eg. FixedUpdate vs PostUpdate vs PreUpdate), and the basic implementation is trivial.

Could then possibly generate from a parry collider without needing a rapier or Xpbd, which would allow fewer dependencies when generating from just meshes. Although even that is possibly too much.

This PR already works like that. If you don't enable the xpbd or rapier features then it won't compile either of them and you just need to pass in your own parry3d wrapper component. (see the parry example).

On a related note, I originally made rapier a default feature, but I think it might be better to have no default features so users don't end up accidentally enabling both rapier and xpbd by adding the xpbd feature and not disabling default features (having them both enabled won't break anything it'll just slow down compiles, though you can't use both rapier and xpbd colliders (or parry3d + rapier/xpbd colliders) without having to make a custom wrapper component that can be either of them).

What I would be interested in though is a private placeholder component for when you aren't using any collision & just meshes. Though that may be out of scope for this as it ties more into meshes in https://github.com/TheGrimsey/oxidized_navigation/issues/9

I think that'll need some much larger changes that's beyond the scope of what I was hoping to do for this.

Elabajaba commented 1 year ago

I think this is ready now?

TheGrimsey commented 1 year ago

Looks great. Thank you for another fantastic PR! 👍