Weird semantics of default collision entries in ACM

[rhaschke]

Background: franka_description introduced coarse collision models to mimic the behavior of self-collision checking of the real robot controller. Thus, planned trajectories will be executable without surprising new collisions.
Problem: collision model of hand is huge and doesn't allow to approach objects for grasping
Solution proposal: Use the coarse collision models for self-collision checking (only) and introduce smaller models for collision checking with the environment.

This would require defining fallback defaults:

• NEVER collide coarse collision models with environment objects, but specify some self-collision pairs
• ALWAYS collide accurate collision models with environment objects, but disable self-collision via specific pairs

I knew that MoveIt provides a default value for collision objects. However, surprisingly this default is not considered as a fallback, but as an override taking precedence over explicitly declared collision pairs:
https://github.com/ros-planning/moveit/blob/f69a324617a14dc5f90b897d8bf6ae9c995960fd/moveit_core/collision_detection/src/collision_matrix.cpp#L281-L286

The corresponding ACM method setDefaultEntry() is only used once in the code base, namely to establish a custom collision function for the kinematic visibility constraint:
https://github.com/ros-planning/moveit/blob/779b7c8b019f70898d4de3189f9261c9697d9b9f/moveit_core/kinematic_constraints/src/kinematic_constraint.cpp#L1115

For this reason, I'm tempted to change the semantics, such that the default is considered a fallback again.
Interestingly, the original implementation was intended to serve as a fallback as well, but @isucan changed it 10 years ago.
I'm curious what other maintainers think about this idea?
@v4hn, @simonschmeisser, @tylerjw, @henningkayser, @davetcoleman

[simonschmeisser]

I was recently wondering about why this exists as well and actually a "valid" use case that I'm likely to implement in the coming month is something akin to https://github.com/PickNikRobotics/tool_changing for the exact same purpose, being able to "disable" JointModelGroups/ detachable end effectors.

[rhaschke]

@simonschmeisser, do you plan to rely on the current, in my view weird behavior, or the proposed one?

[simonschmeisser]

I'm not sure I understand the proposal. Afaik the ACM does not differentiate between self-collisions and collisions with environment objects?

[v4hn]

i looked into this before as well and found the default entry fields unusable.

thanks for looking into it, I like your proposal. however, by what I understand from your post the current behavior enables us to specify a custom method for every pair involving a specific object. if you take that away, is there another way left to do the same without changing each individual entry involving the object? or would you propose just that for the visibility constraint case?

[rhaschke]

To clarify: The current implementation enforces a specific allowed collision result as soon as a default is specified for a given name. If other specifications for collision pairs exist that involve the same name, these are ignored.

The proposed approach would prioritize explicitly specified pairs and only fall back to the default if no such pair exists.
Practically, I don't see an issue with the visibility constraint: The cone object introduced by the constraint will not have any specific collision pairs defined, such that the default applies anyway. There is no need to enforce the default.

[v4hn]

Practically, I don't see an issue with the visibility constraint: The `cone` object introduced by the constraint will not have any specific collision pairs defined, such that the default applies anyway. There is no need to _enforce_ the default.

True. 👍 I will review and merge a patch if you provide it.

[v4hn]

On Fri, Oct 29, 2021 at 03:07:44AM -0700, Michael Görner wrote: I will review and merge a patch if you provide it.

That being said, you will still want to implement support for default collisions in srdfdom. I just looked at it again after you said you saw support for it and could not find anything.

[henningkayser]

I agree, the semantics behind "default" is really weird. To me, having default entries with only 1 link is the only use case that really makes sense: say, you have a collision object that you never want to collide in any case, but you don't want to fill in all the possible pairs. But why would you need an additional default value function for existing pairs if you could just fix the existing entries? Otherwise, default (if we should name it that) should be a fallback, other entries take priority. What would be the use case for a "default override"? To me it sounds like a lazy solution for not having to change the ACM. If I care about keeping the original entries (defaults + specialization) and still want to override these at runtime, there is always the option of creating an "override" ACM that takes precedence for your very specific use case and then you can check the original one as a "fallback".

[JafarAbdi]

As @simonschmeisser I used this feature for the tool changing capability, I do agree that the current semantics is confusing and I had to read the codebase to understand how it works, +1 for making default a fallback rather than override

I don't think the newly proposed solution would break the capability but even if it does we should fix the semantics and adapt it accordingly

That being said, you will still want to implement support for default collisions in srdfdom.

I think we should keep interpreting the disable_collisions tags as default entries which I believe is what the current semantic https://github.com/ros-planning/moveit/blob/master/moveit_core/planning_scene/src/planning_scene.cpp#L162

[v4hn]

I think we should keep interpreting the disable_collisions tags as default entries which I believe is what the current semantic https://github.com/ros-planning/moveit/blob/master/moveit_core/planning_scene/src/planning_scene.cpp#L162

You lost me there. Of course disable_collisions sets defaults for pairs.
But that cannot be used at this point to define a default for a single link, but that's relevant to decide the default for a link w.r.t. new objects. This is at least what Robert and I talked about in a recent phone call.