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

Add named frames to CollisionObjects #50

Merged
merged 12 commits into from
May 20, 2019

Conversation

felixvd
Copy link
Contributor

@felixvd felixvd commented Feb 28, 2019

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

srv/GetConstraintValidity.srv Outdated Show resolved Hide resolved
msg/CollisionObject.msg Outdated Show resolved Hide resolved
msg/CollisionObject.msg Outdated Show resolved Hide resolved
@henningkayser
Copy link
Member

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

Copy link
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.

msg/CollisionObject.msg Outdated Show resolved Hide resolved
Copy link
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
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.

None yet

4 participants