regression in FixStartStateCollision adapter

moveit / moveit2

:robot: MoveIt for ROS 2

https://moveit.ai/

BSD 3-Clause "New" or "Revised" License

1.04k stars 512 forks source link

regression in FixStartStateCollision adapter #2849

Open rhaschke opened 4 months ago

rhaschke commented 4 months ago

This reports issues observed with the FixStartStateCollision adapter in https://github.com/moveit/moveit_task_constructor/pull/573#issuecomment-2128902737:

The adapter seems to interpolate between an in-collision start state and the found non-collision state. However, these states are usually in collision too, resulting in the plan validation to fail. MoveIt1 doesn't behave like that, it jumps to the collision-free state directly (and ignores the invalid start state during validation).
If the adapter fails to find a collision-free state, it erroneously reports SUCCESS as its message.

sjahr commented 4 months ago

Thanks for reporting this issue! At a first glance I don't see any regression between moveit1 and moveit2 in the FixStartStateCollision adapter on the Humble branch. Do you see anything I am missing?

In main I replaced the FixStartState adapter in https://github.com/moveit/moveit2/pull/2429 with a simple CheckStartStateCollision adapter. My rationale is that an adapter that introduces small jumps is un-intuitive and might lead to undesired behavior, especially given that is was used almost everywhere as a default. I think the decision about what to do when the start state is in collision should be made in a higher level logic than the planning pipeline.

We refactored the whole planning pipeline in moveit2 recently.

rhaschke commented 4 months ago

Do you see anything I am missing?

I see exactly the regression I described: In MoveIt1 it works, it MoveIt2 it fails as described. If there are no code changes in the adapter itself, which would explain the interpolation to the new state, maybe time parameterization applies (now) to the whole trajectory (including both, the initial state and the added no-collision waypoint) and adds more waypoints in between? Maybe such kind of interpolation was added there? I don't have the time to dig into this issue and I don't have an overview of changes applied to MoveIt2. That's why I just reported the issue.

github-actions[bot] commented 2 months ago

This issue is being labeled as stale because it has been open 45 days with no activity. It will be automatically closed after another 45 days without follow-ups.

github-actions[bot] commented 2 weeks ago

This issue is being labeled as stale because it has been open 45 days with no activity. It will be automatically closed after another 45 days without follow-ups.