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

Rapier 0.26.0 support! #25

Closed
wants to merge 5 commits into from

Conversation

BaronVonScrub
Copy link

@BaronVonScrub BaronVonScrub commented Jun 2, 2024

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.

…ibility. Example functionality seems 1:1 as before, and test is passed.

Note that Rapier and XPBD features are now mutual exclusive, if they weren't before, due to differing versions of Parry3D.
@TheGrimsey
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

TheGrimsey commented Jun 6, 2024

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
Copy link
Author

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.

Made default case use CFG IF to solve library collision.
@BaronVonScrub
Copy link
Author

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
Copy link
Owner

TheGrimsey commented Jun 8, 2024

Hmmm.

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

@TheGrimsey
Copy link
Owner

TheGrimsey commented Jun 8, 2024

Sorry once again. :p

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

@BaronVonScrub
Copy link
Author

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.

Isolated cfg checking into a macro, so as not to bloat files that make use of the conditional compilation logic.
@BaronVonScrub
Copy link
Author

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
Copy link
Owner

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
Copy link
Owner

In part included with 0.11.

@TheGrimsey TheGrimsey closed this Jul 15, 2024
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

Successfully merging this pull request may close these issues.

2 participants