Skip to content

Add named frames to CollisionObjects#50

Merged
rhaschke merged 12 commits intomoveit:melodic-develfrom
felixvd:extend-planning-scene
May 20, 2019
Merged

Add named frames to CollisionObjects#50
rhaschke merged 12 commits intomoveit:melodic-develfrom
felixvd:extend-planning-scene

Conversation

@felixvd
Copy link
Copy Markdown
Contributor

@felixvd felixvd commented Feb 28, 2019

This belongs to ros-planning/moveit#1060 and fixes #47 (now properly targets melodic, not kinetic).

@henningkayser
Copy link
Copy Markdown
Member

@felixvd I really like this feature and hope #1060 will get merged soon!

Copy link
Copy Markdown
Contributor

@BryceStevenWilley BryceStevenWilley left a comment

Choose a reason for hiding this comment

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

LGTM, with one correction.

Copy link
Copy Markdown
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

I generally approve this. Not sure yet, whether we really need the ValidateConstraintFrames service.
Shouldn't this be called TransformConstraintFrames, because it's main task is to transform the transforms into known link transforms.

@felixvd
Copy link
Copy Markdown
Contributor Author

felixvd commented May 3, 2019

All of the alternatives I could come up with are somewhat imprecise and can be misunderstood. In my mind, the important point is that the constraints are changed from an unusable/invalid state to a formulation that the planner can use. I considered others, too:

  • FixConstraintFrames can be misread as "making immobile or immutable"
  • ResolveConstraintFrames as suggested by @henningkayser reads too much like a lookup or processing operation to me
  • ValidateConstraintFrames does not make clear that "repaired" constraints are returned
  • TransformConstraintFrames sounds generic, does not express the intention behind the action

I think naming and readibility is important, so I'm all for being verbose if it helps understanding. The function does not come up often anyway, so it won't clog any code. TransformConstraintFramesToRobotLink would be fine in my book.

@rhaschke rhaschke merged commit 6350bfb into moveit:melodic-devel May 20, 2019
@v4hn v4hn mentioned this pull request May 8, 2020
18 tasks
rhaschke added a commit to ubi-agni/moveit_msgs that referenced this pull request May 10, 2020
This (partially) reverts commit 6350bfb.
We keep the improvements in the comments.
v4hn pushed a commit that referenced this pull request May 10, 2020
This (partially) reverts commit 6350bfb.
We keep the improvements in the comments.
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