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

[parsing] Support parsing continuous joint in SDFormat #17956

Merged
merged 1 commit into from
Sep 22, 2022

Conversation

xuchenhan-tri
Copy link
Contributor

@xuchenhan-tri xuchenhan-tri commented Sep 20, 2022

Closes #14747.

This change is Reviewable

@xuchenhan-tri xuchenhan-tri changed the title [parsing] Support continuous joint [parsing] Support parsing continuous joint in SDFormat Sep 20, 2022
@xuchenhan-tri
Copy link
Contributor Author

+(status: do not review) while I work on unit tests.

@xuchenhan-tri xuchenhan-tri force-pushed the continuous-joint branch 2 times, most recently from e30b19b to f709d16 Compare September 21, 2022 20:39
@xuchenhan-tri xuchenhan-tri added release notes: feature This pull request contains a new feature and removed status: do not review labels Sep 21, 2022
Copy link
Contributor Author

@xuchenhan-tri xuchenhan-tri left a comment

Choose a reason for hiding this comment

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

-(status: do not review) +@amcastro-tri for feature review please, thanks.
+(release notes: feature)

Reviewable status: LGTM missing from assignee amcastro-tri, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @amcastro-tri)

Copy link
Contributor

@amcastro-tri amcastro-tri left a comment

Choose a reason for hiding this comment

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

Excellent!
:lgtm:

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @xuchenhan-tri)


multibody/parsing/test/detail_sdf_parser_test.cc line 838 at r1 (raw file):

      prismatic_joint.acceleration_upper_limits(), Vector1d(10)));

  // Continuous joint

ditto, wouldn't we also like to have a test for "Limitless revolute joint"?

Code quote:

Limitless revolute joint

multibody/parsing/test/sdf_parser_test/joint_parsing_test.sdf line 98 at r1 (raw file):

        </inertial>
      </link>
      <joint name="continuous_joint" type="continuous">

wouldn't we like to have both tests? one for a revolute joint with no limits and one for a continuous joint? even if they are both an equivalent representation of the same mechanism.

Copy link
Contributor

@amcastro-tri amcastro-tri left a comment

Choose a reason for hiding this comment

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

+@EricCousineau-TRI for platform please.

Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform) (waiting on @EricCousineau-TRI and @xuchenhan-tri)

@xuchenhan-tri
Copy link
Contributor Author

multibody/parsing/test/sdf_parser_test/joint_parsing_test.sdf line 98 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

wouldn't we like to have both tests? one for a revolute joint with no limits and one for a continuous joint? even if they are both an equivalent representation of the same mechanism.

I had the same question when modifying this test -- Do people actually specify revolute joints without limit or is it a stop gap for the lack of support for continuous joint? If the answer is the former, I'll add back revolute joint without limits.

Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

:lgtm_strong:, excellent indeed, thanks!

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: 2 unresolved discussions (waiting on @amcastro-tri and @xuchenhan-tri)


multibody/parsing/detail_sdf_parser.cc line 558 at r1 (raw file):

  // Drake marks joints with no limits with ±numeric_limits<double>::infinity()
  // and therefore we make the change here.
  // Default position limits to infinities which is the case for continuous

FWIW (pre-existing defect) Seems like line continuations in this files are a bit inconsistent, oops!


multibody/parsing/test/sdf_parser_test/joint_parsing_test.sdf line 98 at r1 (raw file):

Previously, xuchenhan-tri wrote…

I had the same question when modifying this test -- Do people actually specify revolute joints without limit or is it a stop gap for the lack of support for continuous joint? If the answer is the former, I'll add back revolute joint without limits.

Given that this current revision might be reducing test coverage, yeah, I think it's good to have revolute_joint_no_limits as well (making this PR a pure addition).

Copy link
Contributor Author

@xuchenhan-tri xuchenhan-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 unresolved discussions (waiting on @amcastro-tri and @EricCousineau-TRI)


multibody/parsing/detail_sdf_parser.cc line 558 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

FWIW (pre-existing defect) Seems like line continuations in this files are a bit inconsistent, oops!

I don't see the inconsistency. I intend the new comment to be a new paragraph. Is this better?


multibody/parsing/test/detail_sdf_parser_test.cc line 838 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

ditto, wouldn't we also like to have a test for "Limitless revolute joint"?

Done, thanks.


multibody/parsing/test/sdf_parser_test/joint_parsing_test.sdf line 98 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Given that this current revision might be reducing test coverage, yeah, I think it's good to have revolute_joint_no_limits as well (making this PR a pure addition).

Gotcha, done, thanks!

Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: 2 unresolved discussions (waiting on @amcastro-tri)

Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 unresolved discussions (waiting on @amcastro-tri)


multibody/parsing/detail_sdf_parser.cc line 558 at r1 (raw file):

Previously, xuchenhan-tri wrote…

I don't see the inconsistency. I intend the new comment to be a new paragraph. Is this better?

Ah, no need to take action - I'm just saying that line continuations + whitespace (hanging indent vs. visual indent) are inconsistent, in addition to some hanging indents not even being correct.
However, that was all preexisting, so not worth defecting.

Copy link
Contributor Author

@xuchenhan-tri xuchenhan-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 unresolved discussions (waiting on @amcastro-tri)


multibody/parsing/detail_sdf_parser.cc line 558 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Ah, no need to take action - I'm just saying that line continuations + whitespace (hanging indent vs. visual indent) are inconsistent, in addition to some hanging indents not even being correct.
However, that was all preexisting, so not worth defecting.

I see. So not this particular line, gotcha.

Copy link
Contributor

@amcastro-tri amcastro-tri left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees amcastro-tri,EricCousineau-TRI(platform) (waiting on @xuchenhan-tri)

@amcastro-tri amcastro-tri merged commit b6aeb35 into RobotLocomotion:master Sep 22, 2022
@amcastro-tri amcastro-tri deleted the continuous-joint branch September 22, 2022 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: feature This pull request contains a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should support continuous joints from SDFormat
3 participants