search

opensim-org / opensim-core

SimTK OpenSim C++ libraries and command-line applications, and Java/Python wrapping.
https://opensim.stanford.edu
Apache License 2.0
797 stars 320 forks source link

SoccerKickingModel fails to open #2024

Closed tkuchida closed 5 years ago

tkuchida commented 6 years ago

@aymanhab commented on Wed Nov 08 2017

Multiple warnings due to space in model name (likely common), then failure/abort open due to Error

screen shot 2017-11-08 at 9 58 18 am

@tkuchida commented on Wed Nov 08 2017

Possibly related to 1970 on opensim-core.

aseth1 commented 6 years ago

I am doubtful that the error is due to a space in a name. It is complaining about a an invalid index, which means it is connected to the Body but that Body is does not have a valid underlying (system) index yet. Could be an issue with the MultibodyTree that was generated. I'll take a look.

aseth1 commented 6 years ago

There appears to be an issue with the SimTK::MultibodyGraphMaker when the root body is massless. This is the resulting multibody graph for a model with zero mass for the root, swivel, Body connected to ground. Everything beyond the femur_r Body was removed for brevity.

MULTIBODY GRAPH
---------------

5 BODIES:
 0  0: /testMasslessRoot/ground mass=inf mob=-1 master=-1
  jointsAsParent=[ 0 3]   jointsAsChild=[]        slaves=[]
 1  1: /testMasslessRoot/swivel mass=0 mob=0 master=-1
  jointsAsParent=[ 1]     jointsAsChild=[ 0]      slaves=[]
 2  2: /testMasslessRoot/pelvis mass=10.4349 mob=1 master=-1
  jointsAsParent=[ 2]     jointsAsChild=[ 1]      slaves=[]
 3  1: /testMasslessRoot/femur_r mass=8.54927 mob=2 master=-1
  jointsAsParent=[]       jointsAsChild=[ 2 3]    slaves=[ 4]
 4  3: #/testMasslessRoot/femur_r_slave_1 mass=nan mob=3 master=3
  jointsAsParent=[]       jointsAsChild=[ 2]      slaves=[]

4 JOINTS:
 0  0: /testMasslessRoot/ground_to_swivel /testMasslessRoot/ground->/testMasslessRoot/swivel   PinJoint loopc=-1
 1  1: /testMasslessRoot/swivel_to_pelvis /testMasslessRoot/swivel->/testMasslessRoot/pelvis  WeldJoint loopc=-1
 2  3: /testMasslessRoot/hip_r /testMasslessRoot/pelvis->/testMasslessRoot/femur_r CustomJoint loopc=-1
 3  2: #/testMasslessRoot/ground_/testMasslessRoot/femur_r /testMasslessRoot/ground->/testMasslessRoot/femur_r  FreeJoint loopc=-1  ADDED BASE JOINT

4 MOBILIZERS:
 0  1: /testMasslessRoot/ground_to_swivel /testMasslessRoot/ground->/testMasslessRoot/swivel   PinJoint  0
 1  2: /testMasslessRoot/swivel_to_pelvis /testMasslessRoot/swivel->/testMasslessRoot/pelvis  WeldJoint  1
 2  1: #/testMasslessRoot/ground_/testMasslessRoot/femur_r /testMasslessRoot/ground->/testMasslessRoot/femur_r  FreeJoint  3
 3  3: /testMasslessRoot/hip_r /testMasslessRoot/pelvis->#/testMasslessRoot/femur_r_slave_1 CustomJoint  2

0 LOOP CONSTRAINTS:

----- END OF MULTIBODY GRAPH.

The splitting of the femur_r body doesn't make sense to me. @sherm1, why does the MultibodyGraphMaker select the femur_r as the new root Body by adding the FreeJoint (base Joint) to ground? Also the slave, femur_r_slave_1, has a mass that is NaN. Seems like a possble bug in MultibodyGraphMaker.

In contrast, compare this to the graph that is generated when the root, swivel has some mass:

MULTIBODY GRAPH
---------------

4 BODIES:
 0  0: /testMasslessRoot/ground mass=inf mob=-1 master=-1
  jointsAsParent=[ 0]     jointsAsChild=[]        slaves=[]
 1  1: /testMasslessRoot/swivel mass=0.1 mob=0 master=-1
  jointsAsParent=[ 1]     jointsAsChild=[ 0]      slaves=[]
 2  2: /testMasslessRoot/pelvis mass=10.4349 mob=1 master=-1
  jointsAsParent=[ 2]     jointsAsChild=[ 1]      slaves=[]
 3  3: /testMasslessRoot/femur_r mass=8.54927 mob=2 master=-1
  jointsAsParent=[]       jointsAsChild=[ 2]      slaves=[]

3 JOINTS:
 0  0: /testMasslessRoot/ground_to_swivel /testMasslessRoot/ground->/testMasslessRoot/swivel   PinJoint loopc=-1
 1  1: /testMasslessRoot/swivel_to_pelvis /testMasslessRoot/swivel->/testMasslessRoot/pelvis  WeldJoint loopc=-1
 2  2: /testMasslessRoot/hip_r /testMasslessRoot/pelvis->/testMasslessRoot/femur_r CustomJoint loopc=-1

3 MOBILIZERS:
 0  1: /testMasslessRoot/ground_to_swivel /testMasslessRoot/ground->/testMasslessRoot/swivel   PinJoint  0
 1  2: /testMasslessRoot/swivel_to_pelvis /testMasslessRoot/swivel->/testMasslessRoot/pelvis  WeldJoint  1
 2  3: /testMasslessRoot/hip_r /testMasslessRoot/pelvis->/testMasslessRoot/femur_r CustomJoint  2

0 LOOP CONSTRAINTS:
----- END OF MULTIBODY GRAPH.

Which is the graph you would expect.

sherm1 commented 6 years ago

@aseth1 this is likely the same as or related to the issue you filed on Simbody almost a year ago that I said I would investigate but then got busy and forgot about!

There is code in MultibodyGraphMaker that tries to avoid making a terminal massless body -- looks like there's a bug in it since swivel doesn't look terminal at all. I'll see if I can spot what's wrong.

aseth1 commented 6 years ago

@sherm1 that explains why it felt so familiar 😄. Thanks for taking a look. Let us know what you catch 🎣

sherm1 commented 6 years ago

@aseth1, I submitted PR simbody/simbody#592 which allegedly fixes this. Can you review that PR (in Reviewable) and verify that it works? Best to verify in OpenSim, I think -- any ideas on how to do that? The fix is localized and small so could be patched in to try in some other version of Simbody if necessary.

aseth1 commented 6 years ago

Best to verify in OpenSim, I think -- any ideas on how to do that? The fix is localized and small so could be patched in to try in some other version of Simbody if necessary.

It seems to work! I patched: simbody Tag: fd5c03115038a7398ed5ac04169f801a2aa737f2

built and tested locally (via super build of OpenSim dependencies).

sherm1 commented 6 years ago

Great -- thanks, Ajay! Another question is whether I broke anything else. Would be good to run all of OpenSim's test suite with the new code to be sure.

aseth1 commented 6 years ago

I did that locally - all tests passed. I should have mentioned that.

sherm1 commented 6 years ago

Awesome -- @chrisdembia or @tkuchida do you want to review this before I merge it?

chrisdembia commented 6 years ago

@sherm1 I made one comment on the PR just now.

sherm1 commented 6 years ago

Thanks @chrisdembia, I replied there.

aymanhab commented 6 years ago

For 4.0 @aseth1 will update the model to conform to new naming. Longer term may need more comprehensive solution to handle non "Ground" components named "ground"

sherm1 commented 6 years ago

PR simbody/simbody#592 is merged into Simbody's master branch now. That fixes the problem @aseth1 mentioned above with MultibodyGraphMaker. I don't think that closes the whole issue here though since there seem to be some other things mentioned above?

aseth1 commented 6 years ago

Model fails to open because:

  1. uniquely renamed ContactGeometry are now not found by associated ContactForce.
  2. model uses binary vtp files.
jimmyDunne commented 5 years ago

This has been fixed in opensim-org/opensim-models#115.

Won't be seen in the package until next release. A working Zip has been placed on the SimTK Page.