moveit / moveit2

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

Unable to set a different default pose for collision checks in the MoveItSetupAssistant #2772

Open firesurfer opened 8 months ago

firesurfer commented 8 months ago

Description

I have a system where the enforced default pose of all joints on 0.0 has multiple self collisions. These collisions are therefore falsely disabled.

I could not find a way to change the default pose.

Your environment

Expected behaviour

From the description in the "Robot Poses" tab grafik

I would expect the the setup assistant to use the first listed pose as default pose for the collision check. Apparently that is not the case.

EDIT: I just did a bit of research in the code: https://github.com/ros-planning/moveit2/blob/df78880a0230dbcf0ad06833d91f2c3d70d9bec3/moveit_setup_assistant/moveit_setup_srdf_plugins/src/compute_default_collisions.cpp#L447C58-L448C73

If I the intended behavior is as described in the UI then this line should do something like:

a) Checking if there any poses defined for the joint group. b) If yes -> Take the first one If no -> Call setToDefaultValues() as it is done at the moment.

github-actions[bot] commented 6 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.

firesurfer commented 6 months ago

@sjahr sorry to tag you directly as I do not know who else would be the right person to address.

Is there any interest in fixing this bug or am I just using the tool the wrong way?

github-actions[bot] commented 5 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.

firesurfer commented 5 months ago

Bump....should not go stale. This is a real issue...

rhaschke commented 5 months ago

There is nothing like a default pose. You can define several (or none) named poses in MSA - as you stated. To determine the allowed collision matrix (I guess you are referring to this), we randomly sample thousands of poses. If these are always in collision for a given pair of joints, this pair is disabled, because there is no value in performing a collision check if it always reports a collision. Maybe you are observing this phenomenon? Please clarify, what you mean by

These collisions are therefore falsely disabled.

firesurfer commented 5 months ago

Thanks for your answer.

What I meant by "default positions" are from my understanding the starting positions of the model.

When we take a look at the collisions_updater tool there is also the option: --default disable default colliding pairs as well as in the generated xml file: <disable_collisions link1="my_first_link" link2="my_second_link" reason="Default"/>

When I take a look at what entries are marked as "reason=Default" it perfectly matches all links that are in collision when the all joint positions of the model are set to 0.

Also the code I linked explicitly calls them default state

And the comment in the linked line says:

  scene.getCurrentStateNonConst().setToDefaultValues();  // set to default values of 0 OR half between low and high
                                                         // joint values

The issue is now: There is no way to define different default positions. If two links of my model collide in the default state the collisions will be disabled. As a mentioned before: Yes it is possible to use the collisions_updater with --default. But then I have the issue that for some other link pairs where the collisions should have been disabled they are still enabled .

rhaschke commented 5 months ago

Thanks for the clarification. I wasn't aware of the function disableDefaultCollisions() in MSA and I think, it should be removed. collisions_updater disables that feature by default.

To answer your question: No, there is no way to define another default state. That's because, this default state doesn't have a particular meaning in MoveIt. It's just a way to initialize a state within valid bounds.

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.

mikeferguson commented 2 weeks ago

I'm making this persistent - not because we want to add default pose change, but rather because we should address the issue noted in https://github.com/moveit/moveit2/issues/2772#issuecomment-2206791777