opensim-org / opensim-core

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

Feature: add `ForceProducer` and `ForceConsumer` APIs #3891

Closed adamkewley closed 2 months ago

adamkewley commented 2 months ago

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

Overview

Key features:

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:

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

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

Looking for feedback on...

CHANGELOG.md

Updated.


This change is Reviewable

adamkewley commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

adamkewley commented 2 months ago

Ok, now it's ready for review :wink:

adamkewley commented 2 months ago

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

adamkewley commented 2 months ago

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.