Skip to content

Feature: add ForceProducer and ForceConsumer APIs#3891

Merged
adamkewley merged 25 commits intomainfrom
feature_ForceProducerConsumer
Sep 5, 2024
Merged

Feature: add ForceProducer and ForceConsumer APIs#3891
adamkewley merged 25 commits intomainfrom
feature_ForceProducerConsumer

Conversation

@adamkewley
Copy link
Contributor

@adamkewley adamkewley commented Aug 26, 2024

This PR adds ForceProducer, ForceConsumer, and ForceApplier and rolls them out to as many OpenSim::Force implementations as feasible.

Overview

  • ForceProducer is an abstract OpenSim::Force that downstream classes can use, rather than Force, to emit their forces one-by-one to a ForceConsumer.
  • ForceConsumer is an abstract class with consumption methods (e.g. consumePointForce) that directly mirror the of OpenSim::Force equivalents (e.g. Force::applyForceToPoint).
  • ForceApplier is a ForceConsumer that applies each consumed force to a SimTK::Vector_<SimTK::SpatialForce> of body forces and a SimTK::Vector of mobility (generalized) forces.

Key features:

  • ForceProducer's virtual one-by-one API is in contrast to OpenSim::Force, where forces are added by directly modifying the physics engine's bodyForces and generalizedForces vectors 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 equivalent OpenSim::Force API methods. This makes it easier to port OpenSim::Force code to the ForceConsumer API. The ForceConsumer API also separates "body forces" from "point forces", so that concrete ForceProducers are 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 of OpenSim::Force::computeForce, which internally uses ForceApplier apply the one-at-a-time forces to the vectors that OpenSim::Force::computeForce uses. This makes ForceProducer backwards-compatible with any existing code that handles OpenSim::Forces.
  • The API's compatibility means no breaking changes for downstream code - save for a small, probably easy-to-update, change to the virtual API of AbstractGeometryPath.

The ForceConsumer API

The ForceConsumer API is an abstract class that has a public API that consumes four types of forces:

class ForceConsumer {
public:
    void consumeGeneralizedForce(const SimTK::State&, const Coordinate&, double);
    void consumeBodySpatialVec(const SimTK::State&, const PhysicalFrame&, const SimTK::SpatialVec&);
    void consumeTorque(const SimTK::State&, const PhysicalFrame&, const SimTK::Vec3&);
    void consumePointForce(const SimTK::State&, const PhysicalFrame&, const SimTK::Vec3&, const SimTK::Vec3&);

    // (see diff for the entire definition)
};

Concrete implementations of ForceConsumer may override each of the corresponding virtual void implConsume* methods in order to consume that type of force. The default implementation of each implConsume* 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 ForceProducer API

The ForceProducer API is an abstract class that exposes produceForces:

class ForceProducer {
public:
    void produceForces(const SimTK::State&, ForceConsumer&) const;

    // supplies a default implementation of `OpenSim::Force::computeForce`
    void computeForce(const SimTK::State&, SimTK::Vector_<SimTK::SpatialVec>&, SimTK::Vector&) const override;

    // (see diff for the entire definition)
};

Concrete implementations can then inherit from ForceProducer and supply an implProduceForces in which the concrete implementation calls (e.g.) ForceConsumer::consumePointForce. The default ForceProducer::computeForce implementation uses ForceApplier to satisfy the OpenSim::Force API. It's overrided, rather than finald, because there
is legacy code in OpenSim that does extra stuff after computeForce is called (e.g. resetting state variables). An ideal implementation would final computeForce.

Motivation (why add this?)

The OpenSim::Force API isn't easy for downstream code, such as OpenSim Creator's, to hook into. Because the OpenSim::Force API couples force generation/production with application. Because of this, downstream code has to:

  • Allocate zero-initialized vectors of body+generalized+particle forces, to emulate acting as a simbody engine.
  • Use a ForceAdaptor to mutate those vectors.
  • Re-scan the vectors to figure out which forces affected which bodies in the model.

The OpenSim::Force API also erases point-force information. At the OpenSim::Force layer 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 visualizing ExternalForce, PrescribedForce, and GeometryPath).

Brief Summary of Changes

  • Added ForceProducer
  • Added ForceConsumer
  • Added ForceApplier
  • Added basic test suite for ForceProducer
  • Rolled out ForceConsumer and ForceProducer APIs to all existing OpenSim classes that were previously using the Force API.
    • This is so that almost any force in opensim-core can be queried using the ForceProducer API
    • It also ensures the API is actually robust enough to rollout to a wide variety of Force implementations
  • BREAKING (virtual API) CHANGE: Replaced virtual AbstractGeometryPath::addInEquivalentForces with a virtual AbstractGeometryPath::produceForces API:
    • The change is to facilitate integrating ForceProducers with AbstractGeometryPath (e.g. necessary for PathActuator and similar).
    • For backwards compatiblity, added a concrete AbstractGeometryPath::addInEquivalentForces function that uses the new produceForces API internally.
    • Although this is a BREAKING, change, it only breaks the virtual, not public, API of 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 use ForceConsumer is an easy change.
  • Added ForceProducer and ForceConsumer APIs to the SWIG bindings
  • Added any additional template instantiations to the SWIG bindings
  • Tried to find+replace relevant comments that say "applies force" to instead say "produces/computes force". This is to make it clear that the forces aren't actually applied by the methods anymore.

No 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

  • Added a basic unit test suite for ForceProducer to ensure it behaves as expected
  • Added a unit test that compares the mutations applied by ForceApplier (via ForceProducer::computeForce) to the mutations caused by a "typical" Force usage.
  • Almost all 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...

  • Design direction, usefulness, rollout, etc.

CHANGELOG.md

Updated.


This change is Reviewable

@adamkewley adamkewley force-pushed the feature_ForceProducerConsumer branch from db15b54 to 9dfc42f Compare August 27, 2024 11:24
@adamkewley
Copy link
Contributor Author

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)

@adamkewley adamkewley force-pushed the feature_ForceProducerConsumer branch from abda563 to 484dbaa Compare September 2, 2024 07:21
@adamkewley
Copy link
Contributor Author

testMocoInverse seems to be broken on Windows (but is otherwise fine on Linux+Mac O_o) - I'm assuming a slight numeric difference has been introduced by the rewrite of GeometryPath::implProduceForces (it was cleaned up from addInEquivalentForces).

@adamkewley
Copy link
Contributor Author

After reverting the GeometryPath implementation to basically be exactly the same as the existing addInEquivalentForces, but now using the ForceConsumer API, I can get testMocoInverse to pass - it's probably a numeric instability that needs to be carefully handled in a separate PR (cleaning up that part of the code would be useful, in general, for point-force visualization).

@adamkewley
Copy link
Contributor Author

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 Force API mutations to the ForceConsumer API mutations to ensure that they are like-for-like - and actually edit the force vectors.

I'll un-WIP this and try to wrangle someone for a review.

@adamkewley adamkewley changed the title [WIP] Feature: add ForceProducer and ForceConsumer APIs Feature: add ForceProducer and ForceConsumer APIs Sep 3, 2024
@adamkewley adamkewley changed the title Feature: add ForceProducer and ForceConsumer APIs WIP: Feature: add ForceProducer and ForceConsumer APIs Sep 3, 2024
@adamkewley
Copy link
Contributor Author

When self-reviewing the diff I found a few things that need to be re-thought:

  • The appliesForce member from Force should only be checked by ForceApplier. ForceProducers should unconditionally produce their forces when directly requested for them. If there's a desire to disable the ability to even ask a ForceProducer to produce its forces (side-effect free), then a new property called producesForces should be added. This is to discriminate between "produces forces into a consumer" from "applies forces to a multibody system"
  • The Force API changes (protected --> public) aren't strictly necessary because ForceApplier directly uses the SimbodyMattterSubsystem API.

Copy link
Contributor Author

@adamkewley adamkewley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was moved upward to match its position in the corresponding header.

return force;
}

//==============================================================================
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a (known) API break:

  • The original implementation marked AbstractGeometryPath::addInEquivalentForces as virtual, 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 - provided produceForces implementations are logically equivalent
  • Therefore, the break shouldn't break downstream code that uses AbstractGeometryPath::addInEquivalentForces - only concrete implementations that override it need to change, and the change is usually very minor (call forceConsumer.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()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@adamkewley adamkewley changed the title WIP: Feature: add ForceProducer and ForceConsumer APIs Feature: add ForceProducer and ForceConsumer APIs Sep 3, 2024
@adamkewley
Copy link
Contributor Author

Ok, now it's ready for review 😉

Copy link
Contributor

@pepbos pepbos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice and useful.

Two small comments but LGTM.

@adamkewley
Copy link
Contributor Author

Thanks for reviewing, @pepbos

@nickbianco - this PR also impacts the AbstractGeometryPath API slightly (it not "produces" forces rather than addInEquivalentForces). Is that an acceptable change (I've ported FunctionBasedPath etc. to make it compatible).

If so, and if there's no further review comments, I'll go ahead and merge this

Copy link
Member

@nickbianco nickbianco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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).

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's

OpenSim/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 testMocoInverse was 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…

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).

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.

Copy link
Contributor Author

@adamkewley adamkewley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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::ReferencePtr here? 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…

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.

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.

@adamkewley
Copy link
Contributor Author

adamkewley commented Sep 5, 2024

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 (main) branch of OSC so that I can start visualizing force vectors etc. using the API - I imagine that doing so will probably kick up some circumstances that require additional PRs to fix. For example, GeometryPath is producing two tension force vectors per path point - I anticipate that it's nicer to visualize the resultant vector, which would require rewriting that function a little bit. I also anticipate similar changes might be nice in other force components (e.g. contact forces would be lovely to visualize), but I don't know which/where until I start attaching a visualizer to the API.

Cheers to @nickbianco and @pepbos for reviewing this.

@adamkewley adamkewley merged commit 7c2c9de into main Sep 5, 2024
@adamkewley adamkewley deleted the feature_ForceProducerConsumer branch September 5, 2024 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants