Use ForceConsumer point force interface for applying Scholz2015GeometryPath forces

[nickbianco]

Fixes issue #4276

Brief summary of changes

This PR replaces usages of ForceConsumer::consumeBodySpatialVec() with ForceConsumer::consumePointForce() for applying forces in Scholz2015GeometryPath. This will enable downstream applications (e.g., OpenSim Creator) to visualize forces applied along muscle paths.

Requires simbody/simbody#857 to be merged in Simbody first before merging here. Done.

Testing I've completed

Ran testScholz2015GeometryPath.cpp test suite.

Looking for feedback on...

CHANGELOG.md (choose one)

• updated.

---

This change is 

[nickbianco]

This is ready for review. @adamkewley you game?

[adamkewley]

👍 It's all fine - my comments aren't going to change behavior etc. but if you have some spare time maybe consider them. Good work 😄

@adamkewley reviewed 2 files and all commit messages, and made 6 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on nickbianco).

---

OpenSim/Simulation/Model/Scholz2015GeometryPath.cpp line 243 at r1 (raw file):
calc methods imply that something is being calculated and returned by value. Looking at simbody, the signature is indeed Transform calcCurveSegmentInitialFrenetFrame( State& state, ix) const;. Therefore, the usage of a const reference here is a little dangerous.

Luckily, the C++ specification saves you here:

Whenever a reference is bound to a temporary object or to a subobject thereof, the lifetime of the temporary object is extended to match the lifetime of the reference (check temporary object lifetime exceptions), where the temporary object or its subobject is denoted by one of following expression.

https://en.cppreference.com/cpp/language/reference_initialization#Lifetime_of_a_temporary

I'd still make it clear that values are being played with, though, because reference lifetime extension gets a little sticky once you start doing stuff like returning the reference from the function and so on - purely paranoid, maybe, ask around.

---

OpenSim/Simulation/Model/Scholz2015GeometryPath.cpp line 253 at r1 (raw file):

        // of the tangent.
        const SimTK::UnitVec3& t_P = -X_GP.R().getAxisUnitVec(SimTK::XAxis);
        const SimTK::UnitVec3& t_Q = X_GQ.R().getAxisUnitVec(SimTK::XAxis);

Purely an observation:

I don't really know about the Frenet frame stuff - feels like a higher-level method for getting these unit vectors would be better than messing around with frenet frames (similar to how you did it for the other pathpoints).

And, again, getAxisUnitVec feels like a method that needs to materialize the unit vector from the rotation (although, of course, it might do so by plucking the unit vector out of an orthonormal matrix) so I'd check the signature:

const UnitVec<P,1> getAxisUnitVec(CoordinateDirection dir) const {

---

OpenSim/Simulation/Model/Scholz2015GeometryPath.cpp line 256 at r1 (raw file):

        // Transform the Frenet frame positions from the ground frame G to the
        // obstacle body frame B, to conform with the ForceConsumer interface.

Is it transforming it to the body frame? Or is it transforming it to the contact geometry's frame, which can be any sort of frame, including a body frame, ground frame, offset frame, station-defined frame, etc.?

---

OpenSim/Simulation/Model/Scholz2015GeometryPath.cpp line 262 at r1 (raw file):

        const SimTK::Vec3& p_GP = X_GP.p();
        const SimTK::Vec3& p_GQ = X_GQ.p();
        SimTK::Vec3 p_BP = ground.findStationLocationInAnotherFrame(

Nit: can const these also, just to align everything.

---

dependencies/CMakeLists.txt line 188 at r1 (raw file):

              DEFAULT    ON
              GIT_URL    https://github.com/simbody/simbody.git
              GIT_TAG    80a3f10a6daa26c8aaf18f2c672cf385b227e62c

Quick note: remember to change this if this PR was built on an experimental branch or something

[nickbianco]

Thanks @adamkewley! Made a few changes based on your comments.

@nickbianco made 6 comments.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on adamkewley).

---

dependencies/CMakeLists.txt line 188 at r1 (raw file):

Previously, adamkewley (Adam Kewley) wrote…

Quick note: remember to change this if this PR was built on an experimental branch or something

This is the latest commit on Simbody's master branch, after the recent PR merge, so we're all good.

---

OpenSim/Simulation/Model/Scholz2015GeometryPath.cpp line 243 at r1 (raw file):

Previously, adamkewley (Adam Kewley) wrote…

calc methods imply that something is being calculated and returned by value. Looking at simbody, the signature is indeed Transform calcCurveSegmentInitialFrenetFrame( State& state, ix) const;. Therefore, the usage of a const reference here is a little dangerous.

Luckily, the C++ specification saves you here:

Whenever a reference is bound to a temporary object or to a subobject thereof, the lifetime of the temporary object is extended to match the lifetime of the reference (check temporary object lifetime exceptions), where the temporary object or its subobject is denoted by one of following expression.

https://en.cppreference.com/cpp/language/reference_initialization#Lifetime_of_a_temporary

I'd still make it clear that values are being played with, though, because reference lifetime extension gets a little sticky once you start doing stuff like returning the reference from the function and so on - purely paranoid, maybe, ask around.

Good catch, I did not intend to make these const reference. Fixed now. But good to know that C++ tidbit.

---

OpenSim/Simulation/Model/Scholz2015GeometryPath.cpp line 253 at r1 (raw file):

Previously, adamkewley (Adam Kewley) wrote…

Purely an observation:

I don't really know about the Frenet frame stuff - feels like a higher-level method for getting these unit vectors would be better than messing around with frenet frames (similar to how you did it for the other pathpoints).

And, again, getAxisUnitVec feels like a method that needs to materialize the unit vector from the rotation (although, of course, it might do so by plucking the unit vector out of an orthonormal matrix) so I'd check the signature:

const UnitVec<P,1> getAxisUnitVec(CoordinateDirection dir) const {

This is based on the internal helper method getTangent() in CableSpan. When given a CoordinateAxis, getAxisUnitVec indeed just returns a column of the rotation matrix.

We could maybe expose the getTangent() helper at some point. But I think this is fine for now.

---

OpenSim/Simulation/Model/Scholz2015GeometryPath.cpp line 256 at r1 (raw file):

Previously, adamkewley (Adam Kewley) wrote…

Is it transforming it to the body frame? Or is it transforming it to the contact geometry's frame, which can be any sort of frame, including a body frame, ground frame, offset frame, station-defined frame, etc.?

Another good catch. Yes, it is the the contact geometry's frame, which can generally be any frame. Updated.

---

OpenSim/Simulation/Model/Scholz2015GeometryPath.cpp line 262 at r1 (raw file):

Previously, adamkewley (Adam Kewley) wrote…

Nit: can const these also, just to align everything.

Done.

[adamkewley]

- minor comment

@adamkewley reviewed 1 file and all commit messages, made 2 comments, and resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on nickbianco).

---

OpenSim/Simulation/Model/Scholz2015GeometryPath.cpp line 253 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

This is based on the internal helper method getTangent() in CableSpan. When given a CoordinateAxis, getAxisUnitVec indeed just returns a column of the rotation matrix.

We could maybe expose the getTangent() helper at some point. But I think this is fine for now.

👍 It's still a reference (should be a value) but, due to the spec, should behave itself. Just mentioning again.

[nickbianco]

@nickbianco made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on nickbianco).

---

OpenSim/Simulation/Model/Scholz2015GeometryPath.cpp line 253 at r1 (raw file):

Previously, adamkewley (Adam Kewley) wrote…

👍 It's still a reference (should be a value) but, due to the spec, should behave itself. Just mentioning again.

Just for my understanding: the signature being used here is const ColType& getAxisUnitVec(CoordinateAxis axis) const, which is return the column by reference, not value. So I think the left-side expression (lvalue?) (e.g., const SimTK::UnitVec3& t_P) is appropriate here. Unless I'm missing something.

[adamkewley]

@nickbianco ah, fair enough - disregard my comment!

[nickbianco]

Merging. Thanks @adamkewley!