Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend createShapeNode to accept more types of arguments #986

Merged
merged 5 commits into from
Feb 15, 2018
Merged

Extend createShapeNode to accept more types of arguments #986

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a4b3b67
Add a test fails with createShapeNode w/o aspect
jslee02 Feb 14, 2018
b5f8ff9
Fix issue and add a few more tests
mxgrey Feb 14, 2018
695fa25
Rename test to match the PR number
mxgrey Feb 14, 2018
05b3d2e
Move implementations to templated functions
mxgrey Feb 14, 2018
f5de86a
Update CHANGELOG.md
mxgrey Feb 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions dart/dynamics/BodyNode.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -899,12 +899,6 @@ ShapeNode* BodyNode::createShapeNode(const ShapePtr& shape,
return createShapeNode(properties, false);
}

//==============================================================================
ShapeNode* BodyNode::createShapeNode(const ShapePtr& shape, const char* name)
{
return createShapeNode(shape, std::string(name));
}

//==============================================================================
const std::vector<ShapeNode*> BodyNode::getShapeNodes()
{
Expand Down
15 changes: 12 additions & 3 deletions dart/dynamics/BodyNode.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -546,12 +546,21 @@ class BodyNode :
/// <BodyNodeName>_ShapeNode_<#>.
ShapeNode* createShapeNode(const ShapePtr& shape);

/// Create an ShapeNode with the specified name
/// Same as createShapeNode(const ShapePtr&), but this accepts pointers to
/// arbitrary Shape types.
template <class ShapeType>
ShapeNode* createShapeNode(const std::shared_ptr<ShapeType>& shape);

/// Create a ShapeNode with the specified name
ShapeNode* createShapeNode(
const ShapePtr& shape, const std::string& name);

/// Create an ShapeNode with the specified name
ShapeNode* createShapeNode(const ShapePtr& shape, const char* name);
/// Same as createShapeNode(const ShapePtr&, const std::string&), but this
/// accepts pointers to arbitrary Shape types, and can accept string literals.
template <class ShapeType, class StringType>
ShapeNode* createShapeNode(
const std::shared_ptr<ShapeType>& shape,
const StringType& name);

/// Return the list of ShapeNodes
const std::vector<ShapeNode*> getShapeNodes();
Expand Down
17 changes: 17 additions & 0 deletions dart/dynamics/detail/BodyNode.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,23 @@ ShapeNode* BodyNode::createShapeNode(ShapeNodeProperties properties,
return createNode<ShapeNode>(properties);
}

//==============================================================================
template <class ShapeType>
ShapeNode* BodyNode::createShapeNode(const std::shared_ptr<ShapeType>& shape)
{
return createShapeNode(static_cast<ShapePtr>(shape));
Copy link
Member Author

@jslee02 jslee02 Feb 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@jslee02 jslee02 Feb 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

}

//==============================================================================
template <class ShapeType, class StringType>
ShapeNode* BodyNode::createShapeNode(
const std::shared_ptr<ShapeType>& shape,
const StringType& name)
{
return createShapeNode(static_cast<ShapePtr>(shape),
static_cast<std::string>(name));
}

//==============================================================================
template <class... Aspects>
ShapeNode* BodyNode::createShapeNodeWith(const ShapePtr& shape)
Expand Down
2 changes: 1 addition & 1 deletion unittests/regression/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,6 @@ if(TARGET dart-utils-urdf)

dart_add_test("regression" test_Issue895)

dart_add_test("regression" test_IssueXXX)
dart_add_test("regression" test_Issue986)

endif()
15 changes: 13 additions & 2 deletions unittests/regression/test_IssueXXX.cpp → unittests/regression/test_Issue986.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,23 @@
#include <dart/utils/urdf/DartLoader.hpp>

//==============================================================================
TEST(IssueXXX, CreateShapeNodeShouldCompile)
TEST(Issue986, CreateShapeNodeShouldCompile)
{
const auto skel = dart::dynamics::Skeleton::create();
auto* bn = skel->createJointAndBodyNodePair<FreeJoint>().second;
auto sphere = std::make_shared<dart::dynamics::SphereShape>(1.0);
const auto sphere = std::make_shared<dart::dynamics::SphereShape>(1.0);

bn->createShapeNode(sphere);
bn->createShapeNode(sphere, "custom name");

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

bn->createShapeNode(generic);
bn->createShapeNode(generic, "another name");

bn->createShapeNode(sphere, std::string("passing a string"));
bn->createShapeNode(generic, std::string("passing another string"));

auto world = dart::simulation::World::create();
world->addSkeleton(skel);
Expand Down