TheGrimsey / oxidized_navigation

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

Rapier 0.26.0 support! #25

Closed BaronVonScrub closed 2 months ago

BaronVonScrub commented 3 months ago

Added compatibility with Rapier 0.26.0 whilst maintaining xpbd compatibility. Example functionality seems 1:1 as before, and test is passed.

Note that Rapier and XPBD features are now mutually exclusive, if they weren't before, due to differing versions of Parry3D.

TheGrimsey commented 3 months ago

Nice solution to the dependency 'desync'.

Though I wish it could be avoided it was likely inevitable that this would happen at some point.

BaronVonScrub commented 3 months ago

My IDE didn't notify me of compile errors inside of tests and examples, so those are now sorted now. Additionally, there was no Parry3D import for the case where no features were enabled, so I changed the cfg if-elif to just an if-else; it now falls back to the latest Parry3D version as the default.

TheGrimsey commented 3 months ago

My IDE didn't notify me of compile errors inside of tests and examples, so those are now sorted now.

Yeah, running the tests is slightly messy. You have to run them for each feature or else it may consider it okay (hence why there are separate actions for rapier, xpbd, and just parry3d).

Hmmm. Feature handling is somewhat complicated by the Parry3d only version. Possibly a specific parry3d feature used by Rapier as well?

BaronVonScrub commented 3 months ago

Alright, hopefully third time is the charm.

On that second commit, I set up the fallback case for no features but since the dependency was optional, it didn't actually HAVE that library to fall back to. On this third one, the latest parry is ALWAYS listed as a dependency - it's just not used in the XPBD case. That SHOULD fix it, I think? My only concern now is if having the two versions of Parry3D under the hood causes collisions in the external libraries themselves. The test and examples run fine here, so fingers crossed it behaves. If not, I'll have to find some more creative way to actually exclude the redundant dependency in that case.

Sorry for the multiple attempts; still learning the ins and outs of Rustrover IDE.

BaronVonScrub commented 3 months ago

Apparently not. I CAN actually reproduce the errors now, though; sorry, didn't realise the tests needed features enabled, only checked that with the Examples.

The tests now pass with each feature enabled individually, and with none.

Sorry once again. :p

TheGrimsey commented 3 months ago

Hmmm.

I am not a fan of having two parry3d dependencies if XPBD is enabled.

I think it'd be appropriate to:

TheGrimsey commented 3 months ago

Sorry once again. :p

It's all good. Supporting both Rapier & XPBD is complicated 😅

BaronVonScrub commented 3 months ago

I am not a fan of having two parry3d dependencies if XPBD is enabled.

I think it'd be appropriate to:

  • Make parry3d optional and add a parry_standalone feature which enables it.
  • Add parry3d as a dependency for the rapier feature
  • Add the parry_standalone feature to the test party action

Yeah, I agree that's the best approach; I was just tryna avoid having to alter your test workflows if possible.

I'll get that sorted and to you today.

BaronVonScrub commented 3 months ago

Alright @TheGrimsey, I've done that.

I also made the conditional checks ensure there was always ONE parry feature on, and never more than one. This made it wayyy too chunky to be used all over the place, though, so I've walled it off into a macro to be used where needed.

Update the standalone workflow to use the new feature, and I think we're set. :)

TheGrimsey commented 2 months ago

Sorry for not getting back to this earlier.

I've implemented a version of this for the bevy-0.14 branch. This is a good solution.

TheGrimsey commented 2 months ago

In part included with 0.11.