opensim-org / opensim-core

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

C3DFileAdapter should not throw if either forces or markers data is not present #2421

Closed aseth1 closed 5 years ago

aseth1 commented 5 years ago

From @jimmyDunne

C3D files without any force data. I am getting an exception when I try to read them with the C3DFileAdapter().


Java exception occurred:
java.lang.RuntimeException: map::at:  key not found
at
org.opensim.modeling.opensimCommonJNI.C3DFileAdapter_read__SWIG_1(Native
Method)
at
org.opensim.modeling.C3DFileAdapter.read(C3DFileAdapter.java:66)

I have attached a zip with four C3Ds from the same session— three are kinematic calibration trials that don't have forces and one walking trial with both markers and forces (which works).

jimmyDunne commented 5 years ago

@jimmyDunne to test. May have been fixed already.

May need to change read to readFile(), I will check and set up an issue to fix. If so, will have to update Matlab and Python scripts as well.

aymanhab commented 5 years ago

@jimmyDunne Can you attach the c3d file with the missing data (described in the issue description) so we can test a fix? Thanks

jimmyDunne commented 5 years ago

Archive.zip

I have attached a zip with four C3Ds from the same session— three are kinematic calibration trials that don't have forces and one walking trial with both markers and forces (which works).

aymanhab commented 5 years ago

One reason we use readFile rather than the generic read is that the read method of C3DFileAdapter takes a second argument

C3DFileAdapter::readFile(filename, C3DFileAdapter::ForceLocation::OriginOfForcePlate);

tkuchida commented 5 years ago

One reason we use readFile rather than the generic read is that the read method of C3DFileAdapter takes a second argument

I found this part of the interface confusing. I would have expected only one method to read a C3D file. Why not override readFile() in the C3DFileAdapter class to throw so that people know to use the 2-argument read() instead? (The second argument of read() has a default anyway.) A potential danger is that, if a user knows only about readFile() and the data doesn't make sense, it isn't obvious that there's a separate read() method where you can specify how the forces are expressed.

jimmyDunne commented 5 years ago

Below are the behaviors as I see right now;

Case 1 - C3D with only Marker If I use readFile(), I get an exception;

>> c3d = C3DFileAdapter().readFile('Static.c3d',0)
Java exception occurred:
java.lang.RuntimeException: map::at:  key not found
    at org.opensim.modeling.opensimCommonJNI.C3DFileAdapter_readFile__SWIG_2(Native Method)
    at org.opensim.modeling.C3DFileAdapter.readFile(C3DFileAdapter.java:82)

And if I use read(), it works;

>> c3d = C3DFileAdapter().read('Static.c3d')

c3d = org.opensim.modeling.StdMapStringAbstractDataTable@25ddbbbb

Case 2 - C3D with both Markers and Forces I use readFile() and it works

>> c3d4 = C3DFileAdapter().readFile('Walking2.c3d',1)
c3d = org.opensim.modeling.StdMapStringTimeSeriesTableVec3@1db0ec27

When I use read(), I have to input only a single argument, otherwise I get an exception;

>> c3d = C3DFileAdapter().read('Walking2.c3d',1)
No method 'read' with matching signature found for class 'org.opensim.modeling.C3DFileAdapter'.

So, as a user, I need to know beforehand if my C3D has forces in it or not and use the correct read method otherwise I get an exception.

We should pick one method, and the user should be able to get markers and forces. And if there are no forces it should return an empty table when asked.