Add BuildDynamicWalker C++ example. #2073

chrisdembia · 2018-02-05T06:08:53Z

This PR is part of #1775.

Brief summary of changes

Add the source code for the "From the Ground Up" example to the GitHub repository, so we can easily update it.
Updated the example code for 4.0.
The source code is tested but is not installed, to avoid giving students "the answer."

Testing I've completed

Built and ran the code and ensured the resulting model was correct.

Looking for feedback on...

Placement of files in the repository. I tried to mimic the existing structure of test and example folders for consistency, even though the split across directories is not ideal.

CHANGELOG.md (choose one)

updated.

Doing so causes test to time out.

chrisdembia · 2018-02-06T22:33:20Z

The travis-ci/push failure can be ignored.

…ld_dynamic_walker

chrisdembia · 2018-02-15T19:21:32Z

Ready for review.

aseth1

One suggestion is to use a PlanarJoint to remove the clutter of accessing and defining Coordinates that are essentially unused by the example.

aseth1 · 2018-02-28T20:20:32Z

OpenSim/Examples/BuildDynamicWalker/BuildDynamicWalkerModel.cpp

+        orientationInParent = Vec3(0.0, 0.0, 0.0);
+        locationInChild = Vec3(0.0, 0.0, 0.0);
+        orientationInChild = Vec3(0.0, 0.0, 0.0);
+        FreeJoint *pelvisToPlatform = new FreeJoint("PelvisToPlatform",


In interest of efficiency and also showcasing other joint types, I would advocate using a PlanarJoint especially since 4 of the 6dofs of the FreeJoint are locked.

chrisdembia · 2018-02-28T22:24:11Z

Thanks @aseth1 ; I converted the FreeJoint to a PlanarJoint. Ready for review again.

chrisdembia · 2018-03-01T22:33:24Z

@aseth1 I think the travis-ci/pr failure can be ignored. All the tests pass; the ❌ is because Travis couldn't upload doxygen to myosin.

aseth1

LGTM. A very minor comment. Fine to merge as is.

aseth1 · 2018-03-02T04:11:52Z

OpenSim/Examples/BuildDynamicWalker/BuildDynamicWalkerModel.cpp

+        orientationInChild = Vec3(0.0, 0.0, 0.0);
+        PlanarJoint *pelvisToPlatform = new PlanarJoint("PelvisToPlatform",
+            *platform, locationInParent, orientationInParent,
+            *pelvis, locationInChild, orientationInChild);


In the interest of streamlining and avoiding the creation of empty transforms, you could use this Joint constructor instead:

Joint(const std::string& name, const PhysicalFrame& parent, const PhysicalFrame& child);

I think it would look a lot cleaner.

chrisdembia · 2018-03-06T00:36:27Z

This PR is ready for review again.

I used the shorter joint constructor in one instance, but still left another "unnecessary" usage of the longer constructor for illustrative purposes (see the Confluence page).
I added Hannah O'Day's updates to CreateWalkingModelAndEnvironment.m, with some of my own changes.

aseth1

Minor comments regarding naming of Coordinate variables. Happy to see it get merged either way.

aseth1 · 2018-03-06T04:39:22Z

Bindings/Java/Matlab/Dynamic_Walker_Example/CreateWalkingModelAndEnvironment.m

-jointCoordinateSet.get(0).setRange([0, 5]);
-jointCoordinateSet.get(0).setName('Pelvis_ty');
-jointCoordinateSet.get(0).setDefaultValue(1.0);
+Pelvis_rz = pelvisToPlatform.upd_coordinates(0);


It seems a bit inconsistent to start using variables with capital first letter.

I don't like the use of capital first letters for variables, but I did it to be consistent with the rest of the file and to match the string name of the coordinates (which I prefer to be all lower-case).

That's fine. I'm happy with the PR.

aseth1 · 2018-03-06T04:42:36Z

OpenSim/Examples/BuildDynamicWalker/BuildDynamicWalkerModel.cpp

+        Vec3 orientationInParent(0.0, 0.0, 0.0);
+        Vec3 locationInChild(0.0, 0.0, 0.0);
+        Vec3 orientationInChild(0.0, 0.0, 0.0);
+        PinJoint *platformToGround = new PinJoint("PlatformToGround",


The MATLAB version uses a WeldJoint with the child joint frame having the 10degrees of incline. Any reason to make the incline a DOF and then constrain it?

That's how the example was written. Using a DOF makes it easy to run simulations in the GUI with different inclines. Changing this would also require rewriting a good portion of the Confluence tutorial; this pin joint is used to explain coordinates, etc.

Makes sense to leave it then.

tkuchida

Minor suggestions only 🐸

tkuchida · 2018-03-06T08:30:33Z

Bindings/Java/Matlab/Dynamic_Walker_Example/CreateWalkingModelAndEnvironment.m

@@ -26,7 +27,7 @@
 % -----------------------------------------------------------------------

 % User Section - Adjust these parameters at will
-outputPath = '..\Model\';
+outputPath = './'; % TODO ./Model/';


Not sure whether this "TODO" was inserted as a note to yourself or intended for the example. Just checking.

tkuchida · 2018-03-06T08:38:43Z

OpenSim/Examples/BuildDynamicWalker/BuildDynamicWalkerModel.cpp

+#include <OpenSim/OpenSim.h>
+// "Use" the OpenSim and SimTK namespace to shorten type names.
+// Note: Several classes appear in both namespaces and require using the
+// fully-qualified class name (for example, OpenSim::Body).


This comment should be updated to explain why you avoided using namespace SimTK; on L7–10. Note that Body is not fully qualified on L43.

tkuchida · 2018-03-06T08:42:48Z

OpenSim/Examples/BuildDynamicWalker/BuildDynamicWalkerModel.cpp

+        // Get a reference to the ground object
+        const Ground& ground = osimModel.getGround();
+
+        // Define the acceleration of gravity


"of" -> "due to" (gravity itself doesn't accelerate)

tkuchida · 2018-03-06T08:46:17Z

OpenSim/Examples/BuildDynamicWalker/BuildDynamicWalkerModel.cpp

+        PlanarJoint *pelvisToPlatform = new PlanarJoint("PelvisToPlatform",
+            *platform, *pelvis);
+
+        // A planar joint has three coordinates, in the following order:


Perhaps an outdated comment? The order of the Coordinates is irrelevant because you're accessing them using enums rather than integers.

tkuchida · 2018-03-06T08:52:10Z

OpenSim/Examples/BuildDynamicWalker/BuildDynamicWalkerModel.cpp

+        return 1;
+    }
+    std::cout << "OpenSim example completed successfully" << std::endl;
+    // std::cout << "Press return to continue" << std::endl;


Intentionally left commented out?

Yes, so that the tests don't time-out.

What are we assuming about the knowledge of the target audience? Possible thoughts:

The code was commented out because it doesn't work.

Uncommenting the code causes something to break.

The code was commented out and should have been removed.

Regardless, I recommend leaving it as it is.

This bit of code is useful when running command-line examples on Windows so that the command window doesn't close on you abruptly, so I left it in. In other places, we've added a way to detect if the code is being run as part of a test via an environment variable, but I decided not to spend time on that this time. I agree with your final suggestion.

tkuchida · 2018-03-06T08:58:31Z

OpenSim/Examples/BuildDynamicWalker/CMakeLists.txt

+        CACHE PATH "Top-level directory of OpenSim install.")
+# This command searches for the file OpenSimConfig.cmake
+# in common system directories and in OPENSIM_INSTALL_DIR.
+find_package(OpenSim 4.0 REQUIRED PATHS "${OPENSIM_INSTALL_DIR}")


Might add a note stating that OpenSim versions < 4.0 will still work, but the user will simply need to provide the package location manually when they run CMake. Otherwise, the reader is left wondering whether L2 is incorrect (and might incorrectly attribute a build error to the apparent dissonance between L2 and L14).

L2 specifies the minimum CMake version; this line specifies the minimum OpenSim version. I'll clarify

Ah, right. Unfortunate coincidence; I read the comment, ending with "OpenSim", then read the following line of code that says "VERSION 3.2". Perhaps just remove the word "OpenSim" from the comment on L1.

tkuchida · 2018-03-06T09:04:35Z

OpenSim/Examples/CMakeLists.txt

@@ -34,4 +34,9 @@ elseif()

 endif()

+# We do not install the BuildDynamicWalker example, because it is an assignment
+# in the OpenSim class. The solution to the BuildDynamicWalker assignment is in


"assignment in the OpenSim class" – I assume you mean ME/BIOE 485, but "assignment" and "class" are ambiguous here. I would replace with something like "…because it is assigned to students as homework."

chrisdembia

Thanks @tkuchida for catching those items. I'll push an update.

chrisdembia · 2018-03-06T16:51:23Z

OpenSim/Examples/BuildDynamicWalker/CMakeLists.txt

+        CACHE PATH "Top-level directory of OpenSim install.")
+# This command searches for the file OpenSimConfig.cmake
+# in common system directories and in OPENSIM_INSTALL_DIR.
+find_package(OpenSim 4.0 REQUIRED PATHS "${OPENSIM_INSTALL_DIR}")


L2 specifies the minimum CMake version; this line specifies the minimum OpenSim version. I'll clarify

chrisdembia · 2018-03-07T01:24:26Z

@tkuchida I've addressed your comments.

tkuchida

👍 🦎

chrisdembia · 2018-03-07T02:41:07Z

Thanks @tkuchida

tkuchida · 2018-03-07T03:19:32Z

Travis failures are unrelated; merging.

chrisdembia · 2018-03-07T03:21:48Z

Thanks @tkuchida

chrisdembia added 4 commits February 4, 2018 21:44

Add BuildDynamicWalker example.

ce5c449

Edit CHANGELOG, uncomment user input.

e485fcf

Do not wait for user input.

ab5c4c1

Doing so causes test to time out.

Tabs -> spaces.

14088e7

Merge branch 'master' of github.com:opensim-org/opensim-core into bui…

48d00b6

…ld_dynamic_walker

aseth1 requested changes Feb 28, 2018

View reviewed changes

FreeJoint -> PlanarJoint.

83dfc39

aseth1 approved these changes Mar 2, 2018

View reviewed changes

aseth1 mentioned this pull request Mar 4, 2018

Add Walker Models opensim-org/opensim-models#34

Merged

chrisdembia mentioned this pull request Mar 5, 2018

Update CMakeLists.txt for building dynamic walker example. opensim-org/opensim-models#9

Closed

chrisdembia added 6 commits March 5, 2018 15:14

Use shorter Joint constructor.

242a19d

Commit Hannah O'Day's update to CreateWalkingModelAndEnvironment.m.

c7c7fc3

Use shorter constructor for PlanarJoint.

1b018b6

Change coordinate variable names.

37a5b7b

Restore outputDir.

b6cf82a

Use PlatformOffset for visualizing the platform.

af4f56c

aseth1 approved these changes Mar 6, 2018

View reviewed changes

tkuchida reviewed Mar 6, 2018

View reviewed changes

chrisdembia commented Mar 6, 2018

View reviewed changes

Address tkuchida's comments.

2775c72

tkuchida approved these changes Mar 7, 2018

View reviewed changes

tkuchida merged commit 3d85133 into master Mar 7, 2018

tkuchida deleted the build_dynamic_walker branch March 7, 2018 03:19

chrisdembia mentioned this pull request Mar 8, 2018

Loading the WalkerModel.osim causes an error related to MobilizedBodyIndex opensim-org/opensim-gui#72

Closed

Add BuildDynamicWalker C++ example. #2073

Add BuildDynamicWalker C++ example. #2073

Conversation

chrisdembia commented Feb 5, 2018

Brief summary of changes

Testing I've completed

Looking for feedback on...

CHANGELOG.md (choose one)

chrisdembia commented Feb 6, 2018

chrisdembia commented Feb 15, 2018

aseth1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisdembia commented Feb 28, 2018

chrisdembia commented Mar 1, 2018

aseth1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisdembia commented Mar 6, 2018

aseth1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisdembia Mar 6, 2018 • 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

tkuchida left a 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

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

Choose a reason for hiding this comment

chrisdembia Mar 7, 2018 • 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

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

chrisdembia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisdembia commented Mar 7, 2018

tkuchida left a comment

Choose a reason for hiding this comment

chrisdembia commented Mar 7, 2018

tkuchida commented Mar 7, 2018

chrisdembia commented Mar 7, 2018

chrisdembia Mar 6, 2018 •

edited

Loading

chrisdembia Mar 7, 2018 •

edited

Loading