Feature: add ForceProducer and ForceConsumer APIs#3891
Conversation
db15b54 to
9dfc42f
Compare
|
The rollout if now almost complete, and it's compiling + passing on mac + ubuntu. The Windows issue is external (CI images were updated and now there's warnings/errors - see #3895) |
abda563 to
484dbaa
Compare
|
|
|
After reverting the |
|
The build is working, test passing etc. with the reversion - I'll tackle it in a separate PR. I have also added a test that compares the I'll un-WIP this and try to wrangle someone for a review. |
ForceProducer and ForceConsumer APIsForceProducer and ForceConsumer APIs
ForceProducer and ForceConsumer APIsForceProducer and ForceConsumer APIs
|
When self-reviewing the diff I found a few things that need to be re-thought:
|
adamkewley
left a comment
There was a problem hiding this comment.
I have written a walkthrough of the code as a self-review so that other reviewers get more context around some parts of the diff.
| void BodyActuator::computeForce(const SimTK::State& s, | ||
| SimTK::Vector_<SimTK::SpatialVec>& bodyForces, | ||
| SimTK::Vector& generalizedForces) const | ||
| double BodyActuator::getPower(const SimTK::State& s) const |
There was a problem hiding this comment.
This was moved upward to match its position in the corresponding header.
| return force; | ||
| } | ||
|
|
||
| //============================================================================== |
There was a problem hiding this comment.
This was removed, because the base class (PathActuator) performs identical steps and supports features like overriding the actuation and setting the actuation parameter so that the actuator's power is also up-to-date.
| SimTK::Vector_<SimTK::SpatialVec>& bodyForces, | ||
| SimTK::Vector& mobilityForces) const | ||
| { | ||
| ForceApplier forceApplier{getModel().getMatterSubsystem(), bodyForces, mobilityForces}; |
There was a problem hiding this comment.
This is a (known) API break:
- The original implementation marked
AbstractGeometryPath::addInEquivalentForcesasvirtual, so that implementations can override it with a specialization - The new implementation instead requires overriding
AbstractGeometryPath::produceForces - The new implementation provides a concrete
AbstractGometryPath::addInEquivalentForces(here) that should behave identically to the old version - providedproduceForcesimplementations are logically equivalent - Therefore, the break shouldn't break downstream code that uses
AbstractGeometryPath::addInEquivalentForces- only concrete implementations thatoverrideit need to change, and the change is usually very minor (callforceConsumer.consume*rather than adding things into vectors - see most of this PR).
| void Blankevoort1991Ligament::computeForce(const SimTK::State& s, | ||
| SimTK::Vector_<SimTK::SpatialVec>& bodyForces, | ||
| SimTK::Vector& generalizedForces) const { | ||
| if (get_appliesForce()) { |
There was a problem hiding this comment.
get_appliesForce is automatically handled by the concrete part of ForceProducer::produceForces, so the virtual part (ForceProducer::implProduceForces) should never need to worry about this flag.
A unit test was added in testForceProducer to ensure that the appliesForce flag is actually obeyed by the concrete method, so that regressions w.r.t. the flag can be spotted.
| // Temporary solution until implemented with Sockets | ||
| SimTK::ReferencePtr<const PhysicalFrame> _body1; | ||
| SimTK::ReferencePtr<const PhysicalFrame> _body2; | ||
| SimTK::ReferencePtr<const SimTK::MobilizedBody> _b1; |
There was a problem hiding this comment.
These were removed as a drive-by for sanity's sake: they are references within _body1 and _body2.
| @@ -0,0 +1,45 @@ | |||
| #include "ForceApplier.h" | |||
There was a problem hiding this comment.
A ForceApplier unconditionally applies each force to the relevant vectors. It's up to a higher-level (e.g. ForceProducer::computeForce) to decide whether forces should be applied at all (e.g. because appliesForces is false).
| const double& tension, | ||
| SimTK::Vector_<SimTK::SpatialVec>& bodyForces, | ||
| SimTK::Vector& mobilityForces) const | ||
| void GeometryPath::produceForces(const SimTK::State& s, |
There was a problem hiding this comment.
There are now unused variables etc. in this because it was directly ported from the original implementation with very very minimal changes. The reason why is because testMocoInverse was breaking after porting this and I suspect that it will have to very carefully be refactored in a separate PR.
| // Cache the computed tension and strain of the ligament | ||
|
|
||
| this->_tensionCV = addCacheVariable("tension", 0.0, SimTK::Stage::Velocity); | ||
| this->_strainCV = addCacheVariable("strain", 0.0, SimTK::Stage::Velocity); |
There was a problem hiding this comment.
Driveby: this was removed because it appears to be unused? I can't find any string uses of "strain" where it might be indirectly read
| } | ||
|
|
||
| const double& Ligament::getTension(const SimTK::State& s) const | ||
| double Ligament::getTension(const SimTK::State& s) const |
There was a problem hiding this comment.
Ligament was refactored to "properly" use cache variables.
Now, calling getTension will result in a cache variable computation (if necessary). Previously, the tension was computed during computeForce, which created a temporal dependency between getTension and computeForce (i.e. you couldn't use Ligament::getTension without first calling Ligament::computeForce).
| const SimTK::State& s, | ||
| SimTK::Vec6 f, SimTK::Vector_<SimTK::SpatialVec>& physicalForces) const | ||
| { | ||
| // A `ForceConsumer` that applies body spatial vectors from `producePhysicalForcesFromInternal` |
There was a problem hiding this comment.
This adaptor was written so that the maths around TwoFrameLinker is all in one place (the ForceConsumer-based one), rather than having two almost-identical implementations of the maths.
ForceProducer and ForceConsumer APIsForceProducer and ForceConsumer APIs
|
Ok, now it's ready for review 😉 |
PrescribedForces
ComputationalBiomechanicsLab/opensim-creator#906
pepbos
left a comment
There was a problem hiding this comment.
Very nice and useful.
Two small comments but LGTM.
|
Thanks for reviewing, @pepbos @nickbianco - this PR also impacts the If so, and if there's no further review comments, I'll go ahead and merge this |
nickbianco
left a comment
There was a problem hiding this comment.
Reviewed 58 of 66 files at r1, 1 of 1 files at r2, 3 of 4 files at r4, 4 of 4 files at r5, all commit messages.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @adamkewley and @pepbos)
a discussion (no related file):
Previously, adamkewley (Adam Kewley) wrote…
(Reviewable was unable to map this GitHub inline comment thread to the right spot — sorry!)
A
ForceApplierunconditionally applies each force to the relevant vectors. It's up to a higher-level (e.g.ForceProducer::computeForce) to decide whether forces should be applied at all (e.g. becauseappliesForcesisfalse).
Nice changes @adamkewley! I left a review, mostly to stay up-to-date with these changes. The AbstractGeometryPath changes look fine to me (I had some thoughts about maybe removing addInEquivalentForces, but it's probably right to keep it for now).
OpenSim/Simulation/Model/AbstractGeometryPath.h line 143 at r5 (raw file):
const double& tension, SimTK::Vector_<SimTK::SpatialVec>& bodyForces, SimTK::Vector& mobilityForces) const;
I wonder if we could safely get rid of addInEquivalentForces() entirely. It would be a more breaking change than the current one, but I wonder how many users rely on it since it modifies the system's vector of body and mobility forces. I think keeping the concrete version is the right thing to do for now, but this could be deprecated in future versions.
OpenSim/Simulation/Model/ForceApplier.h line 82 at r5 (raw file):
const SimTK::SimbodyMatterSubsystem* _matter; SimTK::Vector_<SimTK::SpatialVec>* _bodyForces; SimTK::Vector* _generalizedForces;
minor: would it make sense to use SimTK::ReferencePtr here? Not sure that it matters much, but I'm always inclined to balk at raw pointers.
OpenSim/Simulation/Model/ForceProducer.h line 61 at r5 (raw file):
* provided `ForceConsumer`. * * Note: this function only produces the forces and does not apply them to anything. It's
minor:
Suggestion:
* @note this function only produces the forces and does not apply them to anything. It'sOpenSim/Simulation/Model/GeometryPath.cpp line 385 at r4 (raw file):
Previously, adamkewley (Adam Kewley) wrote…
There are now unused variables etc. in this because it was directly ported from the original implementation with very very minimal changes. The reason why is because
testMocoInversewas breaking after porting this and I suspect that it will have to very carefully be refactored in a separate PR.
testMocoInverse was failing in a previous refactor? That test includes regression tests (e.g., comparing muscle excitations to a "standard" solution) that will be sensitive to changes in muscle force production due to GeometryPath changes. I'm happy to take a look into a proper fix in a follow up PR, feel free to document in a new issue.
OpenSim/Simulation/Model/Ligament.cpp line 205 at r4 (raw file):
Previously, adamkewley (Adam Kewley) wrote…
Ligamentwas refactored to "properly" use cache variables.Now, calling
getTensionwill result in a cache variable computation (if necessary). Previously, the tension was computed duringcomputeForce, which created a temporal dependency betweengetTensionandcomputeForce(i.e. you couldn't useLigament::getTensionwithout first callingLigament::computeForce).
I think this is an issue in other forces (e.g., ExpressionBasedCoordinateForce). I came across this recently but forgot to create an issue to track it, I'll do that now.
OpenSim/Simulation/Test/testForceProducer.cpp line 1 at r5 (raw file):
#include <OpenSim/Simulation/Model/ForceProducer.h>
Add test header with the license statement, copyright, etc.
adamkewley
left a comment
There was a problem hiding this comment.
Reviewable status: 64 of 66 files reviewed, 14 unresolved discussions (waiting on @nickbianco and @pepbos)
OpenSim/Simulation/Model/AbstractGeometryPath.h line 143 at r5 (raw file):
Previously, nickbianco (Nick Bianco) wrote…
I wonder if we could safely get rid of
addInEquivalentForces()entirely. It would be a more breaking change than the current one, but I wonder how many users rely on it since it modifies the system's vector of body and mobility forces. I think keeping the concrete version is the right thing to do for now, but this could be deprecated in future versions.
It's probably right, in the sense that it's now a kind of redundant method, but I'd lean towards keeping it around for a while, purely because the backwards-compatibility shim wasn't too arduous to implement (as in, it's not a large amount of code to maintain)
OpenSim/Simulation/Model/ForceApplier.h line 82 at r5 (raw file):
Previously, nickbianco (Nick Bianco) wrote…
minor: would it make sense to use
SimTK::ReferencePtrhere? Not sure that it matters much, but I'm always inclined to balk at raw pointers.
SimTK::ReferencePtr automatically nulls on-copy, which would be incorrect in this case because ForceApplier isn't cross-referencing within its own datastructure (it makes more sense in OpenSim's larger datastructures where there are cross-references, and not wiping them on-copy would cause issues).
I'd argue the bigger decision is whether it (and the constructor) should use references vs. pointers. If the ctor uses references, that removes the possibility of nullability (good) but requires that the caller knows that the reference lifetime has to extend beyond the constructor call (bad). A pointer argument (as @pepbos mentioned) is largely the same, save for nullability (bad if unnecessary), but makes it more explicit to the caller that the object being created (ForceApplier) points to (i.e. doesn't own its own data) the externally-owned vectors. The use of a pointer member vs. a raw reference member is really about supporting copy(assignment) - raw references disable it.
The class that is closer to ReferencePtr but doesn't have the undesirable wipe-on-copy behavior is std::reference_wrapper - might be worth considering.
It might be worth it to revert the ctor back to using references + reference wrapper if it's simpler, though.
OpenSim/Simulation/Model/GeometryPath.cpp line 385 at r4 (raw file):
Previously, nickbianco (Nick Bianco) wrote…
testMocoInversewas failing in a previous refactor? That test includes regression tests (e.g., comparing muscle excitations to a "standard" solution) that will be sensitive to changes in muscle force production due to GeometryPath changes. I'm happy to take a look into a proper fix in a follow up PR, feel free to document in a new issue.
I'll probably give changing the GeometryPath::implProduceForces a swing quite soon after this is merged because I need to have "resolved" point-forces (i.e. remove the tension forces that cancel out), so we can discuss it there.
OpenSim/Simulation/Test/testForceProducer.cpp line 1 at r5 (raw file):
Previously, nickbianco (Nick Bianco) wrote…
Add test header with the license statement, copyright, etc.
Done.
|
I think I'm going to go ahead and merge this as-is: the use of raw pointer members is limited to private variables and can be refactored later if there's long term maintenance concerns (which there might be - I'm still undecided on the best approach), and the pointer-based constructor, which is more explicit at the cost of having a more C/C++ey feel, can always be overridden with an additional reference-based constructor if it becomes a pain point later on. Just FYI, I plan on integrating the API immediately into the dev ( Cheers to @nickbianco and @pepbos for reviewing this. |
This PR adds
ForceProducer,ForceConsumer, andForceApplierand rolls them out to as manyOpenSim::Forceimplementations as feasible.Overview
ForceProduceris an abstractOpenSim::Forcethat downstream classes can use, rather thanForce, to emit their forces one-by-one to aForceConsumer.ForceConsumeris an abstract class with consumption methods (e.g.consumePointForce) that directly mirror the ofOpenSim::Forceequivalents (e.g.Force::applyForceToPoint).ForceApplieris aForceConsumerthat applies each consumed force to aSimTK::Vector_<SimTK::SpatialForce>of body forces and aSimTK::Vectorof mobility (generalized) forces.Key features:
ForceProducer's virtual one-by-one API is in contrast toOpenSim::Force, where forces are added by directly modifying the physics engine'sbodyForcesandgeneralizedForcesvectors in-place. The benefit of this is that it decouples "force production/consumption" from "force application", which lets not-simbody (e.g. UIs) use the force API more easily.ForceConsumer's API is designed with the similar naming, the same arguments, and the same spatial (e.g. torques in ground) requirements as the equivalentOpenSim::ForceAPI methods. This makes it easier to portOpenSim::Forcecode to theForceConsumerAPI. TheForceConsumerAPI also separates "body forces" from "point forces", so that concreteForceProducersare able to emit point-force information to other systems (e.g. debuggers that would like to print point forces).ForceProducer's API provides a default implementation ofOpenSim::Force::computeForce, which internally usesForceApplierapply the one-at-a-time forces to the vectors thatOpenSim::Force::computeForceuses. This makesForceProducerbackwards-compatible with any existing code that handlesOpenSim::Forces.AbstractGeometryPath.The
ForceConsumerAPIThe
ForceConsumerAPI is an abstract class that has a public API that consumes four types of forces:Concrete implementations of
ForceConsumermay override each of the correspondingvirtual void implConsume*methods in order to consume that type of force. The default implementation of eachimplConsume*function does nothing (e.g. it ignores the force). A concrete implementation does not have to do anything with a force, which enables using the API to (e.g.) implement force debuggers, force visualizers, point-force reporters, and so on.The
ForceProducerAPIThe
ForceProducerAPI is an abstract class that exposesproduceForces:Concrete implementations can then inherit from
ForceProducerand supply animplProduceForcesin which the concrete implementation calls (e.g.)ForceConsumer::consumePointForce. The defaultForceProducer::computeForceimplementation usesForceApplierto satisfy theOpenSim::ForceAPI. It'soverrided, rather thanfinald, because thereis legacy code in OpenSim that does extra stuff after
computeForceis called (e.g. resetting state variables). An ideal implementation wouldfinalcomputeForce.Motivation (why add this?)
The
OpenSim::ForceAPI isn't easy for downstream code, such as OpenSim Creator's, to hook into. Because theOpenSim::ForceAPI couples force generation/production with application. Because of this, downstream code has to:ForceAdaptorto mutate those vectors.The
OpenSim::ForceAPI also erases point-force information. At theOpenSim::Forcelayer of the API, all forces are already resolved into body forces and body torques. However, downstream code might want to visualize point-forces (e.g. when visualizingExternalForce,PrescribedForce, andGeometryPath).Brief Summary of Changes
ForceProducerForceConsumerForceApplierForceProducerForceConsumerandForceProducerAPIs to all existing OpenSim classes that were previously using theForceAPI.opensim-corecan be queried using theForceProducerAPIForceimplementationsvirtualAbstractGeometryPath::addInEquivalentForceswith avirtualAbstractGeometryPath::produceForcesAPI:ForceProducers withAbstractGeometryPath(e.g. necessary forPathActuatorand similar).AbstractGeometryPath::addInEquivalentForcesfunction that uses the newproduceForcesAPI internally.AbstractGeometryPath. I consider this a minor breakage, because the API is very new, it's unlikely 3rd-party code is implementing it, and porting the implementation to useForceConsumeris an easy change.ForceProducerandForceConsumerAPIs to the SWIG bindingsNo changes were made to any existing test suites, binding tests, etc. This is to say, these changes should not have broken the existing functionality of the OpenSim API - apart from the virtual API change in
AbstractGeometryPath.Testing I've completed
ForceProducerto ensure it behaves as expectedForceApplier(viaForceProducer::computeForce) to the mutations caused by a "typical"Forceusage.Forces were ported to this API. Any existing test suite that tests force components will also be indirectly testing this API (e.g. any tests testing muscle implementations are now indirectly exercising the new API).Looking for feedback on...
CHANGELOG.md
Updated.
This change is