Initial implementation of kinematic World #243

brianhou · 2017-10-18T22:48:10Z

This is a first attempt at a kinematic version of dart::simulation::World. Using a DART World as our default environment representation forces users to instantiate a dynamics engine, which is unnecessary overhead for many of our tasks. We'll add more functionality in future PRs.

Before creating a pull request

Document new methods and classes
Format code with make format

Before merging a pull request

Set version target by selecting a milestone on the right side
Summarize this change in CHANGELOG.md
Add unit test(s) for this change

jslee02 · 2017-10-18T22:53:46Z

src/planner/World.cpp

+}
+
+//==============================================================================
+const std::string& World::setName(const std::string& newName)


I don't think we need to return the name. The reason we did this in DART was that the given name and the actual name can be different to manage all the names to be unique. If the given name was taken then the actual name would be the given name + "(1)" (the number will increase until the name becomes unique). However, I don't think we want the names of the kinematic world to be unique.

jslee02 · 2017-10-18T22:55:27Z

src/planner/World.cpp

+dart::dynamics::SkeletonPtr World::getSkeleton(const std::string& name) const
+{
+  for (const auto& skeleton : mSkeletons)
+    if (skeleton->getName() == name)


Nit: The convention is using curly parentheses if the for-state (or other control statements) is multiple-line.

jslee02 · 2017-10-18T22:57:37Z

src/planner/World.cpp

+//==============================================================================
+std::string World::addSkeleton(const dart::dynamics::SkeletonPtr& skeleton)
+{
+  std::lock_guard<std::mutex> lock(mMutex);


Nit: It also looks safe even we lock after the below nullity check because it refers only the parameter.

jslee02 · 2017-10-18T23:03:54Z

include/aikido/planner/World.hpp

+  virtual ~World() = default;
+
+  /// Create a clone of this World. All Skeletons will be copied over.
+  std::shared_ptr<World> clone(const std::string& name) const;


Nit: Need docstring for the parameter.

Also, I like you used newName in the source file for this parameter. It would be good to use the same name here too.

jslee02 · 2017-10-18T23:05:31Z

src/planner/World.cpp

+//==============================================================================
+void World::removeSkeleton(const dart::dynamics::SkeletonPtr& skeleton)
+{
+  std::lock_guard<std::mutex> lock(mMutex);


Nit: It also looks safe even we lock after the below nullity check because it refers only the parameter.

codecov · 2017-10-18T23:24:23Z

Codecov Report

Merging #243 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #243   +/-   ##
=======================================
  Coverage   70.78%   70.78%           
=======================================
  Files         172      172           
  Lines        5055     5055           
  Branches      802      802           
=======================================
  Hits         3578     3578           
  Misses        993      993           
  Partials      484      484

prasbg

Nice, this seems like a straightforward little class.

prasbg · 2017-10-19T19:14:37Z

include/aikido/planner/World.hpp

+{
+public:
+  /// Construct a kinematic World.
+  World(const std::string& name);


If we are creating or cloning worlds inside of an optimizer or something, does it really make sense to need to generate names for all of them? I think DART has some mechanisms for autogenerating IDs if names are not set, something like that might make sense here if this is just for semantic purposes.

prasbg · 2017-10-19T19:22:46Z

tests/planner/test_World.cpp

+    , skel2{dart::dynamics::Skeleton::create("skel2")}
+    , skel3{dart::dynamics::Skeleton::create("skel3")}
+  {
+    mWorld = std::make_shared<aikido::planner::World>("test");


Should World have a similar create() helper to the above one that Skeleton has?

psigen · 2017-10-19T19:27:13Z

Oops, posted PR review with wrong account 🤷‍♀️

jslee02

I think the create() should return std::unique_ptr for the reason mentioned below. Other than that, it looks good to go!

jslee02 · 2017-10-19T23:52:56Z

include/aikido/planner/World.hpp

+  mutable std::mutex mMutex;
+
+  /// NameManager for keeping track of Worlds
+  static dart::common::NameManager<World*> worldNameManager;


I believe our convention is mStaticVariable even for static member variables.

jslee02 · 2017-10-19T23:53:50Z

include/aikido/planner/World.hpp

+  dart::common::NameManager<dart::dynamics::SkeletonPtr> mSkeletonNameManager;
+};
+
+typedef std::shared_ptr<World> WorldPtr;


Nit: Let's use C++11 style of type aliasing: using WorldPtr = std::shared_ptr<World>().

jslee02 · 2017-10-19T23:56:44Z

include/aikido/planner/World.hpp

+  std::shared_ptr<World> clone(const std::string& newName = "") const;
+
+  /// Create a new World inside of a shared_ptr
+  static std::shared_ptr<World> create(const std::string& name = "");


In case we provide one creator function, then it should be returning std::unique_ptr because we can convert std::unique_ptr to std::shared_ptr but the opposite is not allowed.

Sure. I mostly copied this from DART 😅

Yeah, DART also needs to be tweaked. 😞 There is (or was) ongoing discussion: dartsim/dart#845

Implement aikido kinematic World for planning.

a63f910

brianhou added this to the Aikido 0.2.0 milestone Oct 18, 2017

brianhou requested a review from jslee02 October 18, 2017 22:48

jslee02 suggested changes Oct 18, 2017

View reviewed changes

Address Jeongseok's comments.

79f6f9e

brianhou added 3 commits October 18, 2017 16:26

Change return type of World::setName.

f360927

Add tests for aikido::planner::World.

51843ba

Updated CHANGELOG.md.

346ee26

brianhou requested a review from jslee02 October 19, 2017 17:52

prasbg reviewed Oct 19, 2017

View reviewed changes

brianhou added 3 commits October 19, 2017 13:02

Add World::create convenience method.

d99016e

Use DART NameManager to automatically generate World IDs.

67749bb

Clean up documentation.

a7be41e

jslee02 approved these changes Oct 19, 2017

View reviewed changes

Address Jeongseok's comments.

b7c14c1

jslee02 approved these changes Oct 20, 2017

View reviewed changes

Merge branch 'master' into enhancement/brianhou/world

b639ea6

brianhou merged commit 677c719 into master Oct 20, 2017

brianhou deleted the enhancement/brianhou/world branch October 24, 2017 01:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation of kinematic World #243

Initial implementation of kinematic World #243

brianhou commented Oct 18, 2017 •

edited

Loading

jslee02 Oct 18, 2017

jslee02 Oct 18, 2017

jslee02 Oct 18, 2017

jslee02 Oct 18, 2017

jslee02 Oct 18, 2017

codecov bot commented Oct 18, 2017 •

edited

Loading

prasbg left a comment

prasbg Oct 19, 2017

prasbg Oct 19, 2017

psigen commented Oct 19, 2017 •

edited

Loading

jslee02 left a comment •

edited

Loading

jslee02 Oct 19, 2017

jslee02 Oct 19, 2017

jslee02 Oct 19, 2017 •

edited

Loading

brianhou Oct 19, 2017

jslee02 Oct 20, 2017

Initial implementation of kinematic World #243

Initial implementation of kinematic World #243

Conversation

brianhou commented Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 18, 2017 • edited Loading

Codecov Report

prasbg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psigen commented Oct 19, 2017 • edited Loading

jslee02 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jslee02 Oct 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianhou commented Oct 18, 2017 •

edited

Loading

codecov bot commented Oct 18, 2017 •

edited

Loading

psigen commented Oct 19, 2017 •

edited

Loading

jslee02 left a comment •

edited

Loading

jslee02 Oct 19, 2017 •

edited

Loading