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

Use a reserved "Default" layer for memberships in InteractionGroups #706

Merged
merged 6 commits into from
Aug 5, 2024

Conversation

Aceeri
Copy link
Contributor

@Aceeri Aceeri commented Aug 2, 2024

Currently the default is to being a member of all groups with a filter/interaction with everything.

I believe this is a bit backwards and makes special interactions between groups harder to understand/deal with without flipping all of your logic to be a "non-membership" or by modifying the interaction groups of every object in your simulation.

This is more in line with what Box2d (one membership, all filter) and Godot (one membership, one filter) do currently, however there are counterexamples like Unity (all membership, all filter) and the current default. Unity is actually one membership/all filter as well since they do not allow multiple groups to be selected, however they allow configuring the default interactions/filters.

There is also an argument of all filter vs one filter as the default, but I do not believe it matters as much as the membership.

Comment on lines 86 to 84
/// The default group.
const DEFAULT = 1 << 0;
/// The group n°1.
const GROUP_1 = 1 << 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I agree on shifting those groups though:

  • it makes a tedious migrations for users
  • javascript bindings will have to adapt (not hard, but tedious/error prone again)

the naming becomes a bit inconsistent, I would prefer DEFAULT and GROUP_1 being the same, or straight up removing DEFAULT, and add that's the default as a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya I agree

@Vrixyz Vrixyz merged commit 510686a into dimforge:master Aug 5, 2024
7 checks passed
@Vrixyz
Copy link
Contributor

Vrixyz commented Aug 5, 2024

Thanks!

@Aceeri Aceeri deleted the default-layers branch August 6, 2024 03:02
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