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

fix compilation for feature enhanced-determinism #739

Merged
merged 3 commits into from
Sep 23, 2024

Conversation

Vrixyz
Copy link
Contributor

@Vrixyz Vrixyz commented Sep 22, 2024

@Ughuuu
Copy link
Contributor

Ughuuu commented Sep 22, 2024

I solved it similar to this impl too. It's annoying but it's good enough. Adding it to CI will help in future too. Best way to solve would be to make sure the API's match in parry, but it's not a huge problem (would be a huge problem if we had 100's of places of incompatibility).

@Vrixyz
Copy link
Contributor Author

Vrixyz commented Sep 23, 2024

Best way would be to have indexmap's API match HashMap right ?

I'll probably merge the fix without CI and think about CI next.

But yeah in the meantime I was considering:

  • add a trait for implementing those similar functions
  • 'cfg if' in every lines
  • have another CI step without blocking warnings 😅

@Ughuuu
Copy link
Contributor

Ughuuu commented Sep 23, 2024

Maybe best way would be that, but for now best would be to quickly solve this indeed so we don't break anyone who uses master.
I would also say the API should match standard API, but it's harder to do since we use another package that implements that, and we don't have much control over the API contracts.

Comment on lines 234 to 237
#[cfg(feature = "enhanced-determinism")]
let proxy_id = self.colliders_proxy_ids.shift_remove(removed);
#[cfg(not(feature = "enhanced-determinism"))]
let proxy_id = self.colliders_proxy_ids.remove(removed);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are modified because of a deprecated warning:

warning: use of deprecated method `indexmap::map::IndexMap::<K, V, S>::remove`: `remove` disrupts the map order -- use `swap_remove` or `shift_remove` for explicit behavior.
   --> crates/rapier3d/../../src/geometry/broad_phase_multi_sap/broad_phase_multi_sap.rs:234:62
    |
234 |             if let Some(proxy_id) = self.colliders_proxy_ids.remove(removed) {
    |                                                              ^^^^^^
    |
    = note: `#[warn(deprecated)]` on by default

Copy link
Member

@sebcrozet sebcrozet left a comment

Choose a reason for hiding this comment

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

👍

@Vrixyz Vrixyz merged commit 9e1113c into dimforge:master Sep 23, 2024
8 checks passed
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.

3 participants