I'm excited about this improvement!

@v4hn ping for review

The planning pipeline is specified by a prefixing the planner id - <pipeline>|<planner_id> where is the parameter namespace of the pipeline used for MoveItCpp

What's the rationale for using | as separator here?

Historically, paths, parameters, namespaces, TF frames and similar concepts which can be "contained" in namespaces have used a forward slash (ie: /).

The planning pipeline is specified by a prefixing the planner id - <pipeline>|<planner_id> where is the parameter namespace of the pipeline used for MoveItCpp

What's the rationale for using | as separator here?

Historically, paths, parameters, namespaces, TF frames and similar concepts which can be "contained" in namespaces have used a forward slash (ie: /).

Currently, it's technically not forbidden to use / inside the planning pipeline name as it can represent absolute ore relative parameter namespaces and I wanted to keep support for the moveit_cpp.yaml without the overhead of adding a unique id generator. Also, the delimiter is not used anywhere to my knowledge and also indicates a specific separation of the parts.

I would probably suggest to standardise naming here such that / is only used for separating namespaces and nothing else, as / has a long history and will be the most intuitive to most (all?) users.

Also, the delimiter is not used anywhere to my knowledge and also indicates a specific separation of the parts.

it certainly separates things, but the pipe symbol has historically has quite a different use. It's actually more of a connecting symbol than a separating one.

I would probably suggest to standardise naming here such that / is only used for separating namespaces and nothing else, as / has a long history and will be the most intuitive to most (all?) users.

That's the point, it's already used for separating namespaces (pipeline config). Adding the planner id (which can consist of group name + algorithm or anything arbitrary defined by the planner) as a simple subnamespace would be a break in the representation, therefore I wouldn't use /.

Also, the delimiter is not used anywhere to my knowledge and also indicates a specific separation of the parts.

it certainly separates things, but the pipe symbol has historically has quite a different use. It's actually more of a connecting symbol than a separating one.

Hm, the planner_id somehow represents a connection between algorithm and planner instance.

That's the point, it's already used for separating namespaces (pipeline config). Adding the planner id (which can consist of group name + algorithm or anything arbitrary defined by the planner) as a simple subnamespace would be a break in the representation, therefore I wouldn't use /.

Do I understand you correctly that this would essentially only be a problem if the string which identifies the planner and algorithm contains a / and is directly used to index into parameters?

What about decoupling this?

I'm only a single voice here, and it's not a deal breaker for me, but using a / instead of | seems much more natural to me.

That's the point, it's already used for separating namespaces (pipeline config). Adding the planner id (which can consist of group name + algorithm or anything arbitrary defined by the planner) as a simple subnamespace would be a break in the representation, therefore I wouldn't use /.

Do I understand you correctly that this would essentially only be a problem if the string which identifies the planner and algorithm contains a / and is directly used to index into parameters?

What about decoupling this?

I agree that a separate planning_pipeline field inside MotionPlanRequest and PlannerInterfaceDescription would be much cleaner. That could co along with corresponding interface changes for the MoveGroupInterface, which could.

I'm only a single voice here, and it's not a deal breaker for me, but using a / instead of | seems much more natural to me.

True, it might seem more natural, but in the end it's just a delimiter with a specific convention.
To me this approach seems easier than working around handling actual namespaces in the pipeline specifier.

I agree that a separate planning_pipeline field inside MotionPlanRequest and PlannerInterfaceDescription would be much cleaner. That could co along with corresponding interface changes for the MoveGroupInterface, which could.

Changing the message is not what I meant.

My suggestion was to not think of planner/algo as directly related to a ROS parameter namespace, but as an actual identifier. That identifier then could comply with regular ROS naming and users' intuition.

Internally, it would be decomposed and after some post-processing or mapping, could be used to refer to specific namespaces on the ROS parameter server. But it would not be a 1-to-1 mapping, which decouples the occurrence of / from being potentially difficult with parameter namespaces.

I agree with Gijs. Using any other symbol than / as delimiter is
counterintuitive.
You actually invoke a planner within a planner manager, so the
subfolder-analogy is sound as well.

Concerning namespaces, I'm still not sure why you would require more
namespacing with sub-namespaces.
You would end up with planner strings like ompl/configuration1|RRTConnect. That's hard to parse and ambiguous without further insight.
Even if MoveItCpp supports this at the moment, I don't see why we can't change
that for overall better usage and structure.
move_group/planners/xyz/configuration should be very much fine.
If you want to invoke a MoveItCpp with alternative configurations, just upload
them to a different namespace too or provide an optional list to restrict the
available planners for a concrete MoveItCpp instance.

There is a lot of merit in having all planner configurations supported from
the MotionPlanning panel in rviz to try them out and thus they should all be
available in one sub-namespace, e.g., planners).

fun fact: I sent the previous message by mail, but github's mailserver rejected it stating:

    The user you are trying to contact is receiving mail at a rate that
    550-5.2.1 prevents additional messages from being delivered. For more
    550-5.2.1 information, please visit 550 5.2.1
    https://support.google.com/mail/?p=ReceivingRatePerm

@gavanderhoorn

My suggestion was to not think of planner/algo as directly related to a ROS parameter namespace, but as an actual identifier. That identifier then could comply with regular ROS naming and users' intuition.

Makes sense, we just don't have a unique 'planner' here. We would have to generate or specify this pipeline id which would also require another parameter which maps to an actual config namespace.

Internally, it would be decomposed and after some post-processing or mapping, could be used to refer to specific namespaces on the ROS parameter server. But it would not be a 1-to-1 mapping, which decouples the occurrence of / from being potentially difficult with parameter namespaces.

The 1-to-1 mapping is exactly what should be provided imo, and ids need to be coupled to the parameter namespace. What I'm reading here is that we have to encode an actual namespace (pipeline config) into an id (pipeline name) so that we can represent an id (planner id) as a namespace (pipeline name + planner id). To me this just looks extra complicated and not straight forward at all. The more I think about this, the more I'm actually voting for adding an explicit pipeline field to the message. Especially, since pipeline is a property for MoveGroup/MoveItCpp and planner_id is a property for the planning plugin. Mixing both up in a single string doesn't smell right.

@v4hn

I agree with Gijs. Using any other symbol than / as delimiter is
counterintuitive.
You actually invoke a planner within a planner manager, so the
subfolder-analogy is sound as well.

Hm, not convinced, sorry. To me / represents a parent/child relationship between elements of the same type. :, ::, -, # would just be as intuitive imo. I'm fine with switching to this representation though, but only if we can still support the full range of options for MoveGroup and MoveItCpp.
That means we have to add a pipeline name identifier to the MoveItCpp options that is then used instead of the namespace. This one could default to the namespace name if it is a direct child of a specified parent namespace (similar to your suggestion below).

Concerning namespaces, I'm still not sure why you would require more
namespacing with sub-namespaces.
You would end up with planner strings like ompl/configuration1|RRTConnect. That's hard to parse and ambiguous without further insight.

Well, it's supported and I don't see a convincing argument to remove support for it, as it can be very useful for specifying instances running on or using different resources. How is it harder to parse than anything else and with what is this representation ambiguous? I totally understand that it doesn't look pretty - too much information for a simple algorithm identifier - but for now it is the most explicit and still generic representation that supports all planners available with MoveItCpp.

Even if MoveItCpp supports this at the moment, I don't see why we can't change
that for overall better usage and structure.
move_group/planners/xyz/configuration should be very much fine.

I'm fine with a convention like this for MoveGroup and generate a valid MoveItCpp PlanningPipelineOptions with it (that would even make one the pipeline_names parameter redundant). It would be still nicer to just be able to use MoveItCpp parameters or yaml files directly.
There is probably no way around adding a pipeline_name parameter to the options struct.

If you want to invoke a MoveItCpp with alternative configurations, just upload
them to a different namespace too or provide an optional list to restrict the
available planners for a concrete MoveItCpp instance.

That doesn't sound like it's better usable at all.

There is a lot of merit in having all planner configurations supported from
the MotionPlanning panel in rviz to try them out and thus they should all be
available in one sub-namespace, e.g., planners).

Agree, that's what I'm trying to do here ;)

Before this gets stale: I feel like I remember there was some discussion about the architecture choices in this PR, but these notes don't seem to mention it. Could you summarize if you remember and maybe update on what is left to be done?

Before this gets stale

This is stale, to my dismay. :( Henning, could you continue this work anytime soon to get this ready for noetic?

The results of the discussion in the meeting where to take the best of both worlds:

• There should be two separate fields for the Planner-ID and the PlannerManager. Everything else is somewhat messy for no reason.

• The interface should be restricted to have all planners in a fixed namespace on the parameter server, e.g. move_group/planners, to make sure they are in predictable places and to allow RViz/etc. to find all configurations automatically.

@henningkayser assuming you did not just break everything in unexpected ways: Thank you so much! 🎆
It is great to finally have support for, e.g., Pilz and OMPL running in one setup. We had that debate already long before Pilz wrote their planner, but there was clearly not enough incentive to see this through. Also it turned out to be a lot more work than I predicted.

I know a few people who might be annoyed by the config changes and broken messages, but it was worth it! 🥇

We should prepare a new release asap to minimize issue reports like moveit/moveit_tutorials#613. @tylerjw, what's your plan about this?