opensim-org / opensim-core

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

Manage `Controller` actuators with a list `Socket` #3683

Closed nickbianco closed 7 months ago

nickbianco commented 8 months ago

Fixes issue #3675

Brief summary of changes

This PR updates how Controller maintains a list of controlled actuators by replacing the internally-managed Set<Actuator> with a list Socket. This mitigates the need for custom actuator connection code and a custom copy constructor and assignment operator to avoid memory leaks. In addition, all Controllers can now leverage other Sockets (including Inputs) in concrete implementations.

I tried to keep API changes minimal, but the switch to Sockets fundamentally changes Actuators connection, so some changes were necessary. A brief list of the main changes:

After going through the tests and scripts, these changes seemed to be pretty reasonable. Most usages of PrescribedController didn't set controls by index, but rather by actuator name, which is the interface being kept here.

Testing I've completed

Looking for feedback on...

testCMCArm26_Thelen.cpp is failing for reasons I've yet to identify. It fails when comparing muscle activations against the current standard file, for only a single muscle. I'm not sure why this single test fails when all other CMC tests pass. I noticed that testCMCArm26_Millard.cpp passes, but uses looser tolerances for the muscle activation comparisons. Is it possible that testCMCArm26_Thelen.cpp is just fragile and some small differences in Actuator computations in the CMC controller from this PR are leading to slightly different results? CMC uses some dark magic rather odd memory managment of Controllers, so maybe the switch to Sockets is having a subtle effect on how Actuator controls are computed. Or maybe I fixed a bug, and the old standard file is now invalid 🤷.

CHANGELOG.md (choose one)


This change is Reviewable

nickbianco commented 8 months ago

@aymanhab, after looking into it again, I remember why I needed _actuatorNamesFromXML. In order to avoid this extra member variable, I'd need to implement extendFinalizeConnections() to intercept the legacy actuator names before the Socket tries to make connections based on them. However, ModelComponent does not provide an interface to extendFinalizeConnections() (it is marked final), therefore my only option is extendConnectToModel(). Unfortunately, extendConnectToModel() is called after extendFinalizeConnections(), so I need to temporarily store the legacy actuator names to avoid the component trying to connect to an invalid connectee path.

If there's another way around this (short of changing the ModelComponent API), I'm still open to alternatives.

nickbianco commented 8 months ago

@aymanhab, no worries happy to clarify. The biggest change is that connected actuators will now have their whole path stored in the XML.

Before this change, the XML, for example, looks like this:

<ControlSetController name="Control_Set_Controller">
  <!--A list of actuators that this controller will control.The keyword ALL indicates the controller will controll all the acuators in the model-->
  <actuator_list>actu_name1 actu_name2</actuator_list>
  <!--XML file containing the controls for the controlSet.-->
  <controls_file> subject01_walk1_controls.xml </controls_file>
</ControlSetController>

And after this change:

<ControlSetController name="Control_Set_Controller">
  <!--A list of actuators that this controller will control.The keyword ALL indicates the controller will controll all the acuators in the model-->
  <socket_actuators>/path/to/actu_name1 /path/to/actu_name2</socket_actuators>
  <!--XML file containing the controls for the controlSet.-->
  <controls_file> subject01_walk1_controls.xml </controls_file>
</ControlSetController>

We could try prepending all the actuator names with /forceset, but this won't work if someone has an actuator not stored in the ForceSet, e.g., top-level path /actu_name1. The only way I've found (so far) to rigorously check the paths is my current implementation.

nickbianco commented 7 months ago

Thanks @carmichaelong and @aymanhab!