Fix: Ammo contact collision callbacks don't trigger when using physics impostors with collision filter groups and masks

[regnaio]

Playground: https://playground.babylonjs.com/#GE0ASK#12 (with console.log on collision)

Please note that when using AmmoJSPlugin, no console.logs are called. However, console.logs are seen when using CannonJSPlugin and OimoPlugin.

Also note that AmmoJSPlugin seems to work fine when

enum CollisionFilterGroup {
    Ground = 1,
    Rock = 2
}

instead of using 4 and 8

This is due to Ammo/Bullet automatically assigning the collision filter group of collision objects as 1 or 2 if none is specified:

• Assign: https://github.com/kripken/ammo.js/blob/master/bullet/src/BulletDynamics/Dynamics/btDiscreteDynamicsWorld.cpp#L577
• Number codes: https://github.com/kripken/ammo.js/blob/master/bullet/src/BulletCollision/BroadphaseCollision/btBroadphaseProxy.h#L93

So using 1 and 2 in the Playground just happens to work by chance (and for incorrect reasons)

The key to fixing this lies in Bullet/Ammo's ContactResultCallback. Please note how the collision filter group and mask of the contact callback is automatically assigned to 1 and -1 in its constructor, respectively:

• https://github.com/kripken/ammo.js/blob/master/bullet/src/BulletCollision/CollisionDispatch/btCollisionWorld.h#L416

The automatic assignment of the contact callback's collision filter group to 1 means that if the physics impostors have any other collision filter group besides 1, collision callbacks won't be triggered

And there is currently no way to change the contact callback's group and mask values in Ammo.js master build. This is resolved in the PR I opened (kripken/ammo.js#352), which exposes these two attributes in the ammo.idl file. Now we are able to call set_m_collisionFilterGroup() and set_m_collisionFilterMask() in this PR

The logic behind this fix is:

• Once ConcreteContactResultCallback is created, set both its collision filter group and mask to -1 (which means that all bits are set to 1 and all collisions will be registered)
• If a collision check is triggered using contactTest() or contactPairTest() and the physics impostor in question has a collision filter group and mask, use those
• At the end of the collision callback, reset both its collision filter group and mask to -1

Please find a fixed version of the Playground live at: https://regnaio.github.io/playground/ (with rebuilt Ammo and Babylon)
Source code (same as Playground https://playground.babylonjs.com/#GE0ASK#12): https://github.com/regnaio/playground

If this PR is eventually accepted, I think @MackeyK24 would need to first rebuild Ammo with the two exposed attributes, since he has made additional custom changes to Ammo

I've also created a Dockerfile we could use to easily build Ammo: kripken/ammo.js#351 (comment)

I hope @CedricGuillemet , @RaananW , @MackeyK24 could please help advise if this fix needs improvements

[regnaio]

My apologies for the file changes showing a lot of minor refactors.. I think I accidentally triggered an auto-formatter. Only the first 5 change blocks are meaningful

[CedricGuillemet]

@regnaio
So, this fix exists because group and mask are not set by ammojs in the contact callback even when they are properly set in the rigid body?

[regnaio]

@CedricGuillemet Exactly!

[CedricGuillemet]

@regnaio
sounds good then! Please fix the unit tests and whatsnew and I'll approve it.

[regnaio]

I copy and pasted this snippet from @CedricGuillemet, so this now appears 3 times in this file. I'm unsure where we should refactor this if we didn't want to repeat this snippet

For a future similar fix for ClosestRayResultCallback, we may have to repeat this snippet a 4th time if not refactored

[regnaio]

It seems that the corresponding Ammo PR (kripken/ammo.js#352) may be accepted soon

[regnaio]

@CedricGuillemet, the unit tests seem to refer to the Ammo build in the /dist folder instead of the /dist/preview release folder. If I replace the /dist Ammo build with the newly built Ammo from kripken/ammo.js#352, all unit tests pass

If the /dist Ammo is unchanged, set_m_collisionFilterGroup() and set_m_collisionFilterMask() will break the unit tests, since these are undefined

How would you recommend we proceed? Thank you for your help!

new ammo build.zip

[MackeyK24]

Wait... So what happens to all the btSmooth stuff i had in the ammo builds ???

[regnaio]

@MackeyK24 , I was hoping that you could provide the new Ammo build with the ammo.idl changes in kripken/ammo.js#352, since I don't want to undo your btSmooth changes :)

I also hope we could see your additions to Ammo, though I'm unsure where, since Babylon doesn't currently have its own fork of Ammo

[MackeyK24]

I would luv to... I really think we should make a repo with the current changes we have for our babylon environment. That way we can all use the same source and everybody has access to the btSmoothTriangleMesh and btSmoothVehicleRaycaster classes i added to give it the Need For Speed style driving mechanics. Not to mention far better support for Smooth MeshCollider Contacts and Raycasts

[MackeyK24]

We should make a fresh fork from kripken then i can add the btSmooth and raycast vehicle changes to that.

So whoever is the guys who creates the babylon js forks... :)

[MackeyK24]

Yo @CedricGuillemet ... Let me know if i should re build the ammo with the two attributes added (Are Unit Test Ok?)

[regnaio]

@MackeyK24 , the unit tests are only okay if the new Ammo build is placed in the dist/ folder

[CedricGuillemet]

I don't understand the issue with ammojs. Do the unit tests use ammo.js from the dist folder and not the dist preview one? @sebavan

[sebavan]

It looks like the unit tests are loading AMMO from the wrong place, you could change it in the karma.conf.js file of the unit tests folder. Instead of dist/ammo.js replace with the preview one.

In this case, you might also need to await ammo(); in all the existing tests to ensure it is compatible with the new version.

[regnaio]

@MackeyK24 , would you prefer creating another PR for your updated Ammo build in dist/preview release with the extra 2 attributes: m_collisionFilterGroup, m_collisionFilterMask?

[MackeyK24]

I already did. But I a m waiting for me current PR for the playground update

[MackeyK24]

Waiting for PR #9871

[deltakosh]

Can you fix the conflicting file?

[MackeyK24]

@regnaio the ammo js is now exposing your properties. You must fix the conflict before your PR will go thru

[regnaio]

Thanks, @deltakosh, @MackeyK24, and @CedricGuillemet! Just fixed the merge conflict

[deltakosh]

We still have build issues :(

Any update?

[MackeyK24]

What build issues?
The latest version seems to be working fine

[MackeyK24]

@regnaio there must be some issues you still need to work out. The build of ammo.js is fine and has the additional properties exposed via ammo.idl

[deltakosh]

Last call before I close this one for inactivity