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

Add setCollisionMask and getCollisionMask to Shape's API #2090

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Bruception
Copy link

Implements the suggestion in: #2072

@@ -161,8 +161,10 @@ class Shape : public love::physics::Shape

int setCategory(lua_State *L);
int setMask(lua_State *L);
Copy link
Author

Choose a reason for hiding this comment

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

Unclear to me if we want to maintain this API, or prefer to deprecate.

Comment on lines 450 to 452
test:assertEquals(nil, shape2:getMask(), 'check no mask')
shape2:setMask(1, 2, 3)
test:assertEquals(3, #{shape2:getMask()}, 'check set mask')
Copy link
Author

Choose a reason for hiding this comment

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

@Bruception
Copy link
Author

@slime73, is there anything else I should do to get this across the line?

@slime73
Copy link
Member

slime73 commented Aug 10, 2024

I'm wondering about usability - if someone wants an object to collide with everything or nearly everything, they'd need to specify around 16 parameters with the new API whereas with the old one it'd be much less.

@alex-boulay
Copy link

Previous behavior still works if I'm correct so you can still use it. The real question to me is will we keep the two implementations ? I would prefer to have functions:

  • clear()/fill()/add(Fixtures...)/remove(Fixtures...)-Mask.
    It would allow better handling especially when dynamically removing collisions for example in cases of events.

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