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

Added Shapes Features to bullet-featherstone #382

Merged

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Jul 12, 2022

Signed-off-by: ahcorde ahcorde@gmail.com

🎉 New feature

Summary

Added Shapes Features to bullet-featherstone.

TODOs

  • Added test
  • fix boundingbox

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.

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde self-assigned this Jul 12, 2022
Signed-off-by: ahcorde <ahcorde@gmail.com>
@codecov
Copy link

codecov bot commented Jul 13, 2022

Codecov Report

Merging #382 (5bca0c1) into add-bullet-featherstone (9e7af39) will increase coverage by 1.36%.
The diff coverage is 62.80%.

❗ Current head 5bca0c1 differs from pull request most recent head ba3f439. Consider uploading reports for the commit ba3f439 to get more accurate results

@@                     Coverage Diff                     @@
##           add-bullet-featherstone     #382      +/-   ##
===========================================================
+ Coverage                    74.55%   75.91%   +1.36%     
===========================================================
  Files                          128      139      +11     
  Lines                         5628     6578     +950     
===========================================================
+ Hits                          4196     4994     +798     
- Misses                        1432     1584     +152     
Impacted Files Coverage Δ
bullet-featherstone/src/FreeGroupFeatures.cc 0.00% <0.00%> (ø)
bullet-featherstone/src/JointFeatures.cc 0.00% <0.00%> (ø)
dartsim/src/SDFFeatures.cc 62.71% <ø> (ø)
dartsim/src/SimulationFeatures.cc 90.16% <ø> (ø)
dartsim/src/SimulationFeatures.hh 100.00% <ø> (ø)
include/gz/physics/ForwardStep.hh 100.00% <ø> (ø)
include/gz/physics/FrameID.hh 100.00% <ø> (ø)
include/gz/physics/FrameSemantics.hh 100.00% <ø> (ø)
include/gz/physics/Geometry.hh 100.00% <ø> (ø)
include/gz/physics/detail/FrameSemantics.hh 100.00% <ø> (ø)
... and 23 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

ahcorde added 2 commits July 14, 2022 13:53
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde added the Bullet Bullet engine label Jul 15, 2022
…corde/add-bullet-featherstone/shapesFeatures
{
for (unsigned int i = 0; i < value->collisionEntityIds.size(); ++i)
{
if (value->collisionEntityIds[i] == _id.ID())
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you seeing a case where the incoming _id is for a collision entity? The kinematics feature list doesn't include ShapeFrameSemantics so it's not supposed to be possible for a user to ask for the frame data of a collision object.

If you're seeing that happen then it probably indicates an error in the template system. If you can point me at a way to recreate it, that would be very helpful.

@@ -232,7 +232,7 @@ std::optional<Structure> ValidateModel(const ::sdf::Model &_sdfModel)
for (std::size_t i = 0; i < model.LinkCount(); ++i)
{
const auto *link = model.LinkByIndex(i);
if (parentOf.count(link) == 0)
if (parentOf.size() != 0 && parentOf.count(link) == 0)
Copy link
Contributor

@mxgrey mxgrey Jul 18, 2022

Choose a reason for hiding this comment

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

I'm not sure why we would check the size here. parentOf.size() == 0 implies that there is only one link and that link has no parent. In that case parentOf.count(link) == 0 will be true. Inside of this branch we then double-check that if a rootLink was selected while iterating over the model then that root link is the same as the one we're currently looking at. I think it's reasonable to double-check that because if that's not true then there's an error in the logic of the parser or the model has multiple root links which is not currently supported by Bullet's Featherstone algorithm and will produce problems later on.

Was there a specific situation that motivated these additional conditions? If so it would be helpful if I can reproduce it to figure out what might be wrong with the parser's logic.

@mxgrey mxgrey mentioned this pull request Jul 18, 2022
9 tasks
@chapulina chapulina added the 🌱 garden Ignition Garden label Jul 18, 2022
ahcorde and others added 2 commits July 20, 2022 16:06
…corde/add-bullet-featherstone/shapesFeatures
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
@ahcorde ahcorde marked this pull request as ready for review July 21, 2022 11:56
@ahcorde ahcorde requested review from azeey and scpeters as code owners July 21, 2022 11:56
@chapulina chapulina added the enhancement New feature or request label Jul 29, 2022
@chapulina chapulina removed the 🌱 garden Ignition Garden label Aug 6, 2022
ahcorde and others added 6 commits August 22, 2022 12:09
…corde/add-bullet-featherstone/shapesFeatures
Signed-off-by: ahcorde <ahcorde@gmail.com>
…corde/add-bullet-featherstone/shapesFeatures
…corde/add-bullet-featherstone/shapesFeatures
* Improved joints in bullet featherstone

Signed-off-by: ahcorde <ahcorde@gmail.com>
…rstone/shapesFeatures

Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll mjcarroll merged commit 16abf65 into add-bullet-featherstone Sep 16, 2022
@mjcarroll mjcarroll deleted the ahcorde/add-bullet-featherstone/shapesFeatures branch September 16, 2022 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bullet Bullet engine enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants