CI failed

@v4hn I know. And I explicitly asked for feedback on this PR before I tackle all those issues due to the API change.

@henningkayser, thanks for looking into this PR!

What is the reason to move enableGroup() to CollisionRequest/DistanceRequest?

active_components_only / enableGroup() were already part of DistanceRequest - so no change there.
However, they were missing from CollisionRequest, but only were introduced in FCL's CollisionData struct.
I lifted the definition to DistanceRequest to allow for arbitrary active components - up to now, active_components were configured from the joint model group (name), if available.

Since the robot model is known inside collision_world_fcl I also don't know why anything else but the group or link names should be passed by the corresponding requests. Wouldn't it be better to allow setting both? All combinations of group_name and active_components_only could be easily specified.

I don't get your question. Currently, it's not possible to specify an arbitrary set of links. This only becomes possible when active_components_only becomes publicly accessible.

What is the reason to move enableGroup() to CollisionRequest/DistanceRequest?

active_components_only / enableGroup() were already part of DistanceRequest - so no change there.
However, they were missing from CollisionRequest, but only were introduced in FCL's CollisionData struct.
I lifted the definition to DistanceRequest to allow for arbitrary active components - up to now, active_components were configured from the joint model group (name), if available.

I see, that makes sense!

Since the robot model is known inside collision_world_fcl I also don't know why anything else but the group or link names should be passed by the corresponding requests. Wouldn't it be better to allow setting both? All combinations of group_name and active_components_only could be easily specified.

I don't get your question. Currently, it's not possible to specify an arbitrary set of links. This only becomes possible when active_components_only becomes publicly accessible.

My thought was that the requests could have public std::string members for the names of joint group and link models. The collision_world_fcl has access to the robot model and could call enableGroup() or instantiate active_components_only itself, maybe via a function enableActiveComponents(). This way we don't have to change any existing code and could also verify if new requests with selected components are valid (links are part of the robot model).

@henningkayser I still don't get your point. My proposal was to augment the public request with the list of robot links to be considered for collision. Up to now, you can only provide the group name and from that, the link list is generated in private code. So I guess, we agree that it would be useful to specify links directly and to have a convenience function (enableGroup) to enable all links in a group.

Do you suggest to use link names instead of link pointers in the public interface to avoid passing invalid link pointers?

Do you suggest to use link names instead of link pointers in the public interface to avoid passing invalid link pointers?

Yes exactly this. On top of that we could keep the group_name and make it optional which one should be specified. It's just a suggestion to keep support for the existing interface. Setting the actual link pointers could then be done in private code.

Using link names vs. link pointers is much more costly: To get the the actual link pointers, one needs to lookup the links in a map. Keeping the existing interface, i.e. the group_name, and adding the list of link names creates an ambiguity: If both of them are filled, which one should be considered?
That's why I decided to remove the group_name and enforce users to call enableGroup().

Fair enough. One could filter for both parameters, i.e. if group_name and active_componentes_only is set then only the links in the corresponding group are considered. But this is definitely not necessary. I think making active_components_only public is really useful so I will approve once the conflicts and remaining changes are resolved.
@v4hn I bet you have an opinion on this, right?

whether you think this generalization to allow for collision checking of arbitrary link groups (in contrast to only pre-defined JMGs) is desired or not.

I like the simplicity of doing everything with just JMGs. An alternative idea would be to allow JMGs to be constructed at realtime. I ran into this limitation recently - you have to create a string-based SRDF and load it at startup currently. Just a thought.

An alternative idea would be to allow JMGs to be constructed at realtime.

That would be great. However, I don't want to name the group.

... you have to create a string-based SRDF and load it at startup currently.

This is not an option. There should be a programmatic API.

I procrastinated by reading this. I'm not familiar enough with the code to have an opinion on a624ef5, just some comments as a first time reader:

• Being able to check for arbitrary link groups sounds useful for grasp planning (or foot placement, or object placement...). I have looked for a setCollisions convenience function that applies to all child links (and attached objects) before.
• isPoseColliding looks good to me, but it isn't clear from the name that only link's descendants are considered for collision checking. You have to deduce it from the fact that isStateColliding exists. Would recommend areChildLinksColliding or isEndEffectorColliding (to cover the assumed use case explicitly).
• active_components_only and enableGroup confuse me. The former sounds like it would be a bool, the latter makes me think groups aren't enabled by default. If I understand right, active_components_only is a list of components with the meaning only_use_these_components. I think active_components and setActiveComponents might have been easier to read. I know this is not related to this PR, I'm just noting it.

I just reviewed all the comments - it seems to me we were so close to getting this merged in, but then it got dropped. Can you resolve the conflicts and we finish this?

Yes, I should address the open issues so that we can finally merge this. I will add a corresponding TODO to my list.