From cb88eacfb81c3cd1af085d48cefd553a38a3abda Mon Sep 17 00:00:00 2001 From: Adam Kewley Date: Thu, 3 Oct 2024 15:00:29 +0200 Subject: [PATCH 1/2] Add failing test for ExternalLoads-breaks-on-copy behavior --- OpenSim/Tools/Test/CMakeLists.txt | 5 + .../external-loads-in-subdir.xml | 27 ++++ .../forces-in-subdir.mot | 9 ++ .../model-in-subdir.osim | 115 ++++++++++++++++++ OpenSim/Tools/Test/testExternalLoads.cpp | 33 ++--- 5 files changed, 174 insertions(+), 15 deletions(-) create mode 100644 OpenSim/Tools/Test/ExternalLoadsInSubdir/external-loads-in-subdir.xml create mode 100644 OpenSim/Tools/Test/ExternalLoadsInSubdir/forces-in-subdir.mot create mode 100644 OpenSim/Tools/Test/ExternalLoadsInSubdir/model-in-subdir.osim diff --git a/OpenSim/Tools/Test/CMakeLists.txt b/OpenSim/Tools/Test/CMakeLists.txt index a9ad9d72ec..70c8238b94 100644 --- a/OpenSim/Tools/Test/CMakeLists.txt +++ b/OpenSim/Tools/Test/CMakeLists.txt @@ -9,3 +9,8 @@ OpenSimAddTests( DATAFILES ${TEST_FILES} LINKLIBS osimTools Catch2::Catch2WithMain ) + + +# edge-case: one test ensures that `ExternalLoads` still works - even when it isn't in +# the working directory of the test runner +file(COPY "ExternalLoadsInSubdir" DESTINATION "${CMAKE_CURRENT_BINARY_DIR}") diff --git a/OpenSim/Tools/Test/ExternalLoadsInSubdir/external-loads-in-subdir.xml b/OpenSim/Tools/Test/ExternalLoadsInSubdir/external-loads-in-subdir.xml new file mode 100644 index 0000000000..1b125b1492 --- /dev/null +++ b/OpenSim/Tools/Test/ExternalLoadsInSubdir/external-loads-in-subdir.xml @@ -0,0 +1,27 @@ + + + + + + + false + + new_body + + new_body + + new_body + + head_v + + head_p + + + Unassigned + + + + + forces-in-subdir.mot + + diff --git a/OpenSim/Tools/Test/ExternalLoadsInSubdir/forces-in-subdir.mot b/OpenSim/Tools/Test/ExternalLoadsInSubdir/forces-in-subdir.mot new file mode 100644 index 0000000000..80f90f9dca --- /dev/null +++ b/OpenSim/Tools/Test/ExternalLoadsInSubdir/forces-in-subdir.mot @@ -0,0 +1,9 @@ +pendulum_swing +version=1 +nRows=2 +nColumns=7 +inDegrees=yes +endheader +time head_vx head_vy head_vz head_px head_py head_pz +0 0 0 0 0 0 0 +1 1000 1000 1000 0 0 0 diff --git a/OpenSim/Tools/Test/ExternalLoadsInSubdir/model-in-subdir.osim b/OpenSim/Tools/Test/ExternalLoadsInSubdir/model-in-subdir.osim new file mode 100644 index 0000000000..3091f5cdb9 --- /dev/null +++ b/OpenSim/Tools/Test/ExternalLoadsInSubdir/model-in-subdir.osim @@ -0,0 +1,115 @@ + + + + + + + + + .. + + 0.20000000000000001 0.20000000000000001 0.20000000000000001 + + + + + + + + + + .. + + 0.20000000000000001 0.20000000000000001 0.20000000000000001 + + + + + + .. + + 0.10000000000000001 + + + + 1 + + 0 0 0 + + 1 1 1 0 0 0 + + + + + + + + + + ground_offset + + new_body_offset + + + + + 1.5707963705062866 + + + + + + + + + .. + + 0.20000000000000001 0.20000000000000001 0.20000000000000001 + + + /ground + + 0 1 0 + + -0 0 -0 + + + + + + .. + + 0.20000000000000001 0.20000000000000001 0.20000000000000001 + + + /bodyset/new_body + + -0 0.46722877025604248 -0 + + -0 0 -0 + + + + + + + + + + + + + + + + + + + + + + true + + + + diff --git a/OpenSim/Tools/Test/testExternalLoads.cpp b/OpenSim/Tools/Test/testExternalLoads.cpp index d5478453cc..c292db9aad 100644 --- a/OpenSim/Tools/Test/testExternalLoads.cpp +++ b/OpenSim/Tools/Test/testExternalLoads.cpp @@ -21,24 +21,15 @@ * limitations under the License. * * -------------------------------------------------------------------------- */ -#include #include #include -using namespace OpenSim; -using namespace std; - -void testExternalLoad(); -void testExternalLoadDefaultProperties(); +#include -int main() -{ - SimTK_START_TEST("testExternalLoads"); - SimTK_SUBTEST(testExternalLoad); - SimTK_SUBTEST(testExternalLoadDefaultProperties); - SimTK_END_TEST(); -} +#include +using namespace OpenSim; +using namespace std; void addLoadToStorage(Storage &forceStore, SimTK::Vec3 force, SimTK::Vec3 point, SimTK::Vec3 torque) { @@ -85,7 +76,7 @@ void addLoadToStorage(Storage &forceStore, SimTK::Vec3 force, SimTK::Vec3 point, forces->addToRdStorage(forceStore, 0.0, 1.0); } -void testExternalLoad() +TEST_CASE("ExternalLoads") { using namespace SimTK; @@ -249,7 +240,8 @@ void testExternalLoad() } // Ensure the default values for the ExternalForce properties work as expected. -void testExternalLoadDefaultProperties() { +TEST_CASE("ExternalLoads Default Properties") +{ using namespace SimTK; Model model("Pendulum.osim"); @@ -303,3 +295,14 @@ void testExternalLoadDefaultProperties() { xf->set_force_expressed_in_body("nonexistent"); model.initSystem(); } + +TEST_CASE("ExternalLoads Can Be Copied") +{ + OpenSim::Model model{"ExternalLoadsInSubdir/model-in-subdir.osim"}; + model.finalizeConnections(); // should work + model.addModelComponent(&dynamic_cast(*Object::makeObjectFromFile("ExternalLoadsInSubdir/external-loads-in-subdir.xml"))); + model.finalizeConnections(); // should also work + + OpenSim::Model copy{model}; // create an independent copy containing the `OpenSim::ExternalLoads` + copy.finalizeConnections(); // should work (wasn't when this test was written) +} From 57dd758c8e40f9d2301ec14dd27c0ae805ab3ada Mon Sep 17 00:00:00 2001 From: Adam Kewley Date: Thu, 3 Oct 2024 15:16:42 +0200 Subject: [PATCH 2/2] Fix ExternalLoads not copying the underlying connection to its associated document (#3926) --- CHANGELOG.md | 2 ++ OpenSim/Simulation/Model/ExternalLoads.cpp | 6 ++++++ OpenSim/Tools/Test/CMakeLists.txt | 2 +- OpenSim/Tools/Test/testExternalLoads.cpp | 4 ++++ 4 files changed, 13 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b8ac3b1666..780137b318 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,8 @@ v4.6 - Made improvements to `MocoUtilities::createExternalLoadsTableForGait()`: center of pressure values are now set to zero, rather than NaN, when vertical force is zero, and the vertical torque is returned in the torque columns (rather than the sum of the sphere torques) to be consistent with the center of pressure GRF representation. +- Fixed an issue where a copy of an `OpenSim::Model` containing a `OpenSim::ExternalLoads` could not be + finalized (#3926) v4.5.1 ====== diff --git a/OpenSim/Simulation/Model/ExternalLoads.cpp b/OpenSim/Simulation/Model/ExternalLoads.cpp index 686c287a30..2b1d8e3257 100644 --- a/OpenSim/Simulation/Model/ExternalLoads.cpp +++ b/OpenSim/Simulation/Model/ExternalLoads.cpp @@ -76,6 +76,12 @@ ExternalLoads::ExternalLoads(const ExternalLoads &otherExternalLoads) : ModelComponentSet(otherExternalLoads), _dataFileName(_dataFileNameProp.getValueStr()) { + // copy the document over, because it's used during `extendFinalizeConnections` + // to figure out where the associated motion file (#3926) + if (auto* document = otherExternalLoads.getDocument()) { + setDocument(std::make_unique(*document).release()); + } + setNull(); // Class Members diff --git a/OpenSim/Tools/Test/CMakeLists.txt b/OpenSim/Tools/Test/CMakeLists.txt index 70c8238b94..2dac8afae6 100644 --- a/OpenSim/Tools/Test/CMakeLists.txt +++ b/OpenSim/Tools/Test/CMakeLists.txt @@ -12,5 +12,5 @@ OpenSimAddTests( # edge-case: one test ensures that `ExternalLoads` still works - even when it isn't in -# the working directory of the test runner +# the working directory of the test runner (#3926) file(COPY "ExternalLoadsInSubdir" DESTINATION "${CMAKE_CURRENT_BINARY_DIR}") diff --git a/OpenSim/Tools/Test/testExternalLoads.cpp b/OpenSim/Tools/Test/testExternalLoads.cpp index c292db9aad..105ec25517 100644 --- a/OpenSim/Tools/Test/testExternalLoads.cpp +++ b/OpenSim/Tools/Test/testExternalLoads.cpp @@ -296,6 +296,10 @@ TEST_CASE("ExternalLoads Default Properties") model.initSystem(); } +// related: #3926 +// +// adding a valid `OpenSim::ExternalLoads` to a model shouldn't result in a model +// that cannot be copied. TEST_CASE("ExternalLoads Can Be Copied") { OpenSim::Model model{"ExternalLoadsInSubdir/model-in-subdir.osim"};