Add calc methods and Outputs for whole-body momentum to Model
#3487
Conversation
|
The failiing Windows builds should be fixed by #3486. |
OpenSim/Simulation/Model/Model.cpp
Outdated
| * | ||
| */ | ||
| SimTK::Vec3 Model::calcAngularMomentum(const SimTK::State& s) const { | ||
| return getMatterSubsystem().calcSystemCentralMomentum(s).get(0); |
There was a problem hiding this comment.
Mostly a style suggestion, but may make future maintenance simpler. (and same for calcLinearMomentum if accepted)
| return getMatterSubsystem().calcSystemCentralMomentum(s).get(0); | |
| return calcMomentum(s).get(0); |
OpenSim/Simulation/Model/Model.cpp
Outdated
| } | ||
| /** |
There was a problem hiding this comment.
These Doxygen-style comments don't get picked up from the .cpp file (only the headers). See, for example, the (blank) documentation for Model::calcMassCenterPosition() (notably, the potentially useful comment "Return the position vector of the system mass center, measured from the Ground origin, and expressed in Ground." does not appear). Perhaps at least put the Doxygen comments for your new methods in the header file, and create an Issue to move the others.
There was a problem hiding this comment.
Good catch! I can move all these over.
| */ | ||
| SimTK::SpatialVec Model::calcMomentum(const SimTK::State &s) const | ||
| { | ||
| return getMatterSubsystem().calcSystemCentralMomentum(s); |
There was a problem hiding this comment.
Following the examples above (e.g., Model::calcMassCenterAcceleration()), shouldn't this line be preceded by
getMultibodySystem().realize(s, Stage::Velocity);There was a problem hiding this comment.
(I think the realize() calls should be here since an API user might call the method directly. IIRC, calling realize() isn't [supposed to be] expensive if the State has already been realized to the required Stage.)
There was a problem hiding this comment.
I thought it was weird these calls were inside the calc-COM methods, but I think you're right, the Stage is probably cached and realize won't re-realize if it is. (I also tried taking out the realize calls in the calc-COM methods and it broke some tests, so there's that.)
…omentum calc methods
|
Thanks @tkuchida! I made some updates based on your comments. |
|
Thanks @tkuchida! |
Fixes issue #3474
Brief summary of changes
This PR introduces methods and
Outputs for computing whole-body momentum (about the center of mass, expressed in ground) onModel.Testing I've completed
Successfuly ran an optimization problem using
MocoOutputGoalwith the new momentumOutputs.Looking for feedback on...
I noticed that
calcMassCenterPosition,calcMassCenterVelocity, andcalcMassCenterAccelerationall callrealizewithin each method. This might lead torealizebeing called twice when usingOutputs, sinceOutputrequire aSimTK::Stagedependency. I think that this means thatrealizewill automatically be called for theseOutputs, or at least an "invalid stage" error will be thrown. Either way, I don't think therealizecalls should be baked into these methods, so I'm inclined to remove them, which I could append to this PR.CHANGELOG.md (choose one)
This change is