Extend createShapeNode to accept more types of arguments

[jslee02]

GCC 7.2.0/Clang Apple LLVM 9.0.0 fail to build createShapeNode() with errors as follows:

BodyNode.hpp:149:16: error: ‘class std::shared_ptr<dart::dynamics::SphereShape>’ has no member named ‘mName’
     properties.mName = getName()+"_ShapeNode_"

Travis also reports the same error.

[jslee02]

@mxgrey Could you take a look at this when you have a chance?

[mxgrey]

First, notice that if you change line 43 to:

dart::dynamics::ShapePtr sphere = std::make_shared<dart::dynamics::SphereShape>(1.0);

then it builds just fine.

Essentially, what's happening is the C++ overload inference rules are working against us in this scenario. Before the compiler attempts to implicitly upcast the SphereShapePtr to a ShapePtr, it attempts to pass it into the templated version of the function. The templated version of the function expects a Properties object, so it naturally fails to compile when given a ShapePtr object.

This kind of issue would be solved by C++ "Concepts", but unfortunately that won't be available until C++20 at the earliest.

I'm creating a fix for this right now. It simply involves creating an overload of the template that's specialized for std::shared_ptr<T>. It will explicitly cast the input to a ShapePtr and pass it along to the correct overload.

I'm in the process of testing the fix right now. Would you like me to push it to this branch when it's finished?

[jslee02]

Thanks for the explanation. Yeah, I remember I had the same issue with template function before. C++ is always more complex then I expected. 😱

I'm in the process of testing the fix right now. Would you like me to push it to this branch when it's finished?

Sure, feel free to push to this branch.

[mxgrey]

I've pushed the fix, as well as some additional tests.

[jslee02]

Thanks for the quick fix! I left a question below.

[jslee02]

Could we remove the non-template function by having the implementation here? I think this template function can handle the case as well. 🤔 If this works, we could apply this to the below function as well.

[mxgrey]

Good point. I'll try that out right now and then push it if it works (which I fully expect it will).

[mxgrey]

The change worked, so I've pushed it.

The one caveat with this is it breaks ABI compatibility, so anyone who was using that function will need to recompile. We don't support ABI compatibility between minor versions, so this doesn't violate our guidelines, but it's something to watch out for.

[mxgrey]

On the bright side, we've reduced three functions to two while also expanding their usability.

[jslee02]

Yeah, I like this change. Btw, I recall that the same issue was of createShapeNode(shape, name). I couldn't pass const char* for the name because of the template function. Now everything is resolved in a neat way!

[jslee02]

@mxgrey This PR looks good to go. Feel free to update changelog (and the PR title) and merge.

[codecov]

Codecov Report

Merging #986 into release-6.4 will increase coverage by 0.02%.
The diff coverage is 100%.

@@              Coverage Diff               @@
##           release-6.4    #986      +/-   ##
==============================================
+ Coverage        56.67%   56.7%   +0.02%     
==============================================
  Files              310     310              
  Lines            23960   23958       -2     
==============================================
+ Hits             13580   13585       +5     
+ Misses           10380   10373       -7

Impacted Files	Coverage Δ
dart/dynamics/BodyNode.hpp	100% <ø> (ø)	⬆️
dart/dynamics/BodyNode.cpp	75.44% <ø> (+0.33%)	⬆️
dart/dynamics/detail/BodyNode.hpp	97.61% <100%> (+1.61%)	⬆️

[mxgrey]

Are we concerned about the alleged Travis failures? It seems to be complaining about failing to install nlopt.

[jslee02]

Let's ignore build tests on macOS. I genuinely don't know how to resolve it. 😞