Stage::pointer vs. Stage::reference

[rhaschke]

We need to distinguish owned stage instances from referenced, not-owned ones. See https://github.com/ros-planning/moveit_task_constructor/issues/139#issuecomment-592389785.

[v4hn]

Just to clarify, are you talking about C++ references?

At the moment we use raw pointers to act as the kind of reference/handles I have in mind, which is obviously not ideal. Using C++ references right away will not work, as they do not allow for MonitoringGenerator::setMonitoredStage. References have to be bound on construction.

Possible approaches from my point of view would be:

• Go for shared_ptrs (as discussed before) in order to use weak_ptr as references
• Implement a Handler system, where a Task can return a unique id for lookup for each transitively owned stage
• Wrap references in allocatable classes for storage, similar to this:

  
  struct Stage {};

struct StageHandle { StageHandle(Stage& _s) : s(_s) {}; Stage& s; };

struct Monitor { Monitor(Stage& s) : monitored_stage(new StageHandle(s)) {} void setMonitoredStage(Stage& s){ monitored_stage.reset(new StageHandle(s)); }

std::unique_ptr monitored_stage; };

The first and second option could give guarantees on lifetime, the third one would be a blunt hack to avoid the explicit raw_pointer...
What would work for bindings with python and what would you prefer?

At the moment I think dropping the `unique_ptr` altogether seems the best option...

[rhaschke]

What I had in mind was your 3rd option. Using shared_ptr is an easy approach, but it doesn't reflect the semantics of stage pointers: they cannot be shared across several containers. Also, we should forbid access to the stages after planning has started. For example, resetting individual stages or modifying their properties when planning has already started, will result in hard-to-find bugs.

[v4hn]

Using shared_ptr is an easy approach, but it doesn't reflect the semantics of stage pointers: they cannot be shared across several containers

These semantics are already reflected in the unique parent_ pointer of each stage, so I do not see the need to enforce them in the pointer structure. It would make sense to restrict the number of calls to StagePrivate::setHierarchy to one though and throw an exception if a stage already has a parent.

Also, we should forbid access to the stages after planning has started. For example, resetting individual stages or modifying their properties when planning has already started, will result in hard-to-find bugs.

I get your reasoning, but you can't really forbid access as users can always take raw pointers anyway. If you want to block access to parameters, this should probably be done by adding a status field to the stages themselves, setting them to a PLANNING mode when the task starts planning. But, even that is fragile unless you pass this on to the PropertyMaps... Enforcing this on the side of the framework seems to be more of a hassle than it's worth from my perspective. Lastly, I might even see merit in accessing stages during the planning process, e.g. adapting them to already detected solutions...

[zflat]

What I had in mind was your 3rd option. Using shared_ptr is an easy approach, but it doesn't reflect the semantics of stage pointers: they cannot be shared across several containers. Also, we should forbid access to the stages after planning has started. For example, resetting individual stages or modifying their properties when planning has already started, will result in hard-to-find bugs.

I'm not sure if trying to "forbid access" is a realistic design goal given the way containers are built up (which I like a lot and think it gives a lot of flexibility). It would make more sense to make methods that mutate stages perform their own checking to determine what is allowed and when.

Even the ContainerBase::findChild gives access to stages right? I see that returning a raw pointer indicates the intent that ownership of the child is solely the responsibility of the container. However, it brings the disadvantages of using raw pointers.

[zflat]

If stages cannot be shared across several containers, it sound like we have a hierarchical structure. I think that can be enforced by ensuring each stage has no more than one parent. When attempting to add a stage to a container we can check if the stage already has a parent and either remove from the existing parent or fail to add to the new parent.

[rhaschke]

I think that can be enforced by ensuring each stage has no more than one parent.

We already enforce this: https://github.com/ros-planning/moveit_task_constructor/blob/39427f89bfb77ae55cf6ae95632bac841e75f1a8/core/src/container.cpp#L185-L189