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

Split panda_link5 into lower/upper sections and improve collision filtering #74

Merged

Conversation

calderpg-tri
Copy link
Contributor

@calderpg-tri calderpg-tri commented Jan 13, 2025

Replaces #71


This change is Reviewable

@calderpg-tri
Copy link
Contributor Author

As the title says, splits panda_link5 from the previous
Screenshot from 2025-01-15 00-16-03

into panda_link5_lower
Screenshot from 2025-01-15 00-16-13

and panda_link5_upper
Screenshot from 2025-01-15 00-16-24

to allow better collision filtering around the wrist, and adds more complete collision filtering around links throughout the arm. See anzu#15207 for code changes resulting from the model changes here.

+@rcory and +@siyuanfeng-tri for review/delegation, thanks!

Copy link
Contributor

@rcory rcory left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee siyuanfeng-tri, LGTM missing from assignee rcory, platform LGTM missing (waiting on @calderpg-tri and @siyuanfeng-tri)


franka_description/README.md line 9 at r1 (raw file):

ability to filter collisions around the wrist. In this model of the hand, the
fingers are independently actuated, rather than using <mimic> tag, which Drake
does not yet support.

btw Drake does support a drake:mimic tag, it is just not used in these models.


franka_description/README.md line 49 at r1 (raw file):

### Collision Filters

There are several collision filters applied:

nit I'd suggest rewording to something like:

The following collision filter groups are defined, all of which filter collisions within their own group:

franka_description/urdf/panda_arm.urdf line 607 at r1 (raw file):

    </actuator>
  </transmission>
  <drake:collision_filter_group name="group_link0123">

To be honest, I'm not quite sure how to review the correctness of these filter groups, or exactly what problem you're addressing with these changes. Same comment for the filter definitions in panda_arm_hand.urdf

Can you elaborate? Is there an issue that explains the need for these changes?

Copy link
Contributor Author

@calderpg-tri calderpg-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, LGTM missing from assignee siyuanfeng-tri, LGTM missing from assignee rcory, platform LGTM missing (waiting on @rcory and @siyuanfeng-tri)


franka_description/README.md line 9 at r1 (raw file):

Previously, rcory (Rick Cory) wrote…

btw Drake does support a drake:mimic tag, it is just not used in these models.

I left the original language in the comment. The hand model probably could use the <mimic>tag, but given that we don't use this hand model I think it's best left alone.


franka_description/urdf/panda_arm.urdf line 607 at r1 (raw file):

Previously, rcory (Rick Cory) wrote…

To be honest, I'm not quite sure how to review the correctness of these filter groups, or exactly what problem you're addressing with these changes. Same comment for the filter definitions in panda_arm_hand.urdf

Can you elaborate? Is there an issue that explains the need for these changes?

A number of these filter entries are simply a consequence of splitting panda_link5 - what would formerly have been implicitly filtered as adjacent bodies now needs to explicitly include both panda_link5_lower and panda_link5_upper. In redoing these filters, I have also explicitly included for clarity the links that would otherwise be implicitly filtered - e.g. in the existing model (6, 7) and (7, 8) are filtered implicitly and (6, 8) is filtered explicitly.

Beyond those changes, I have tried to make the filters as complete as possible given the actual kinematic/geometric design of the arm to reduce collision checking work and avoid the possibility of spurious self-collision avoidance distances/gradients. By design, group_link0123, group_link1234, and group_link3456 reflect groups of links in which none of the links can ever self-collide (you can exercise the model in meshcat and see that these links can't collide, so long as you stay within joint limits). In fact, if the true geometry of the robot was used by the internal self-collision check, all collisions in (3, 4, 5, 6, 7, 8) could be filtered.

The overarching goal is to eliminate entirely spurious self-collision avoidance distances/gradients so that we can use accurate gripper collision models and get consistently reasonable collision avoidance behavior without having to do the current dangerous things (i.e. filtering between gripper and panda_link5, even though these collisions are possible).

Copy link
Contributor

@rcory rcory left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 1 unresolved discussion, LGTM missing from assignee siyuanfeng-tri, platform LGTM missing (waiting on @calderpg-tri and @siyuanfeng-tri)


franka_description/README.md line 9 at r1 (raw file):

Previously, calderpg-tri wrote…

I left the original language in the comment. The hand model probably could use the <mimic>tag, but given that we don't use this hand model I think it's best left alone.

I think you misunderstood my suggestion. I don't think we should change the model, but the documentation here is incorrect. It says "Drake does not yet support the mimic tag. We should at least remove that wording from this documentation (since Drake does in fact support a mimic tag).


franka_description/urdf/panda_arm.urdf line 607 at r1 (raw file):

Previously, calderpg-tri wrote…

A number of these filter entries are simply a consequence of splitting panda_link5 - what would formerly have been implicitly filtered as adjacent bodies now needs to explicitly include both panda_link5_lower and panda_link5_upper. In redoing these filters, I have also explicitly included for clarity the links that would otherwise be implicitly filtered - e.g. in the existing model (6, 7) and (7, 8) are filtered implicitly and (6, 8) is filtered explicitly.

Beyond those changes, I have tried to make the filters as complete as possible given the actual kinematic/geometric design of the arm to reduce collision checking work and avoid the possibility of spurious self-collision avoidance distances/gradients. By design, group_link0123, group_link1234, and group_link3456 reflect groups of links in which none of the links can ever self-collide (you can exercise the model in meshcat and see that these links can't collide, so long as you stay within joint limits). In fact, if the true geometry of the robot was used by the internal self-collision check, all collisions in (3, 4, 5, 6, 7, 8) could be filtered.

The overarching goal is to eliminate entirely spurious self-collision avoidance distances/gradients so that we can use accurate gripper collision models and get consistently reasonable collision avoidance behavior without having to do the current dangerous things (i.e. filtering between gripper and panda_link5, even though these collisions are possible).

Thanks for the explanation.

Copy link
Contributor

@jwnimmer-tri jwnimmer-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: 1 unresolved discussion, LGTM missing from assignee siyuanfeng-tri, platform LGTM missing (waiting on @calderpg-tri and @siyuanfeng-tri)


franka_description/README.md line 9 at r1 (raw file):
My suggestion would just be to insert the weasel word "fully" as in:

... which Drake does not yet fully support.

Some parts of Drake support it, but plenty more do not.

Copy link
Contributor

@jwnimmer-tri jwnimmer-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: 1 unresolved discussion, LGTM missing from assignee siyuanfeng-tri, platform LGTM missing (waiting on @calderpg-tri and @siyuanfeng-tri)


franka_description/README.md line 9 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

My suggestion would just be to insert the weasel word "fully" as in:

... which Drake does not yet fully support.

Some parts of Drake support it, but plenty more do not.

(I would okay be okay just leaving it alone since it's unchanged and out of scope in this PR, but Rick seems to want to fix it now, in which case "fully" is a quick and easy fix.)

Copy link
Contributor

@rcory rcory 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: 1 unresolved discussion, LGTM missing from assignee siyuanfeng-tri, platform LGTM missing (waiting on @calderpg-tri and @siyuanfeng-tri)


franka_description/README.md line 9 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

(I would okay be okay just leaving it alone since it's unchanged and out of scope in this PR, but Rick seems to want to fix it now, in which case "fully" is a quick and easy fix.)

I'm on board with @jwnimmer-tri's suggestion (and it's a simple fix).

Copy link
Contributor Author

@calderpg-tri calderpg-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: all discussions resolved, LGTM missing from assignee siyuanfeng-tri, platform LGTM missing (waiting on @rcory and @siyuanfeng-tri)


franka_description/README.md line 9 at r1 (raw file):

Previously, rcory (Rick Cory) wrote…

I'm on board with @jwnimmer-tri's suggestion (and it's a simple fix).

Done.


franka_description/README.md line 49 at r1 (raw file):

Previously, rcory (Rick Cory) wrote…

nit I'd suggest rewording to something like:

The following collision filter groups are define, all of which filter collisions within their own group:

Done.

Copy link
Contributor Author

@calderpg-tri calderpg-tri left a comment

Choose a reason for hiding this comment

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

-@siyuanfeng-tri

Reviewable status: all discussions resolved, platform LGTM missing (waiting on @rcory)

Copy link
Contributor Author

@calderpg-tri calderpg-tri left a comment

Choose a reason for hiding this comment

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

+@jwnimmer-tri for platform, thanks!

Reviewable status: all discussions resolved, LGTM missing from assignee jwnimmer-tri, platform LGTM missing (waiting on @rcory)

Copy link
Contributor

@rcory rcory left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all discussions resolved, LGTM missing from assignee jwnimmer-tri, platform LGTM missing (waiting on @calderpg-tri)

Copy link
Contributor

@jwnimmer-tri jwnimmer-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:

Reviewed 2 of 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, platform LGTM from [jwnimmer-tri] (waiting on @calderpg-tri)

@jwnimmer-tri jwnimmer-tri merged commit 6b67e4c into RobotLocomotion:master Jan 21, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants