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.

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

Recorder xml #2

Open
wants to merge 34 commits into
base: AD-recorder-xml
Choose a base branch
from

Conversation

gilserrancoli
Copy link

Those are the changes to enable the reading of .osim files in the AD version of OpenSim. The main changes consist of enabling readFromXML... functions.
I added a test to read arm26_noMuscles .osim and gait2354_simbody osim file with no muscles and no Yamaguchi knee functions (gait2354_simbody_noMuscles_nokneeYamag.osim).
Pendulum_2dof_osim External function has also been created to read the model directly from the .osim file and prepared to run an OCP.

Copy link
Owner

@antoinefalisse antoinefalisse left a comment

Choose a reason for hiding this comment

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

@gilserrancoli, I had a first look. I wonder whether it is necessary to uncomment everything. In the end we do not support certain things with AD, such as the elastic foundation model. So I am not sure we should make that available.

target_link_libraries(${TEST_TARGET} osimSimulation)
set_target_properties(${TEST_TARGET} PROPERTIES
FOLDER "External_Functions")
OpenSimCopySharedTestFiles(Pendulum2dof.osim)
Copy link
Owner

Choose a reason for hiding this comment

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

Can you add an empty line in the end?

@@ -0,0 +1,147 @@
/* This code describes the OpenSim model and the skeleton dynamics
Copy link
Owner

Choose a reason for hiding this comment

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

Can you adjust the tabs to make it look nice? Thanks.

//
//} // end of namespace OpenSim
/* -------------------------------------------------------------------------- *
* OpenSim: ElasticFoundationForce.cpp *
Copy link
Owner

Choose a reason for hiding this comment

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

Do we support that with AD? I don't think so...I don't think we should uncomment if we do not support...

@@ -25,6 +25,7 @@
// INCLUDES
//=============================================================================
#include "Body.h"
#include <OpenSim/Common/XMLDocument.h>
Copy link
Owner

Choose a reason for hiding this comment

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

Strange that we need to add those

@@ -30,7 +30,7 @@
#include <OpenSim/Common/Constant.h>
#include <OpenSim/Common/LinearFunction.h>
#include "simbody/internal/MobilizedBody_FunctionBased.h"

#include <OpenSim/Common/XMLDocument.h>
Copy link
Owner

Choose a reason for hiding this comment

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

Idem

@@ -19,5 +19,6 @@ if(BUILD_TESTING)
add_subdirectory(ExampleLuxoMuscle)
add_subdirectory(AnalysisPluginExample)
add_subdirectory(BodyDragExample)
add_subdirectory(testReadOsimFile)
Copy link
Owner

Choose a reason for hiding this comment

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

I would include Recorder or AD as part of the test name

@@ -0,0 +1,1045 @@
<?xml version="1.0" encoding="UTF-8" ?>
Copy link
Owner

Choose a reason for hiding this comment

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

Where does this model come from?

@@ -0,0 +1,53 @@

Copy link
Owner

Choose a reason for hiding this comment

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

Can you clean this? There are some unnecessary comments.

@@ -0,0 +1,100 @@
/* -------------------------------------------------------------------------- *
Copy link
Owner

Choose a reason for hiding this comment

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

Let's work on the test in the end. I think we should test more features.

ARCHIVE DESTINATION lib
RUNTIME DESTINATION bin
)

Copy link
Owner

Choose a reason for hiding this comment

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

Seems to be an empty line in the end

antoinefalisse pushed a commit that referenced this pull request Jun 23, 2020
Avoid issue with order of tibial faces (columns in pairs) and change pressure calculation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants