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 attributes m_collisionFilterGroup, m_collisionFilterMask to ContactResultCallback in ammo.idl #352

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

Conversation

regnaio
Copy link

@regnaio regnaio commented Jan 26, 2021

Related to #350

Please refer to the bullet source code for ContactResultCallback: https://github.com/kripken/ammo.js/blob/master/bullet/src/BulletCollision/CollisionDispatch/btCollisionWorld.h#L411

Note how the ContactResultCallback constructor automatically sets m_collisionFilterGroup to btBroadphaseProxy::DefaultFilter (value 1) and m_collisionFilterMask to btBroadphaseProxy::AllFilter (value -1)

ContactResultCallback uses these 2 attributes in needsCollision() to determine whether addSingleResult(...) should be triggered

Thus, without the 2 added attributes in this PR, we cannot control when addSingleResult is triggered by contactTest , especially when using collision filter groups and masks on our btRigidBodys

Could we please add these 2 attributes to ammo.idl? Thank you for your time and help!

(Note that the build files were generated using the command cmake -B builds -DCLOSURE=1 -DTOTAL_MEMORY=268435456 with the environment based on the Dockerfile in #351 (comment). Please disregard these build files if another configuration is preferred)

@ianpurvis
Copy link
Collaborator

Looks like CI is breaking because of deprecated code in https://github.com/mymindstorm/setup-emsdk. I'll work on getting this fixed later today... thanks!

@ianpurvis
Copy link
Collaborator

@regnaio Sorry for the delay... CI is now fixed in #353. Can you rebase or merge ammo.js master into your PR branch? Thanks!

@regnaio
Copy link
Author

regnaio commented Feb 3, 2021

Thank you, @ianpurvis! Just merged ammo.js master into this PR branch

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