Dave's PRs didn't touch it either (because I don't think it existed at the time), but I think the ('recently' added) LMA based plugin would also need to be updated?

This is great!

@jrgnicho can hopefully expand upon moveit_kinematic_tests

The kinematics_base_test repo was created to ensure that changes to the kinematics::KinematicsBase interface did't break existing implementation ( see my comments here ). It can be used to test the changes in this PR.

@gavanderhoorn I'll take a look at updating the LMA based plugin. That should definitely be included in this PR.

I've made some progress on this PR but still have some more work to do before it is ready to be reviewed again. The biggest problem remaining is that there is definitely a loop of shared pointers that keeps the RobotModel from ever being destroyed. The problem is:

• RobotModel contains JointModelGroup,
• JointModelGroup has the kinematics solver in the group_kinematics_ member variable
• The kinematics solver contains a shared_ptr to a RobotState
• RobotState contains a shared_ptr to the RobotModel

So the loop is RobotModel->JointModelGroup->KDLKinematicsPlugin->RobotState->RobotModel.

Any suggestions on how to fix this? It seems like one of these shared pointers needs to change to a weak pointer, but I'm not sure which.

Any suggestions on how to fix this? It seems like one of these shared pointers needs to change to a weak pointer, but I'm not sure which.

With the current design, I suppose KDLKinematicsPlugin->RobotState is the best candidate,
because it points upwards in MoveIt's hierarchy.

The reason @v4hn gives to motivate the KDLKinematicsPlugin->RobotState as a weak_ptr candidate is not clear to me. IMHO, we should look for weak couplings of instances. KDLKinematicsPlugin -> RobotState is the worst candidate: KDLKinematicsPlugin owns these RobotState pointers!

@jonbinney, there are many other cycles, e.g. RobotModel->JointModelGroup->RobotModel. In most cases, some "child class" stores a pointer back to its "parent class", here JointModelGroup stores a pointer to RobotModel in parent_model_. In this case this back pointer should be the weak one. Probably it could be even replaced by a proper reference: The pointer should never be invalid...

I think, the best way to break the loop in the above case, is to pass a new RobotModelSharedPtr to RobotState: As long as the KDLKinematicsPlugin is alive, its parent RobotModel will be so too. Hence, we could pass in the same pointer (.get()), but with a no-deleter.

The reason @v4hn gives to motivate the KDLKinematicsPlugin->RobotState as a weak_ptr candidate is not clear to me. IMHO, we should look for weak couplings of instances. KDLKinematicsPlugin -> RobotState is the worst candidate: KDLKinematicsPlugin owns these RobotState pointers!

ok, I didn't read through the dependencies in the recent past, so feel free to ignore my statement. 👶

ping @jonbinney any chance we could finish up this PR?

Ah yes, sorry! This fell off my radar. I'll start working on it again.

Having fun merging these changes with all the clang-format changes....

I haven't had any time to work on this for a while, and unfortunately I'm going to be very busy for the next few months as well. I'm going to close this for now. When/if I have time in the future I'll re-open it (or if anyone else has some time they're welcome to take it over). The changes in this PR are good, but time will need to be put in to make sure that they don't break existing behavior, since it affects a lot of code. Things that need to be resolved:

• merge/rebase to account for changes in kinetic-devel
• fix shared pointer cycles
• update some of the inverse kinematic plugins and create a guide to updating others
• lots of testing

I'd suggest to keep this open -- closed issue & PRs are harder to be found.

I agree with @130s, there's no need to close the request.
With an open request, the topic pops up every now and then.
Without it, especially new contributors will never notice that work in this direction already exists.

I'll reopen this request. @jonbinney if you don't want to have the open request on your name around or don't want to keep the branch on your repository I can transition the commits to my fork. But I don't have the time to work on this either.

Makes sense - I'm fine with keeping it open.

@v4hn you mentioned on Sunday that this PR has been discussed at ROScon, could you summarize the results? I'll try to look into this soonish and would like to be up to date beforehand

I might just take the changes here as an inspiration and do a new PR without any rebasing etc. My git skills are weak. Would that offend anyone?

@simonschmeisser thanks for taking this on! Yes, feel free to start fresh - no need to preserve the history here.

One thought - it would be good to have a shim set of functions that keep the current interface, construct the robot model from the URDF, and then call the new functions. We can mark them as deprecated so people have some warning and their custom IK plugins don't just stop working.

Just had a discussion with @v4hn about this at IPA at WorldMoveItDay:

It seems the best way to cut the shared_ptr dependency loop is to change the kinematics::KinematicsBasePtr solver_instance_ in JointModelGroup::KinematicsSolver JointModelGroup.h to a std::weak_ptr

I have a slight fear that I will not finish that within World MoveIt! Day (any time zone) but I will start a PR testing that implementation.

It seems the best way to cut the shared_ptr dependency loop is to change the kinematics::KinematicsBasePtr solver_instance_ in JointModelGroup::KinematicsSolver JointModelGroup.h to a std::weak_ptr

Can you please elaborate on this conclusion? I tend to agree, but it would be nice to understand (and document) your motivation as well ;-)

Please also get rid of JointModelGroup::solver_instance_const_. There is no need for this. A shared_ptr<T> can be casted into a shared_ptr<const T>.

Sure, sorry for not giving more details.

Your ( @rhaschke ) suggestion was to create a special std::shared_ptr<RobotModel, doNotDeleteMe>(robotModel.get()) and pass that to RobotState instances in the IK plugins. While this (probably) works, there would no direct way to stop people from using the "normal" RobotModel shared ptr and create that loop. We had some ideas for hacks/factory patterns etc but ...

On the other hand all kinematic plugin instances are stored in KinematicsPluginLoader/RobotModelLoader as shared_ptr already. So we can use weak_ptrs elsewhere, especially in JointModelGroup

Making the RobotModel pointer of RobotState weak would also have been possible but too invasive

re const, yes, thought that as well

Your suggestion was to create a special std::shared_ptr<RobotModel, doNotDeleteMe>(robotModel.get()) and pass that to RobotState instances in the IK plugins. ... There would be no direct way to stop people from using the "normal" RobotModelPtr and create that loop.

I don't understand what you mean by "normal" here. I looked into this issue again for last hour and found the following:

The RobotModelLoader (RML) owns its RobotModel (RM) as well as its KinematicsPluginLoader (KPL).
The JointModelGroup (JMG) stores the actual solver instance (KS), which is also cached in KPL.

RML => RM => JMG => KS
        \------<----/

Usually those caches are the proper choice for weak ptrs: As long as the pointer is still actively in use (outside the cache), the cached pointer can be reused. If all active instances are gone, the cache instance should be freed as well. However, in the present case, the cache only maintains unique instances - they are not shared. The only purpose of this logic is to not allocate KS twice in a row - once for validating that it is suitable for JMG here and once for actual use here. The important fact is, that not the KPL owns those instances, but the JMGs! Hence, the JMGs should store the shared_ptr. By clearing KPL's cache (and only the cache!) before RM in RML's destructor this can be handled. A better solution is to std::move the cached shared_ptr for return, but keeping the cache entry (which then has use_count = 0).

Regardless of this, the loop isn't broken, because the RM and thus JMG is not destroyed in first place, when the RML is unloaded. Thus, we need to ensure that the RML, in its destructor, actually triggers the destructor pipeline RM => JMG => KS. Hence, the only chance to break the loop is from KS back to RM.
(We cannot break the loop at arbitrary position, but we have to at (all?) back pointers.)

As far as I see, the old code doesn't provide an interface to directly pass a RobotModelPtr to KS.
KDLKinematicsPlugin, creates its own, new instance here, which we want to avoid with this PR. Thus, my suggestion is to break the loop at this new interface, not passing in a shared_ptr, but a reference. This states, that the fact that the pointer is weak. Passing in a weak_ptr, would not prevent KS authors to turn it into a proper shared_ptr and store it again. Instead, KS authors need to create their own "dummy" shared_ptr from this reference via std::shared_ptr<RobotModel, noDeleter>(&model). That's exactly the same conclusion I found in #150 (comment).

@simonschmeisser Sorry for the late reply. But it took me a while to understand all this. I suggest, I propose a solution along this line? (I still have all files open...)

I committed a solution to a similar issue in KinematicsPluginLoader here - #1104 (comment).
@simonschmeisser I didn't continued here, as I didn't get feedback from you. I guess you already rebased this PR to melodic-devel?

Please see my comments on #1153 and consider reopening it

#1166 and #1153 are more recent attempts at this, so I'll close this PR.