Use collision bitmask flag for meshes

[luca-della-vedova]

Preliminary support for collision bitmask was introduce in ign-physics with this PR.

That introduces filling the bitmask and introducing the filtering in the ConstructSdfCollision function. The issue is that the function is called in an else statement only if the object is not a mesh.
This means that before this PR meshes didn't implement the collide_bitmask flag, now they do. This effectively makes the change in the ConstructSdfCollision obsolete and it can be removed.

While this is just a few lines and it fixes the problem, it introduces an additional feature to the MinimumFeatureList which is probably not desirable. Also, I guess there should be a discussion about whether more features from the ConstructSdfCollision functions should be ported to make them work with meshes as well, from what I can see in the code there is a whole chunk of SDF parameters related to friction that will be applied to primitives but not to meshes, which doesn't sound correct.

Open to ideas on whether this fix is acceptable (and whether we should find a way to port the above) or if I should look at alternative fixes.

[codecov]

Codecov Report

Merging #160 into master will increase coverage by 0.09%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##           master     #160      +/-   ##
==========================================
+ Coverage   65.68%   65.77%   +0.09%     
==========================================
  Files         128      128              
  Lines        6297     6305       +8     
==========================================
+ Hits         4136     4147      +11     
+ Misses       2161     2158       -3     

Impacted Files	Coverage Δ
src/systems/physics/Physics.cc	33.84% <55.55%> (+0.27%)	⬆️
src/systems/physics/Physics.hh	100.00% <100.00%> (+9.09%)	⬆️
src/SimulationRunner.cc	87.54% <0.00%> (+1.18%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e6f026...9f5ef0f. Read the comment docs.

[iche033]

looks good to me, just one minor codecheck error. Can you update changelog?

[iche033]

the homebrew build failure looks be unrelated. I think we may need to release a new ign-gui bottle for it to pass

[chapulina]

Thanks for removing it from the minimum feature list, but I think this still breaks backwards compatibility.

TPE for example will lose collision support because it doesn't have the bitmask feature. Do you mind creating a new feature list just for this? I know it's more work, but this feature feels niche enough to be an option in itself.

[luca-della-vedova]

Thanks for removing it from the minimum feature list, but I think this still breaks backwards compatibility.

TPE for example will lose collision support because it doesn't have the bitmask feature. Do you mind creating a new feature list just for this? I know it's more work, but this feature feels niche enough to be an option in itself.

Done here, I used a different strategy compared to the parts above for printing the error (not printing any). The reason is that even if the user doesn't specify a collide_bitmask in SDF there will still be a value assigned to it (default of 0xFF), so the warning for the missing feature would be always printed.

There will still be warnings printed in the entityCast function though if the bitmask filter is not implemented, but I'm not sure how to handle them.
We would need a way to know if the user defined a bitmask, I can think of three ways:

• Returning a std::optional in the sdf CollideBitmask function, possibly the proper way, although would break API.
• Comparing with the default of 0xFF (would probably work but a bit hacky).
• Adding a "CollideBitmaskDefined" function to sdformat that returns a boolean whether a collide_bitmask tag was found or not while parsing the sdf file.

Ideas?

[chapulina]

There will still be warnings printed in the entityCast function though if the bitmask filter is not implemented, but I'm not sure how to handle them.

That's a good point. We could just make that function silent and let the caller responsible for printing messages.

We would need a way to know if the user defined a bitmask

I'd be ok punting on this for now, maybe ticketing an issue.

[luca-della-vedova]

That's a good point. We could just make that function silent and let the caller responsible for printing messages.

Something like this?

[chapulina]

Something like this?

Yup, thanks, that looks good to me!

[chapulina]

Thanks for iterating! I bumped the required physics version in ff69acb.

[iche033]

the latest changes look good to me, thanks!