Added suggestions to #373

[ahcorde]

Signed-off-by: ahcorde [email protected]

🎉 New feature

Summary

Added more features to bullet-featherstone

Checklist

• Signed all commits for DCO
• Added tests
• Added example and/or tutorial
• Updated documentation (as needed)
• Updated migration guide (as needed)
• Consider updating Python bindings (if the library has them)
• codecheck passed (See contributing)
• All tests passed (See test coverage)
• While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

[codecov]

Codecov Report

Merging #374 (2f345bb) into add-bullet-featherstone (8fca02f) will increase coverage by 5.77%.
The diff coverage is 63.97%.

❗ Current head 2f345bb differs from pull request most recent head 1dcfe3a. Consider uploading reports for the commit 1dcfe3a to get more accurate results

@@                     Coverage Diff                     @@
##           add-bullet-featherstone     #374      +/-   ##
===========================================================
+ Coverage                    68.91%   74.68%   +5.77%     
===========================================================
  Files                          137      138       +1     
  Lines                         6305     6448     +143     
===========================================================
+ Hits                          4345     4816     +471     
+ Misses                        1960     1632     -328     

Impacted Files	Coverage Δ
bullet-featherstone/src/Base.hh	100.00% <ø> (+88.46%)	⬆️
bullet-featherstone/src/plugin.cc	100.00% <ø> (ø)
...ullet-featherstone/src/EntityManagementFeatures.cc	14.55% <27.86%> (+12.49%)	⬆️
bullet-featherstone/src/SimulationFeatures.cc	68.29% <68.42%> (+68.29%)	⬆️
bullet-featherstone/src/WorldFeatures.cc	92.30% <92.30%> (ø)
bullet/src/SimulationFeatures.cc	94.87% <97.22%> (+94.87%)	⬆️
bullet-featherstone/src/SDFFeatures.cc	68.14% <100.00%> (+68.14%)	⬆️
bullet/src/Base.hh	55.88% <0.00%> (+7.35%)	⬆️
bullet/src/SDFFeatures.cc	74.67% <0.00%> (+29.61%)	⬆️
... and 4 more

---

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 8fca02f...1dcfe3a. Read the comment docs.

[mxgrey]

I would discourage adding many of these features to the bullet-featherstone plugin. Things like constructing empty models and empty links is not helpful or meaningful for the Featherstone API. In the Featherstone a model needs to have a base link, and a link needs to belong to a model.

I assume that these features were probably added so that the bullet-featherstone plugin can be tested against more of the "common" physics tests, but I think the common testing framework should just have each test skip over any plugins which don't provide the features that the test requires.

[ahcorde]

I would discourage adding many of these features to the bullet-featherstone plugin. Things like constructing empty models and empty links is not helpful or meaningful for the Featherstone API. In the Featherstone a model needs to have a base link, and a link needs to belong to a model.

I assume that these features were probably added so that the bullet-featherstone plugin can be tested against more of the "common" physics tests, but I think the common testing framework should just have each test skip over any plugins which don't provide the features that the test requires.

Do you know which features make sense to include to the bullet-featherstone plugin ?

[mxgrey]

Do you know which features make sense to include to the bullet-featherstone plugin ?

We could consider adding frame semantics for shapes and joints if that's getting used anywhere in gz-sim.

We might also be able to support adding and removing collision shapes, but it's not totally clear to me if that will work nicely with Bullet's Featherstone API. It's also probably not a very useful feature when we don't support attaching/detaching links.

We should be able to add a feature for creating fixed joints between the links of models. That might also allow bullet-featherstone to support closed kinematic chains, similar to what's been done recently with the dartsim plugin.

[mxgrey]

I've gone through and removed a couple features that I don't think are appropriate for bullet-featherstone and also fixed some implementation issues that I noticed. I hope you don't mind that I committed directly to the branch for expediency.

I think it's fine to merge this into the upstream add-bullet-featherstone branch if no one else has any concerns about it.