Add NMSM Pipeline contact elements for use in Moco

[SpencerTWilliams]

Brief summary of changes

This fork includes the new MeyerFregly2016Muscle and completes the implementation of the MeyerFregly2016Force included in the StationPlaneContactForce class.

The MeyerFregly2016Muscle is intended to be used without tendon compliance.

Testing I've completed

I've successfully compiled OpenSim on Ubuntu with these changes. However, I have not been able to test using the new elements.

Looking for feedback on...

I ran into an error while trying to compile a test for the MeyerFregly2016Muscle by modifying testMocoActuators.cpp:

/home/compiler/opensim-workspace/opensim-core-source/OpenSim/Moco/Test/testMocoActuators.cpp:95:26: error: expected type-specifier before ‘MeyerFregly2016Muscle’
   95 |         auto* actu = new MeyerFregly2016Muscle();
      |                          ^~~~~~~~~~~~~~~~~~~~~

I had the same error if I replaced the auto with MeyerFregly2016Muscle, so I think there may be an issue with including the new muscle as a type, even though the code files for the muscle compile correctly. Are there any issues with how I included the muscle in linking or registering the type in 2063d6f?

CHANGELOG.md (choose one)

• updated, but needs a merge conflict resolved I don't have permission for.

---

This change is 

[nickbianco]

Thanks for creating the PR @SpencerTWilliams! I've left a handful of comments to get started. My biggest question right now is whether or not we should create a separate class the muscle or if it is possible to use DeGrooteFregly2016Muscle, since the implementations look very similar. Could you list the primary differences between the two classes? I'd like to get a better understanding before I review.

I ran into an error while trying to compile a test for the MeyerFregly2016Muscle by modifying testMocoActuators.cpp:

I think you might have forgotten an include statement in the test file? Feel free to push the test changes if you want me to take a look.

Finally, we use Reviewable for code reviews (click the purple button in the PR description). Let me know if you have any questions on how it works.

Reviewed 6 of 8 files at r1, all commit messages.
Reviewable status: 6 of 8 files reviewed, 10 unresolved discussions (waiting on @SpencerTWilliams)

---

CHANGELOG.md line 63 at r1 (raw file):

- Fixed bug in `report.py` preventing plotting multiple MocoParameter values. (#3808)
- Added SynergyController, a controller that computes controls for a model based on a linear combination of a set of Input control signals and a set of synergy vectors. (#3796)
- Added `MeyerFregly2016Muscle` and completed the implementation of the `MeyerFregly2016Force` included in the `StationPlaneContactForce` class to support NMSM Pipeline-equivalent muscle and contant models in Moco. 

Typically we append the PR number in changelog entries:

Suggestion:

- Added `MeyerFregly2016Muscle` and completed the implementation of the `MeyerFregly2016Force` included in the `StationPlaneContactForce` class to support NMSM Pipeline-equivalent muscle and contant models in Moco. (#3877)

---

OpenSim/Actuators/MeyerFregly2016Muscle.h line 41 at r1 (raw file):

This muscle implementation is based on the previously implemented 
DeGrooteFregly2016Muscle.

At first glance, this implementation looks very similar to DeGrooteFregly2016Muscle, which makes me wonder again if it's worth making these muscle separate classes. I think it would be worth enumerating the differences between the curves in DeGrooteFregly2016Muscle and the ones here to get a bette sense if an entirely separate class is necessary.

---

OpenSim/Moco/Components/StationPlaneContactForce.h line 6 at r1 (raw file):

 * OpenSim: StationPlaneContactForce.h                                        *
 * -------------------------------------------------------------------------- *
 * Copyright (c) 2017 Stanford University and the Authors                     *

Might as well update the year too:

Suggestion:

 * Copyright (c) 2024 Stanford University and the Authors                     *

---

OpenSim/Moco/Components/StationPlaneContactForce.h line 146 at r1 (raw file):

B. J. (2016). Muscle Synergies Facilitate Computational Prediction of
Subject-Specific Walking Motions. Frontiers in Bioengineering and
Biotechnology, 4, 1055–27. http://doi.org/10.3389/fbioe.2016.00077 

Now that this contact element is being fleshed out, it would be good to expand the class description here (and possibly update the reference, if relevant). Consider explaining how the contact forces are compute and include equations. Refer to MocoControlGoal on how to write equations using Doxygen comments.

---

OpenSim/Moco/Components/StationPlaneContactForce.h line 178 at r1 (raw file):

        const SimTK::Real y = pos[1];
        const SimTK::Real velNormal = vel[1];
        SimTK::Real velSliding = sqrt(vel[0] * vel[0] + vel[2] * vel[2]);

The OpenSim convention is that y-direction is vertical, but it would be good to note this somewhere in the class documentation above.

---

OpenSim/Moco/Components/StationPlaneContactForce.h line 185 at r1 (raw file):

        const SimTK::Real h = 1e-3;
        const SimTK::Real c = 5e-4;
        const SimTK::Real ymax = 1e-2;

It would be good to explain these hardcoded parameters in the class docs.

---

OpenSim/Moco/Components/StationPlaneContactForce.h line 198 at r1 (raw file):

        if (SimTK::isNaN(Fspring) || SimTK::isInf(Fspring)) {
            Fspring = 0;
        }

I'm wondering if this if-statment should be removed. I'm not sure if we added these originally or borrowed them from the Meyer et al. implementation.

---

OpenSim/Moco/Components/StationPlaneContactForce.h line 211 at r1 (raw file):

        if (velSliding < 1e-10) {
                velSliding = 0;
        }

Why not just let this be near zero? We try to avoid if-statements like this, as they can introduce discontinuties in the model.

---

OpenSim/Moco/Components/StationPlaneContactForce.h line 217 at r1 (raw file):

        if (SimTK::isNaN(horizontalForce) || SimTK::isInf(horizontalForce)) {
            horizontalForce = 0;
        }

Similar to my comment above: it would be ideal to not require this if-statement. I'm also wary about hiding NaN or Inf values that might indicate deeper issues with the model.

---

OpenSim/Moco/Components/StationPlaneContactForce.h line 219 at r1 (raw file):

        }
        
        const SimTK::Real slipOffset = 1e-4;

What is this? Should it be a property or grouped with the other hardcoded parameters above?

[nickbianco]

@SpencerTWilliams, just checking in on this PR. Let me know if you have any questions about the review.

[SpencerTWilliams]

Thanks for the suggestions! We found the documentation for our contact force equation and the meanings of the constants, so I'll push my changes with the updated class description.

The MeyerFregly2016Muscle class is based on DeGrooteFregly2016Muscle and is very similar, but it uses different active and passive force-length and force-velocity curves with different numbers of coefficients. I made this class since we want a muscle that is completely consistent with the muscle model in our code, though it will look similar to the already existing DeGrooteFregly2016Muscle.

For your comments on the if statements, I included those to guarantee that the contact force elements work exactly the same as they do in our code. We haven't had issues with discontinuities, but if these statements will cause problems for Moco, we can try removing them. The slipOffset constant likely accounts for any of these potential issues already as it prevents dividing by zero when calculating horizontal forces. This constant is not a property, so I'll move it up to be with the other hardcoded values for clarity.

[nickbianco]

Thanks for the update @SpencerTWilliams!

The MeyerFregly2016Muscle class is based on DeGrooteFregly2016Muscle and is very similar, but it uses different active and passive force-length and force-velocity curves with different numbers of coefficients.

Could you be more specific about these differences? My understanding is that the curves in the MeyerFregly2016Muscle entirely different, or do they use the same general formulations (e.g., sum of Gaussians for the force-length curve) with slightly different coefficients? If it's the latter, then I'm somewhat inclined to not introduce a new muscle class.

For your comments on the if statements, I included those to guarantee that the contact force elements work exactly the same as they do in our code. We haven't had issues with discontinuities, but if these statements will cause problems for Moco, we can try removing them.

I'm generally concerned about hiding Inf or NaN values in a component accessible to the wider userbase. If your force is produce Inf or NaN values something should probably be changed about your model, initial guess, etc. Zeroing out velSliding in that if-statement will introduce a discontinuity, but that doesn't mean your probably won't converge, especially with finite differences. I would want to know if these if-statements are merely safeguards or if they provide some functional benefit before agreeing to leave them in.

[nickbianco]

Also, just a heads up that the Force API will be changing based on #3891 soon. It might make sense to break up these two changes: we can use this PR for the contact force addition and then (if needed) open a second PR for the new muscle class.

[SpencerTWilliams]

I looked at the curve differences again, and the active force-length curves do have a similar formulation. However, the passive force-length and force-velocity curves are very different between the two muscles, with different numbers of coefficients. I think these differences would require a new muscle class.

I'll test our model more to see whether it's possible to remove the Inf and NaN values. I think this check would be safe to remove at this point, so I'll remove it if it isn't possible to get Inf and NaN values from actual double inputs.

Let me know if it ends up making more sense to make a different PR for the muscle. Would I need to change the contact force class to account for the Force API changes?

[adamkewley]

The force API changes are non-breaking so, in general, code can still override+use the "raw" computeForce APIs. The ForceProducer + ForceConsumer APIs are extra - they're to facilitate other systems, such as visualizers, force reporters, and so on.

Specifically for this PR, I don't think you need to change what you're doing in order to receive the benefits of the new API. I've already ensured that the new API is rolled out to OpenSim::PathActuator, which OpenSim::Muscle inherits from:

• https://github.com/opensim-org/opensim-core/pull/3891/files#diff-5531d7f57434db355a0f850f1439e04caeab955f43dc90a8b5ee22fb740c4ef5R201

Which ultimately means that any forces/actuations produced by MeyerFregly2016Muscle muscle class will automatically be report-able, visualize-able, etc.

Your changes to the StationPlaneContactForce look like they're separate to the API changes I made in order to ensure StationPlaneContactForce emits its forces as point forces to a ForceConsumer (so that they could potentially be visualized). The change I made is here:

• https://github.com/opensim-org/opensim-core/pull/3891/files#diff-70c5e57686d093bb16e1765aeb5cb014cb317a50baaef60508ba2c8c83812faaR76

But it's far away enough from your modifications that you may not even see a merge error.

[nickbianco]

@adamkewley, thanks for the info!

@SpencerTWilliams, yes, let's focus on the contact force additions here and have a separate PR for the muscle.

[SpencerTWilliams]

I've pushed my documentation and changes based on your suggestions. The NaN and Inf checks have been deleted from the force element. Should I go ahead and make the second PR for the muscle?

[nickbianco]

The updates look good overall! I just have one comment to address since I'm still unsure about the if-statement for velSliding.

Should I go ahead and make the second PR for the muscle?

Yes, go for it!

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: 6 of 8 files reviewed, 5 unresolved discussions (waiting on @SpencerTWilliams)

---

CHANGELOG.md line 26 at r4 (raw file):

  means that API users can now now ask many `Force` components what forces they produce (see #3891 for a
  comprehensive overview).
- Added `MeyerFregly2016Muscle` and completed the implementation of the `MeyerFregly2016Force` included in the `StationPlaneContactForce` class to support NMSM Pipeline-equivalent muscle and contant models in Moco. (#3877)

Update the changelog when removing MeyerFregly2016Muscle from this PR.

---

OpenSim/Moco/Components/StationPlaneContactForce.h line 211 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Why not just let this be near zero? We try to avoid if-statements like this, as they can introduce discontinuties in the model.

I'm still unsure about keeping this if-statement. What would be the downside of removing it?

[SpencerTWilliams]

It looks like it's okay to remove that if-statement, so I took it out. I'll make the new muscle PR now.

[nickbianco]

@SpencerTWilliams, I've left a few minor formatting suggestions. Now that the component looks complete and the muscle is no longer in the PR I have a few additional thoughts:

1. It looks like there are no testing changes currently in the PR. It looks like the contact force is not tested at all in testMocoContact. At the very least, the contact model should be added to the test there. It might make sense to beef up the tests for the StationPlaneContactForce elements, we have not looked at them in a while.
2. Now that it is being formalized, we should probably move StationPlaneContactForce.h/.cpp next to SmoothSphereHalfSpaceForce under OpenSim/Simulation/Model. The OpenSim/Moco/Components folder is mostly reserved for internal classes. (Note that OSIMMOCO_API should be changed to OSIMSIMULATION_API when moved.) I would also move AckermannVanDenBogert2010Force to the bottom of the file so that MeyerFregly2016Force is the first concrete class under StationPlaneContactForce.
3. As a follow on from 2., now that MeyerFregly2016Force is becoming a proper OpenSim force element, we should probably also spruce up the base class StationPlaneContactForce. This would involve adding some class documentation and addressing the TODO comments regarding caching. I can address these TODOs in a later PR, but if you want to take a stab at the class documentation that would be great.

Reviewed 8 of 8 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @SpencerTWilliams)

---

CHANGELOG.md line 26 at r5 (raw file):

  means that API users can now now ask many `Force` components what forces they produce (see #3891 for a
  comprehensive overview).
- Completed the implementation of the `MeyerFregly2016Force` included in the `StationPlaneContactForce` class to support NMSM Pipeline-equivalent contant models in Moco. (#3877)

Suggestion:

- Completed the implementation of the `MeyerFregly2016Force` included in the `StationPlaneContactForce` class to support NMSM Pipeline-equivalent contact models in Moco. (#3877)

---

OpenSim/Moco/Components/StationPlaneContactForce.h line 153 at r5 (raw file):

Biotechnology, 4, 1055–27. http://doi.org/10.3389/fbioe.2016.00077 

Following OpenSim convention, this contact element assumes that the y direction 

Suggestion:

Following OpenSim convention, this contact element assumes that the y-direction

---

OpenSim/Moco/Components/StationPlaneContactForce.h line 197 at r5 (raw file):

with a tanh function) and viscous (modeled with a linear function) friction 
models may be used. 
 */

Suggestion:

models may be used. */