These changes fix bugs in PrescribedController recently introduced after converting Controller to support a list Socket to maintain actuator connections. This PR also changes the behavior of prescribeControlForActuator() by making a copy of the Function at the passed in Function pointer rather than taking ownership. This change protects users from possible crashes when Function ownership is passed to the controller (e.g., during garbage collection in Java/Matlab).
The main changes:
A check was added during deserialization to ensure that the number of actuators in the list Socket matches the number of control function. EDIT: removed, this was causing serialization tests to fail.
The map _actuLabelsToControlFunctionIndexMap is also updated during deserialization to account for any control functions existing the model.
The functions in ControlFunctions are now reordered based on the order of actuators connected via the list Socket to provide the correct order for computeControls() and serialization. This actually fixes an undetected bug: the member variable _actuIndexToControlFunctionIndexMap was not being used within computeControls() as it should have been. However, reordering ControlFunctions makes _actuIndexToControlFunctionIndexMap unnecessary, so it has been removed.
Testing I've completed
Added relavant tests to testControllers.cpp.
Tested the changes on the sample scripts included in the related issues: no crashes occur when passing functions to prescribeControlForActuator.
Looking for feedback on...
If we're going to copy the Function passed via prescribeControlForActuator(), should change the signature to use a const reference to the function (i.e., const Function& prescribedFunction), rather than a pointer? It would change usage in C++, but scripting users wouldn't notice a difference.
CHANGELOG.md (choose one)
no need to update because...bug fixes (unless we change the signature for prescribeControlForActuator()).
Fixes issue #3749, #3750
Brief summary of changes
These changes fix bugs in
PrescribedController
recently introduced after convertingController
to support a listSocket
to maintain actuator connections. This PR also changes the behavior ofprescribeControlForActuator()
by making a copy of theFunction
at the passed inFunction
pointer rather than taking ownership. This change protects users from possible crashes whenFunction
ownership is passed to the controller (e.g., during garbage collection in Java/Matlab).The main changes:
A check was added during deserialization to ensure that the number of actuators in the listEDIT: removed, this was causing serialization tests to fail.Socket
matches the number of control function._actuLabelsToControlFunctionIndexMap
is also updated during deserialization to account for any control functions existing the model.ControlFunctions
are now reordered based on the order of actuators connected via the listSocket
to provide the correct order forcomputeControls()
and serialization. This actually fixes an undetected bug: the member variable_actuIndexToControlFunctionIndexMap
was not being used withincomputeControls()
as it should have been. However, reorderingControlFunctions
makes_actuIndexToControlFunctionIndexMap
unnecessary, so it has been removed.Testing I've completed
testControllers.cpp
.prescribeControlForActuator
.Looking for feedback on...
Function
passed viaprescribeControlForActuator()
, should change the signature to use a const reference to the function (i.e.,const Function& prescribedFunction
), rather than a pointer? It would change usage in C++, but scripting users wouldn't notice a difference.CHANGELOG.md (choose one)
prescribeControlForActuator()
).This change is![Reviewable](https://reviewable.io/review_button.svg)