Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement AbstractPath and related API changes to support multiple path types #3535

Merged
merged 25 commits into from
Sep 7, 2023
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
34daa7a
First pass at implementing AbstractPath
nickbianco Aug 23, 2023
3d5c2ee
Update examples and tests after GeometryPath API changes
nickbianco Aug 24, 2023
e0f19be
Merge branch 'main' into wrapping_abstract_path
nickbianco Aug 25, 2023
81d1aee
Safely initialize GeometryPath for path-based objects
nickbianco Aug 25, 2023
b4f11fd
Update changelog // use updPath instead of upd_path
nickbianco Aug 26, 2023
b70b2fa
Update the Java bindings with path changes
nickbianco Aug 28, 2023
5e98a45
Revert Appearance to unnamed property
nickbianco Aug 29, 2023
4f81105
Add deprecated functions (e.g., getGeometryPath()) back in
nickbianco Aug 29, 2023
016d8bd
Reverting API changes pt. 2 + diff clean up
nickbianco Aug 29, 2023
bb4b8be
Reverting API changes pt. 3 + more diff clean up
nickbianco Aug 29, 2023
5a74515
More diff clean up
nickbianco Aug 29, 2023
ce3b0c4
Stop IDE from removing trailing whitespaces...
nickbianco Aug 29, 2023
c383f64
Rule of five; more code clean up
nickbianco Aug 30, 2023
e57c1f6
Handle unsupported path types when replacing muscles
nickbianco Aug 30, 2023
e53ea0b
Remove initGeometryPath() for now
nickbianco Aug 30, 2023
c3ac610
Backwards compatibility for deserializing GeometryPath
nickbianco Aug 30, 2023
df76b0a
Add abstraction for path copying
nickbianco Aug 31, 2023
f64e3cd
Move path properties back to public scope
nickbianco Aug 31, 2023
f6e7208
More reverted code
nickbianco Aug 31, 2023
2940c77
Remove unnecessary includes
nickbianco Aug 31, 2023
25ed001
Rename copyFrom() to assign(); change hasPath() to hasVisualPath()
nickbianco Sep 1, 2023
e9e0008
Move backwards compatibility for GeometryPath deserialization to Force
nickbianco Sep 1, 2023
507e4e1
Minor whitespace cleanup
nickbianco Sep 1, 2023
83577ea
Use AbstractProperty::assign() instead of a new virtual method
nickbianco Sep 6, 2023
83959bb
Flesh out changelog
nickbianco Sep 7, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ request related to the change, then we may provide the commit.

This is not a comprehensive list of changes but rather a hand-curated collection of the more notable ones. For a comprehensive history, see the [OpenSim Core GitHub repo](https://github.com/opensim-org/opensim-core).

v4.5
====
- Added `AbstractPath` which is a base class for `GeometryPath` and other path types (#3388).

v4.4.1
======
- IMU::calcGyroscopeSignal() now reports angular velocities in the IMU frame.
Expand Down
58 changes: 34 additions & 24 deletions OpenSim/Actuators/DeGrooteFregly2016Muscle.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include <OpenSim/Actuators/Thelen2003Muscle.h>
#include <OpenSim/Common/CommonUtilities.h>
#include <OpenSim/Simulation/Model/Model.h>
#include "OpenSim/Simulation/Model/GeometryPath.h"
#include "OpenSim/Common/STOFileAdapter.h"

using namespace OpenSim;
Expand Down Expand Up @@ -1021,31 +1022,40 @@ void DeGrooteFregly2016Muscle::replaceMuscles(
actu->set_ignore_activation_dynamics(
muscBase.get_ignore_activation_dynamics());

const auto& pathPointSet = muscBase.getGeometryPath().getPathPointSet();
auto& geomPath = actu->updGeometryPath();
for (int ipp = 0; ipp < pathPointSet.getSize(); ++ipp) {
auto* pathPoint = pathPointSet.get(ipp).clone();
const auto& socketNames = pathPoint->getSocketNames();
for (const auto& socketName : socketNames) {
pathPoint->updSocket(socketName)
.connect(pathPointSet.get(ipp)
.getSocket(socketName)
.getConnecteeAsObject());
// Copy the muscle's path.
Copy link
Contributor

Choose a reason for hiding this comment

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

Changes behavior/intent

The function is DeGrooteFregly2016Muscle::replaceMuscles. Apart from that function, DeGrooteFregly2016Muscle doesn't appear to rely on "point-based paths" directly, so it should work with function-based paths also? The refactor effectively breaks DeGrootFregly2016Muscle if it is given a non-point-based path because it makes no attempt to handle the else part of if (auto* pGeometryPath = dynamic_cast<GeometryPath*>(&path))

If that's the case, I'd recommend writing a generic CopyPath/CopyMuscle functions that can be used in cases where muscles want to copy this information around.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as below, I've added an else branch to catch unsupported path types. As we add new path types, we can add new elseif branches to included supported path types.

The generic functions for copying muscle information could be implemented and should probably live somewhere else (@aymanhab has suggested this before). I think that could be addressed in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd tackle copying centrally in this PR rather than leaving an else as a future exercise: there's a high chance it'll be forgotten and undetected until a user runs into it at runtime (e.g. by using DeGrooteFregly2016Muscle with a non-GeometryPath)

Plus, if copy assignment of paths is something that's in downstream code (e.g. as it is here) then, architecturally speaking, it makes sense that copying is tackled centrally and code is ported accordingly.

if (auto* pGeometryPath = muscBase.tryUpdPath<GeometryPath>()) {
const auto& pathPointSet = pGeometryPath->getPathPointSet();
for (int ipp = 0; ipp < pathPointSet.getSize(); ++ipp) {
auto* pathPoint = pathPointSet.get(ipp).clone();
const auto& socketNames = pathPoint->getSocketNames();
for (const auto& socketName : socketNames) {
pathPoint->updSocket(socketName)
.connect(pathPointSet.get(ipp)
.getSocket(socketName)
.getConnecteeAsObject());
}
actu->updGeometryPath().updPathPointSet()
.adoptAndAppend(pathPoint);
}
geomPath.updPathPointSet().adoptAndAppend(pathPoint);
}

const auto& pathWrapSet = muscBase.getGeometryPath().getWrapSet();
for (int ipw = 0; ipw < pathWrapSet.getSize(); ++ipw) {
auto* pathWrap = pathWrapSet.get(ipw).clone();
const auto& socketNames = pathWrap->getSocketNames();
for (const auto& socketName : socketNames) {
pathWrap->updSocket(socketName)
.connect(pathWrapSet.get(ipw)
.getSocket(socketName)
.getConnecteeAsObject());
const auto& pathWrapSet = pGeometryPath->getWrapSet();
for (int ipw = 0; ipw < pathWrapSet.getSize(); ++ipw) {
auto* pathWrap = pathWrapSet.get(ipw).clone();
const auto& socketNames = pathWrap->getSocketNames();
for (const auto& socketName : socketNames) {
pathWrap->updSocket(socketName)
.connect(pathWrapSet.get(ipw)
.getSocket(socketName)
.getConnecteeAsObject());
}
actu->updGeometryPath().updWrapSet()
.adoptAndAppend(pathWrap);
}
geomPath.updWrapSet().adoptAndAppend(pathWrap);
} else {
OPENSIM_THROW(Exception,
"Muscle '{}' contains supported path type {}.",
muscBase.getName(),
muscBase.getPath().getConcreteClassName())
}

std::string actname = actu->getName();
Expand Down Expand Up @@ -1074,14 +1084,14 @@ void DeGrooteFregly2016Muscle::extendPostScale(
const SimTK::State& s, const ScaleSet& scaleSet) {
Super::extendPostScale(s, scaleSet);

GeometryPath& path = upd_GeometryPath();
AbstractPath& path = updPath();
adamkewley marked this conversation as resolved.
Show resolved Hide resolved
if (path.getPreScaleLength(s) > 0.0)
{
double scaleFactor = path.getLength(s) / path.getPreScaleLength(s);
upd_optimal_fiber_length() *= scaleFactor;
upd_tendon_slack_length() *= scaleFactor;

// Clear the pre-scale length that was stored in the GeometryPath.
// Clear the pre-scale length that was stored in the path.
path.setPreScaleLength(s, 0.0);
}
}
2 changes: 1 addition & 1 deletion OpenSim/Actuators/McKibbenActuator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ void McKibbenActuator::computeForce(const SimTK::State& s,

double actuation = computeActuation(s);

getGeometryPath().addInEquivalentForces(s, actuation, bodyForces, generalizedForces);
getPath().addInEquivalentForces(s, actuation, bodyForces, generalizedForces);
}
//_____________________________________________________________________________
/**
Expand Down
4 changes: 2 additions & 2 deletions OpenSim/Actuators/Millard2012AccelerationMuscle.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -557,14 +557,14 @@ extendPostScale(const SimTK::State& s, const ScaleSet& scaleSet)

Super::extendPostScale(s, scaleSet);

GeometryPath& path = upd_GeometryPath();
AbstractPath& path = updPath();
if (path.getPreScaleLength(s) > 0.0)
{
double scaleFactor = path.getLength(s) / path.getPreScaleLength(s);
upd_optimal_fiber_length() *= scaleFactor;
upd_tendon_slack_length() *= scaleFactor;

// Clear the pre-scale length that was stored in the GeometryPath.
// Clear the pre-scale length that was stored in the path.
path.setPreScaleLength(s, 0.0);
}
}
Expand Down
4 changes: 2 additions & 2 deletions OpenSim/Actuators/Millard2012EquilibriumMuscle.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -598,14 +598,14 @@ extendPostScale(const SimTK::State& s, const ScaleSet& scaleSet)
{
Super::extendPostScale(s, scaleSet);

GeometryPath& path = upd_GeometryPath();
AbstractPath& path = updPath();
if (path.getPreScaleLength(s) > 0.0)
{
double scaleFactor = path.getLength(s) / path.getPreScaleLength(s);
upd_optimal_fiber_length() *= scaleFactor;
upd_tendon_slack_length() *= scaleFactor;

// Clear the pre-scale length that was stored in the GeometryPath.
// Clear the pre-scale length that was stored in the path.
path.setPreScaleLength(s, 0.0);
}
}
Expand Down
62 changes: 36 additions & 26 deletions OpenSim/Actuators/ModelFactory.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include <OpenSim/Simulation/SimbodyEngine/PinJoint.h>
#include <OpenSim/Simulation/SimbodyEngine/SliderJoint.h>
#include <OpenSim/Simulation/SimbodyEngine/WeldJoint.h>
#include <OpenSim/Simulation/Model/GeometryPath.h>
#include <OpenSim/Common/CommonUtilities.h>

using namespace OpenSim;
Expand Down Expand Up @@ -171,35 +172,44 @@ void ModelFactory::replaceMusclesWithPathActuators(OpenSim::Model &model) {

model.addForce(actu);

// For the connectee names in the PathPoints to be correct, we must add
// the path points after adding the PathActuator to the model.
const auto& pathPointSet = musc.getGeometryPath().getPathPointSet();
auto& geomPath = actu->updGeometryPath();
for (int ip = 0; ip < pathPointSet.getSize(); ++ip) {
auto* pathPoint = pathPointSet.get(ip).clone();
const auto& socketNames = pathPoint->getSocketNames();
for (const auto& socketName : socketNames) {
pathPoint->updSocket(socketName)
.connect(pathPointSet.get(ip)
.getSocket(socketName)
.getConnecteeAsObject());
// Copy the muscle's path.
Copy link
Contributor

Choose a reason for hiding this comment

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

This code's behavior/intent changes with the change.

The function is ModelFactory::replaceMusclesWithPathActuator. That should be fine with either point-based or function-based paths. The change here just entirely ignores not-point-based versions of paths, which means that the intent (i.e. to replace all muscles in a model with path actuators) is not upheld.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added an else branch to catch unsupported path types. As we add new path types, we can add new elseif branches to included supported path types.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't do this. it's a bad idea for a large C++ codebase to start having many if (downcastFailed) { throw ...; } code, because it makes maintaining the codebase long-term much harder. The compiler won't be able to help developers when new abstractions are added, for example, and implementors will be forced to look for all of these dynamic_casts that you are bodging in.

Either:

  • Make the abstraction have enough pieces to perform the replacement virtually (required, if you plan on having external code implement new AbstractPaths independently of OpenSim)
  • Add a visitor pattern to the abstraction, where the visitor pattern double-dispatches to a known set of path implementations (constrains downstream implementations to a limited set, but adds static checks that will trigger when new paths are added)

Then you can do what you're doing within the framework of those solutions, but only if the concrete implementation has no feasible way of working without points (e.g. the visitor would throw for non-point-based paths with "there's no way of doing this operation with a function-based path - sorry not sorry" or similar).

if (auto* pGeometryPath = musc.tryUpdPath<GeometryPath>()) {
// For the connectee names in the PathPoints to be correct, we must
// add the path points after adding the PathActuator to the model.
const auto& pathPointSet = pGeometryPath->getPathPointSet();
for (int ip = 0; ip < pathPointSet.getSize(); ++ip) {
auto* pathPoint = pathPointSet.get(ip).clone();
const auto& socketNames = pathPoint->getSocketNames();
for (const auto& socketName : socketNames) {
pathPoint->updSocket(socketName)
.connect(pathPointSet.get(ip)
.getSocket(socketName)
.getConnecteeAsObject());
}
actu->updGeometryPath().updPathPointSet()
.adoptAndAppend(pathPoint);
}
geomPath.updPathPointSet().adoptAndAppend(pathPoint);
}

// For the connectee names in the PathWraps to be correct, we must add
// the path wraps after adding the PathActuator to the model.
const auto& pathWrapSet = musc.getGeometryPath().getWrapSet();
for (int ipw = 0; ipw < pathWrapSet.getSize(); ++ipw) {
auto* pathWrap = pathWrapSet.get(ipw).clone();
const auto& socketNames = pathWrap->getSocketNames();
for (const auto& socketName : socketNames) {
pathWrap->updSocket(socketName)
.connect(pathWrapSet.get(ipw)
.getSocket(socketName)
.getConnecteeAsObject());
// For the connectee names in the PathWraps to be correct, we must
// add the path wraps after adding the PathActuator to the model.
const auto& pathWrapSet = pGeometryPath->getWrapSet();
for (int ipw = 0; ipw < pathWrapSet.getSize(); ++ipw) {
auto* pathWrap = pathWrapSet.get(ipw).clone();
const auto& socketNames = pathWrap->getSocketNames();
for (const auto& socketName : socketNames) {
pathWrap->updSocket(socketName)
.connect(pathWrapSet.get(ipw)
.getSocket(socketName)
.getConnecteeAsObject());
}
actu->updGeometryPath().updWrapSet()
.adoptAndAppend(pathWrap);
}
geomPath.updWrapSet().adoptAndAppend(pathWrap);
} else {
OPENSIM_THROW(Exception,
"Muscle '{}' contains supported path type {}.",
musc.getName(),
musc.getPath().getConcreteClassName())
}

musclesToDelete.push_back(&musc);
Expand Down
2 changes: 1 addition & 1 deletion OpenSim/Actuators/ModelFactory.h
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ class OSIMACTUATORS_API ModelFactory {
/// @name Modify a Model
/// @{

/// Replace muscles in a model with a PathActuator of the same GeometryPath,
/// Replace muscles in a model with a PathActuator of the same path,
/// optimal force, and min/max control defaults.
/// @note This only replaces muscles within the model's ForceSet.
static void replaceMusclesWithPathActuators(Model& model);
Expand Down
2 changes: 1 addition & 1 deletion OpenSim/Actuators/RigidTendonMuscle.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ void RigidTendonMuscle::calcMusclePotentialEnergyInfo(const SimTK::State& s,
void RigidTendonMuscle::calcFiberVelocityInfo(const State& s, FiberVelocityInfo& fvi) const
{
/*const MuscleLengthInfo &mli = */getMuscleLengthInfo(s);
fvi.fiberVelocity = getGeometryPath().getLengtheningSpeed(s);
fvi.fiberVelocity = getPath().getLengtheningSpeed(s);
fvi.normFiberVelocity = fvi.fiberVelocity /
(getOptimalFiberLength()*getMaxContractionVelocity());
fvi.fiberForceVelocityMultiplier =
Expand Down
1 change: 1 addition & 0 deletions OpenSim/Analyses/MuscleAnalysis.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
//=============================================================================
// INCLUDES
//=============================================================================
#include <OpenSim/Common/PropertyStrArray.h>
#include <OpenSim/Simulation/Model/Analysis.h>
#include <OpenSim/Simulation/Model/Muscle.h>
#include "osimAnalysesDLL.h"
Expand Down
4 changes: 3 additions & 1 deletion OpenSim/Common/XMLDocument.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,10 @@ using namespace std;
// 30516 for GeometryPath default_color -> Appearance
// 30517 for removal of _connectee_name suffix to shorten XML for socket, input
// 40000 for OpenSim 4.0 release 40000
// 40500 for OpenSim 4.5 release, moving 'GeometryPath' nodes under the 'path'
// node for supporting generic path types.

const int XMLDocument::LatestVersion = 40000;
const int XMLDocument::LatestVersion = 40500;
//=============================================================================
// DESTRUCTOR AND CONSTRUCTOR(S)
//=============================================================================
Expand Down
2 changes: 1 addition & 1 deletion OpenSim/ExampleComponents/HopperDevice.h
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ class OSIMEXAMPLECOMPONENTS_API HopperDevice : public ModelComponent {
void extendRealizeDynamics(const SimTK::State& s) const override {
const auto& actuator = getComponent<PathActuator>(get_actuator_name());
double level = fmin(1., getTension(s) / actuator.get_optimal_force());
actuator.getGeometryPath().setColor(s, SimTK::Vec3(level, 0.5, 0));
actuator.getPath().setColor(s, SimTK::Vec3(level, 0.5, 0));
}

}; // end of HopperDevice
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ class Device : public ModelComponent {
void extendRealizeDynamics(const SimTK::State& s) const override {
const auto& actuator = getComponent<PathActuator>("cableAtoB");
double level = fmin(1., getTension(s) / actuator.get_optimal_force());
actuator.getGeometryPath().setColor(s, SimTK::Vec3(0.1, level, 0.1));
actuator.getPath().setColor(s, SimTK::Vec3(0.1, level, 0.1));
}

}; // end of Device
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ class Device : public ModelComponent {
void extendRealizeDynamics(const SimTK::State& s) const override {
const auto& actuator = getComponent<PathActuator>("cableAtoB");
double level = fmin(1., getTension(s) / actuator.get_optimal_force());
actuator.getGeometryPath().setColor(s, SimTK::Vec3(0.1, level, 0.1));
actuator.getPath().setColor(s, SimTK::Vec3(0.1, level, 0.1));
}

}; // end of Device
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -747,7 +747,6 @@ void createLuxoJr(OpenSim::Model& model){
"back_extensor_right",
back_extensor_F0, back_extensor_lm0,
back_extensor_lts, pennationAngle);

backExtensorRight->addNewPathPoint("back_extensor_right_origin", *chest,
back_extensor_origin);
backExtensorRight->addNewPathPoint("back_extensor_right_insertion", *back,
Expand Down
68 changes: 68 additions & 0 deletions OpenSim/Simulation/Model/AbstractPath.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
/* -------------------------------------------------------------------------- *
* OpenSim: AbstractPath.cpp *
* -------------------------------------------------------------------------- *
* The OpenSim API is a toolkit for musculoskeletal modeling and simulation. *
* See http://opensim.stanford.edu and the NOTICE file for more information. *
* OpenSim is developed at Stanford University and supported by the US *
* National Institutes of Health (U54 GM072970, R24 HD065690) and by DARPA *
* through the Warrior Web program. *
* *
* Copyright (c) 2005-2023 Stanford University and the Authors *
* Author(s): Nicholas Bianco, Joris Verhagen, Adam Kewley *
* *
* Licensed under the Apache License, Version 2.0 (the "License"); you may *
* not use this file except in compliance with the License. You may obtain a *
* copy of the License at http://www.apache.org/licenses/LICENSE-2.0. *
* *
* Unless required by applicable law or agreed to in writing, software *
* distributed under the License is distributed on an "AS IS" BASIS, *
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. *
* See the License for the specific language governing permissions and *
* limitations under the License. *
* -------------------------------------------------------------------------- */

#include "AbstractPath.h"
#include "Appearance.h"

using namespace OpenSim;

AbstractPath::AbstractPath() : ModelComponent() {
setAuthors("Nicholas Bianco, Joris Verhagen, Adam Kewley");

Appearance appearance;
appearance.set_color(SimTK::Gray);
constructProperty_Appearance(appearance);
}

AbstractPath::AbstractPath(AbstractPath const&) = default;

AbstractPath::~AbstractPath() noexcept = default;

AbstractPath& AbstractPath::operator=(const AbstractPath&) = default;
nickbianco marked this conversation as resolved.
Show resolved Hide resolved

AbstractPath::AbstractPath(AbstractPath&& other) noexcept = default;

AbstractPath& AbstractPath::operator=(AbstractPath&& other) noexcept = default;

// DEFAULTED METHODS
const SimTK::Vec3& AbstractPath::getDefaultColor() const
{
return get_Appearance().get_color();
}

void AbstractPath::setDefaultColor(const SimTK::Vec3& color)
{
updProperty_Appearance().setValueIsDefault(false);
upd_Appearance().set_color(color);
}

double AbstractPath::getPreScaleLength(const SimTK::State&) const
{
return _preScaleLength;
}

void AbstractPath::setPreScaleLength(const SimTK::State&,
double preScaleLength)
{
_preScaleLength = preScaleLength;
}
Loading
Loading