Add opw_kinematics as a git submodule

[JeroenDM]

This way we do not depend on the release of that package.

• Added opw_kinematics as a submodule in the external directory.
• Removed opw_kinematics from the rosinstall file used by travis.
• Add the include path to CMakeLists, remove all other references to opw_kinematics that are not needed anymore.

Everything was indeed quite straightforward, except for one issue:

You would want to make sure to not export the include path from the IK plugin's package. Easiest way to do that would be to only include opw_kinematics headers in .cpp files. Never in public headers.

The current implementation requires at least the #include "opw_kinematics/opw_parameters.h" in the header file. But I'm not sure I completely understand the issue. The opw_kinematics headers are not exported to dependent packages as far as I understand:

catkin_package(
  INCLUDE_DIRS include
  LIBRARIES ${MOVEIT_LIB_NAME}
#  CATKIN_DEPENDS moveit_core moveit_ros_planning roscpp
#  DEPENDS system_lib
)

The header is required because the plugin has a member variable that contains the opw_parameters here. This variable had to be available in multiple methods of the plugin.

[simonschmeisser]

You could either turn that into a unique_ptr and forward declare the data type or use a PIMPL (a private class with the member variables which is forward declared). You need to implement the (empty) destructor in the cpp file for forward-declaration and unique_ptr to work together.

However, @gavanderhoorn , as this is meant to be used as a plugin, why do we care about exposing transient headers or not? (Admittedly we should then stop installing the headers?)

[gavanderhoorn]

However, @gavanderhoorn , as this is meant to be used as a plugin, why do we care about exposing transient headers or not? (Admittedly we should then stop installing the headers?)

There's two ways to go about this I believe:

1. support users who'd like to link against the library directly -> install headers
2. only allow "normal" use of the plugin (ie: as a plugin) -> no headers needed

Note that 2 does not prevent anyone from loading the plugin directly in their application, as long as they use pluginlib to do that. This would also not require any headers.

So my suggestion would be not install the headers, nor export the library in the catkin_package(..). You (ie: @JeroenDM) can still decide to do that later, if/when you get requests for it.

[gavanderhoorn]

The header is required because the plugin has a member variable that contains the opw_parameters here. This variable had to be available in multiple methods of the plugin.

Would it not work if you'd make that a "global variable" in the .cpp? If you want to be nice, you could use one of the smart ptr variants @simonschmeisser mentions, but it may not even be needed.

As long as it's only declared & defined in the .cpp, it will be invisible.

You could either turn that into a unique_ptr and forward declare the data type or use a PIMPL (a private class with the member variables which is forward declared)

That sounds like a lot of work for just some storage. I'm not convinced it even needs to be a member of the plugin class. As long as the class code has access to it.

[simonschmeisser]

That sounds like a lot of work for just some storage. I'm not convinced it even needs to be a member of the plugin class. As long as the class code has access to it.

I'm not 100% sure but wouldn't multiple instances of this class/plugin then operate on the same memory?

[gavanderhoorn]

That sounds like a lot of work for just some storage. I'm not convinced it even needs to be a member of the plugin class. As long as the class code has access to it.

I'm not 100% sure but wouldn't multiple instances of this class/plugin then operate on the same memory?

Yes, that would indeed be a problem. I had not considered multiple instances in the same process.

[gavanderhoorn]

If we don't install (edit: nor export in catkin_package(..)) the headers, having opw_kinematics appear in one of the plugin's headers wouldn't be as much of a problem if we did.

I've just tested this PR and it works for me.

Quite nicely actually, as I only needed to:

1. add a .yaml with the appropriate content to the robot support package
2. update the MoveIt config to load that .yaml (similar to how ros-planning/moveit#1997 would do this) and to use the OPW IK solver plugin

@JeroenDM: I've got a few minor cosmetic changes which I'll propose in a PR (see #51).

[JeroenDM]

I did not fully appreciate how the pluginlib worked, interesting!

Given that the headers are not necessary for normal use it seems only natural to avoid installing and exporting them. Then main reason it's in there is because other similar packages do it and I had no idea what I was doing when I first started creating this package :)

I will push I commit that reflects my understanding of the solution.

Edit: I still have this hunch that it is not unreasonable to use the plugin without pluginlib, but I guess in that case you download to source and link the headers manually...

[JeroenDM]

I also noticed the moveit_kinematics recently switched to

set(CMAKE_CXX_STANDARD 14)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
set(CMAKE_CXX_EXTENSIONS OFF)

to define the C++ standard. Replacing:

add_compile_options(-std=c++11)

Also, we should probably use C++ 14 in melodic-devel.

[gavanderhoorn]

@JeroenDM wrote:

Edit: I still have this hunch that it is not unreasonable to use the plugin without pluginlib, but I guess in that case you download to source and link the headers manually...

That's the first item in the list in https://github.com/JeroenDM/moveit_opw_kinematics_plugin/pull/48#issuecomment-623375483.

No, it's not completely unheard of to do this, but it is pretty rare.

As I wrote in that comment: if/when you/we get sufficient requests to allow this sort of usage, we could enable it.

---

Edit: and you don't necessarily need to use pluginlib to load the .so. If you're not interested in runtime discoverability nor platform-agnosticity, you could just use dlopen() (on Linux fi).

[gavanderhoorn]

I also noticed the moveit_kinematics recently switched to

set(CMAKE_CXX_STANDARD 14)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
set(CMAKE_CXX_EXTENSIONS OFF)

to define the C++ standard. Replacing:

add_compile_options(-std=c++11)

It's somewhat nicer, but seeing as this is a fairly stand-alone package, probably not a required change before doing the first release.

Also, we should probably use C++ 14 in melodic-devel.

ABI is compatible between 11 and 14, so unless you're using C++14 features here, or the kinematic base headers are, there doesn't seem to be a need per se.

[simonschmeisser]

I guess cmake has ways to translate this by now, but -std=c++ is not something that msvc understands. So maybe those Microsoft folks lurking around recently will like you for it? (not that I care that much ...)

[gavanderhoorn]

True, for platform compatibility using CMAKE_CXX_STANDARD would be nice.

[simonschmeisser]

I think you'll need to grant me more rights if you want my reviews to count :)

[JeroenDM]

Also, turns out I can't approve my own pull request... :) those settings are not ideal yet. This repository is as much my excuse to learn Github as it is a kinematics plugin.

I'll do the cross-platform stuff in a new pull request. If that change should make it compile on windows, I should consider testing if it actually works. (aka, find a college with that uses windows and visual studio :) )

[JeroenDM]

I changed the rule back again so only Travis is required for now.

Edit: @simonschmeisser and @gavanderhoorn I can still add you as a collaborator if you want more rights, but I'll leave it optional for now. I think it is more convenient if anyone can approve and I can just wait for approval before merging.

Also, I rebased this branch because a lot has changed in the main branch since the start of this pull request. Although I do not expect problems there.

Next steps, trying the prerelease tests again and writing documentation.

[simonschmeisser]

Protecting you from yourself is basically the point, so sure you cannot approve your own PRs ;)

I'm fine this way