personalrobotics / prpy

Python utilities used by the Personal Robotics Laboratory.
BSD 3-Clause "New" or "Revised" License
62 stars 19 forks source link

Fixed spurious CloneException errors in planning stack. #298

Closed psigen closed 8 years ago

psigen commented 8 years ago

Due to OpenRAVE weirdness, we are forced to throw a CloneException if a robot is cloned from a self-colliding configuration. We also clone at the beginning of some of our planning operations.

This means that calling a planning method while a robot is in self-collision can create a CloneException, which is generally not expected or handled in code that is calling a planning method. It is also not clear to calling code that this is actually a failure within a planner, and it breaks MetaPlanner logic.

In this PR, ClonedPlanningMethods will catch CloneExceptions and wrap them as ClonedPlanningErrors instead, which inherit from PlanningError and are thus handled correctly throughout the planning stack as a form of planning failure.

mkoval commented 8 years ago

I am concerned that letting meta-planners catch CloneException as a PlanningError will mask deeper issues. All ClonedPlanningMethods will fail if the start configuration is in self-collision. There are only a handful of LockedPlanningMethods, which are all fast heuristic planners, so it is unlikely that any non-trivial planning calls will succeed.

I'd rather fail fast by letting the CloneException bubble up to the user, instead of silently ignoring most of the motion planners in a Sequence. What is the motivation for this change in behavior?

psigen commented 8 years ago

The motivation for this is that no one actually handles CloneException anywhere right now. It's simply not something you expect to happen when making an opaque planning method call. You just haven't run into it.

If I put the robot into collision, and tell it to plan to another place, I don't expect to have to catch a CloneException to determine that I gave it an invalid starting location. I expect a PlanningError, because it failed to plan.

Note that the only reason we can get a CloneException right now is a situation in which no planner can succeed. LockedPlanningMethods are also going to fail. So I see no reason that we shouldn't be handling it the same way as any other error that makes planning fail.

mkoval commented 8 years ago

I see your point. I was missing the obvious (in hindsight) fact that a LockedPlanner would also fail because the robot starts in self-collision. I will merge this once Travis is happy.

psigen commented 8 years ago

@mkoval bump.

mkoval commented 8 years ago

Travis is happy. Merging.