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

bullet-featherstone: fix how links are flattened in ConstructSdfModel #562

Merged

Conversation

shameekganguly
Copy link
Contributor

Fixes a segfault in ConstructSdfModel when loading a model with links defined out of order. Here's an example model that triggers the segfault:

  <model name="test_model">
      <link name="child_linkC" />
      <link name="parent_link" />
      <link name="child_linkA" />
      <link name="child_linkB" />
      <joint name="world_joint" type="fixed">
        <parent>world</parent>
        <child>parent_link</child>
      </joint>
      <joint name="joint1" type="prismatic">
        <parent>parent_link</parent>
        <child>child_linkA</child>
        <axis>
            <xyz>0 0 1</xyz>
        </axis>
      </joint>
      <joint name="joint2" type="prismatic">
        <parent>child_linkA</parent>
        <child>child_linkB</child>
        <axis>
            <xyz>0 0 1</xyz>
        </axis>
      </joint>
      <joint name="joint3" type="prismatic">
        <parent>child_linkB</parent>
        <child>child_linkC</child>
        <axis>
            <xyz>0 0 1</xyz>
        </axis>
      </joint>
    </model>

The current logic for sorting links to ensure that a parent link is placed in the flatLinks vector before its children links is inadequate for such cases. So the fix is to build the link tree and do a preorder traversal of it to create the flatLinks vector.

@github-actions github-actions bot added the 🌱 garden Ignition Garden label Oct 17, 2023
@codecov
Copy link

codecov bot commented Oct 17, 2023

Codecov Report

Merging #562 (1419fe2) into gz-physics6 (3b7a7c8) will increase coverage by 1.07%.
The diff coverage is 76.19%.

❗ Current head 1419fe2 differs from pull request most recent head ef0ef82. Consider uploading reports for the commit ef0ef82 to get more accurate results

@@               Coverage Diff               @@
##           gz-physics6     #562      +/-   ##
===============================================
+ Coverage        75.95%   77.02%   +1.07%     
===============================================
  Files              143      143              
  Lines             7327     7321       -6     
===============================================
+ Hits              5565     5639      +74     
+ Misses            1762     1682      -80     
Files Coverage Δ
bullet-featherstone/src/SDFFeatures.cc 74.15% <76.19%> (+0.09%) ⬆️

... and 2 files with indirect coverage changes

Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

we should add a test that uses the snippet you provided. I'll look into the easiest way to do that

@scpeters
Copy link
Member

scpeters commented Nov 2, 2023

we should add a test that uses the snippet you provided. I'll look into the easiest way to do that

I added a test in 49de232 without this branch, and it fails CI. I will push it to this branch

@scpeters
Copy link
Member

scpeters commented Nov 2, 2023

@azeey I added a small test for this in case you want to review

Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

LGTM, though I think the test I added deserves a second look from someone else if possible

Copy link
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

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

LGTM! Just a few minor comments.

test/common_test/worlds/world_unsorted_links.world Outdated Show resolved Hide resolved
test/common_test/worlds/world_unsorted_links.world Outdated Show resolved Hide resolved
test/common_test/world_features.cc Outdated Show resolved Hide resolved
test/common_test/world_features.cc Outdated Show resolved Hide resolved
test/common_test/world_features.cc Outdated Show resolved Hide resolved
test/common_test/world_features.cc Outdated Show resolved Hide resolved
bullet-featherstone/src/SDFFeatures.cc Outdated Show resolved Hide resolved
@scpeters scpeters changed the title Fix how links are flattened in bullet-featherstone ConstructSdfModel bullet-featherstone: fix how links are flattened in ConstructSdfModel Nov 8, 2023
@scpeters scpeters merged commit 1260ef2 into gazebosim:gz-physics6 Nov 8, 2023
11 checks passed
@shameekganguly
Copy link
Contributor Author

Thanks Steve, Ian, Addisu for taking care of this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants