search

moveit / moveit2

:robot: MoveIt for ROS 2
https://moveit.ai/
BSD 3-Clause "New" or "Revised" License
1.06k stars 516 forks source link

RobotState: Throw exception in case of unknown frame transforms #614

Open henningkayser opened 3 years ago

henningkayser commented 3 years ago

Related to: https://github.com/ros-planning/moveit2/pull/612

Currently, if a RobotState doesn't know a link frame, getFrameTransform() will complain about it and return an identity transform. I think it would be a useful improvement to not only print a warning or error, but actually throw an exception, especially thinking about possible future use cases like dynamic robot models.

v4hn commented 3 years ago

This pattern (has* and get*) exists in multiple places in MoveIt, doesn't it? We can change it in ROS2, but I believe the original idea was that MoveIt-based code can run in normal operation without relying on exceptions. -fno-exceptions is interesting to some people.

AdamPettinger commented 3 years ago

What about using std::optional? That way we don't have to return identity or throw an exception. This of course would be a large and API-breaking change...

v4hn commented 3 years ago

What about using std::optional? That way we don't have to return identity or throw an exception. This of course would be a large and API-breaking change...

Sounds like the better option to me, also because it mimics the current interface in one method. We might just as well offer this as an alternative API, so there is no need to break the old APIs for no reason. Though it could be part of the moveit2 "cleanup".

BTW: is there a moveit->moveit2 migration guide somewhere already to list such modifications/better alternatives? It seems you either did not continue the tradition of the MIGRATION.md, or do not have any non-ROS2-typical migration steps (that could be documented just as well).

tylerjw commented 3 years ago

BTW: is there a moveit->moveit2 migration guide somewhere already to list such modifications/better alternatives? It seems you either did not continue the tradition of the MIGRATION.md, or do not have any non-ROS2-typical migration steps (that could be documented just as well).

There isn't yet because we haven't really changed much in the way of the MoveIt API itself yet (except for servo, I'm sorry there is not a guide for that). This would be a change and we should write a migration guide for it. We are doing those migrations for customers and we really should author a tutorial at least for the various migrations that we've been doing.

Another option would be to use StatusOr from abseil: https://chromium.googlesource.com/chromium/src.git/+/refs/heads/main/third_party/abseil-cpp/absl/status/statusor.h

We've worked on a few projects that have copied this into their source as it offers a really nice way of solving this API problem. I am not advocating for depending on the whole abseil library, but presenting the option that we could adopt StatusOr.