Add test case to exercise ScaleTool as in GUI operation. #1903
Conversation
|
Ready for review/feedback |
aymanhab
changed the title
Bindings/Java/tests/CMakeLists.txt
Outdated
| @@ -86,6 +86,14 @@ macro(OpenSimAddJavaTest TESTNAME) | |||
| DESTINATION "${CMAKE_CURRENT_BINARY_DIR}/TestVisualization") | |||
| file(COPY "${OPENSIM_SHARED_TEST_FILES_DIR}/gait10dof18musc_subject01.osim" | |||
| DESTINATION "${CMAKE_CURRENT_BINARY_DIR}/TestEditProperties") | |||
| file(COPY "${OPENSIM_SHARED_TEST_FILES_DIR}/gait2354_simbody.osim" | |||
There was a problem hiding this comment.
Is it possible to use model/setup files that are already in the repo (e.g., the files used by testScale.cpp), particularly since this isn't a correctness/regression test? There are already 4 files named "gait2354_simbody.osim" in the repo.
There was a problem hiding this comment.
Thanks for the note @tkuchida I'm actually using the recommended method of having these files added to the shared testing folder, we should make sure other tests are using it from there. This may or maynot be part of this PR but good point.
There was a problem hiding this comment.
Yes, this is the recommended method, but ideally, one of the pre-existing gait2354 models would have been moved to the shared folder.
There was a problem hiding this comment.
Added a comment to this effect in Issue #206.
There was a problem hiding this comment.
While we want to move to shared resources to avoid duplication, I think copying essentially the same files in testScale violates that spirit. I would prefer we use the testScale files that already exist instead of making yet another copy.
| // calls below which exercise the "run" button | ||
| Model processedModel = new Model(unscaledModel); | ||
| processedModel.setName(scaleTool.getName()); | ||
| OpenSimContext peocessedContext = new OpenSimContext(processedModel.initSystem(), processedModel); |
There was a problem hiding this comment.
I know it doesn't matter to the compiler, but there's a typo in the variable name ("peocessedContext").
Fix typo in variable name per feedback on PR.
| System.exit(-1); | ||
| } | ||
| System.out.println("ScaleTool created!"); | ||
| // Populate the tabs of Scale factors and measurements |
There was a problem hiding this comment.
This comment does not describe what is happening below.
| } | ||
| System.gc(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we remove Model::replaceMarkerSet() (see #1780) and deleteUnusedMarkers() since they appear to be unused by the GUI?
There was a problem hiding this comment.
MarkerPlacer::processModel uses deleteUnusedMarkers(), it's not used by the GUI directly. replaceMarkerSet is not called by the GUI anymore and can be removed.
…va_scaletool # Conflicts: # Bindings/Java/tests/CMakeLists.txt
Fixes issue #<issue_number>
Brief summary of changes
Exercise ScaleTool in workflow similar to GUI. This should be beefed up to check the values of scale factors and also to exercise the workflow where ScaleTool is constructed live from scratch rather than from XML file.
Testing I've completed
Ran test case and loaded models before and after scaling in GUI
Looking for feedback on...
CHANGELOG.md (choose one)
The Doxygen for this PR can be viewed at http://myosin.sourceforge.net/?C=N;O=D; click the folder whose name is this PR's number.
This change is