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 6 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
17 changes: 11 additions & 6 deletions Applications/Scale/test/testScale.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
#include <OpenSim/Simulation/Model/ForceSet.h>
#include <OpenSim/Simulation/Model/Ligament.h>
#include <OpenSim/Simulation/Model/PhysicalOffsetFrame.h>
#include <OpenSim/Simulation/Model/GeometryPath.h>
#include <OpenSim/Simulation/SimbodyEngine/PinJoint.h>
#include <OpenSim/Simulation/SimbodyEngine/FreeJoint.h>
#include <OpenSim/Simulation/SimbodyEngine/EllipsoidJoint.h>
Expand Down Expand Up @@ -561,8 +562,9 @@ void scalePhysicalOffsetFrames()
const Vec3 offset1 = Vec3(0.2, 0.4, 0.6);
PathActuator* act1 = new PathActuator();
act1->setName("pathActuator1");
act1->addNewPathPoint("point1a", model->updGround(), Vec3(0));
act1->addNewPathPoint("point1b", *body, offset1);
auto& path = act1->updPath<GeometryPath>();
path.appendNewPathPoint("point1a", model->updGround(), Vec3(0));
path.appendNewPathPoint("point1b", *body, offset1);
body->addComponent(act1);

// Second PathActuator is attached to the Body via a POF. Both new
Expand All @@ -575,8 +577,9 @@ void scalePhysicalOffsetFrames()

PathActuator* act2 = new PathActuator();
act2->setName("pathActuator2");
act2->addNewPathPoint("point2a", model->updGround(), Vec3(0));
act2->addNewPathPoint("point2b", *pof2, offset2);
auto& path2 = act2->updPath<GeometryPath>();
path2.appendNewPathPoint("point2a", model->updGround(), Vec3(0));
path2.appendNewPathPoint("point2b", *pof2, offset2);
act1->addComponent(act2);

State& s = model->initSystem();
Expand All @@ -592,9 +595,11 @@ void scalePhysicalOffsetFrames()
model->getComponent<PathActuator>(pathToAct1);
const PathActuator& pa2 =
model->getComponent<PathActuator>(pathToAct2);
const auto& path1 = dynamic_cast<const GeometryPath&>(pa1.getPath());
adamkewley marked this conversation as resolved.
Show resolved Hide resolved
const auto& path2 = dynamic_cast<const GeometryPath&>(pa2.getPath());

const PathPointSet& pps1 = pa1.getGeometryPath().getPathPointSet();
const PathPointSet& pps2 = pa2.getGeometryPath().getPathPointSet();
const PathPointSet& pps1 = path1.getPathPointSet();
const PathPointSet& pps2 = path2.getPathPointSet();

for (int i = 0; i < 2; ++i)
{
Expand Down
17 changes: 13 additions & 4 deletions Bindings/Java/OpenSimJNI/OpenSimContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include <OpenSim/Simulation/Model/MovingPathPoint.h>
#include <OpenSim/Simulation/Model/Muscle.h>
#include <OpenSim/Simulation/Model/PathPoint.h>
#include <OpenSim/Simulation/Model/GeometryPath.h>
#include <OpenSim/Simulation/Wrap/PathWrap.h>
#include <OpenSim/Simulation/Model/ConditionalPathPoint.h>
#include <OpenSim/Simulation/SimbodyEngine/SimbodyEngine.h>
Expand Down Expand Up @@ -124,13 +125,21 @@ double OpenSimContext::getMuscleLength(Muscle& m) {
}

const Array<AbstractPathPoint*>& OpenSimContext::getCurrentPath(Muscle& m) {
return m.getGeometryPath().getCurrentPath(*_configState);
if (auto* p = dynamic_cast<const GeometryPath*>(&m.getPath())) {
adamkewley marked this conversation as resolved.
Show resolved Hide resolved
return p->getCurrentPath(*_configState);
} else {
OPENSIM_THROW(Exception, "Muscle path is not a GeometryPath.")
}
}

void OpenSimContext::copyMuscle(Muscle& from, Muscle& to) {
to = from;
recreateSystemKeepStage();
to.updGeometryPath().updateGeometry(*_configState);
if (auto* p = dynamic_cast<const GeometryPath*>(&from.getPath())) {
aymanhab marked this conversation as resolved.
Show resolved Hide resolved
to = from;
recreateSystemKeepStage();
to.updPath<GeometryPath>().updateGeometry(*_configState);
} else {
OPENSIM_THROW(Exception, "Muscle path is not a GeometryPath.")
}
}

// Muscle Points
Expand Down
1 change: 1 addition & 0 deletions Bindings/Java/OpenSimJNI/OpenSimContext.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ class GeometryPath;
class AbstractPathPoint;
class PathWrap;
class ConditionalPathPoint;
class PathPoint;
class WrapObject;
class Analysis;
class AnalyzeTool;
Expand Down
21 changes: 11 additions & 10 deletions Bindings/Java/OpenSimJNI/Test/testContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
#include <OpenSim/Simulation/Model/PathPoint.h>
#include <OpenSim/Simulation/Wrap/PathWrap.h>
#include <OpenSim/Simulation/Model/ConditionalPathPoint.h>
#include <OpenSim/Simulation/Model/GeometryPath.h>
#include <OpenSim/Simulation/SimbodyEngine/SimbodyEngine.h>
#include <OpenSim/Simulation/Wrap/WrapObject.h>
#include <OpenSim/Simulation/Model/Analysis.h>
Expand Down Expand Up @@ -121,7 +122,7 @@ int main()
cout << "Prop after create system is " << dProp2.toString() << endl;
bool after = PropertyHelper::getValueBool(dProp);
SimTK_ASSERT_ALWAYS(!after, "Property has wrong value!!");
dTRIlong->updGeometryPath().updateGeometry(context->getCurrentStateRef());
adamkewley marked this conversation as resolved.
Show resolved Hide resolved
dTRIlong->updPath<GeometryPath>().updateGeometry(context->getCurrentStateRef());
const OpenSim::Array<AbstractPathPoint*>& path = context->getCurrentPath(*dTRIlong);
cout << "Muscle Path" << endl;
cout << path.getSize() << endl;
Expand Down Expand Up @@ -156,7 +157,7 @@ int main()
cout << "After setting coordinate to 100 deg." << endl;
cout << xform << endl;
// Compare to known xform
dTRIlong->updGeometryPath().updateGeometry(context->getCurrentStateRef());
dTRIlong->updPath<GeometryPath>().updateGeometry(context->getCurrentStateRef());
const OpenSim::Array<AbstractPathPoint*>& newPath = context->getCurrentPath(*dTRIlong);
context->realizePosition();
// Compare to known path
Expand Down Expand Up @@ -190,37 +191,37 @@ int main()
context->restoreStateFromCachedModel();

// Exercise PathPoint operations used to edit Path in GUI
PathPointSet& pathPoints = dTRIlong->updGeometryPath().updPathPointSet();
std::string pathBeforeInXML = dTRIlong->updGeometryPath().dump();
PathPointSet& pathPoints = dTRIlong->updPath<GeometryPath>().updPathPointSet();
std::string pathBeforeInXML = dTRIlong->updPath<GeometryPath>().dump();
std::cout << pathBeforeInXML << endl;
int origSize = pathPoints.getSize();
AbstractPathPoint& savePoint = pathPoints.get("TRIlong-P2");
const std::string& saveFrameName = savePoint.getParentFrame().getName();
AbstractPathPoint* clonedPoint = savePoint.clone();

// Test delete second PathPoint from TRIlong muscle
context->deletePathPoint(dTRIlong->updGeometryPath(), 2);
context->deletePathPoint(dTRIlong->updPath<GeometryPath>(), 2);
assert(pathPoints.getSize() == origSize - 1);
std::string pathAfterDeletionInXML = dTRIlong->updGeometryPath().dump();
std::string pathAfterDeletionInXML = dTRIlong->updPath<GeometryPath>().dump();
std::cout << pathAfterDeletionInXML << endl;

// Test adding PathPoint to TRIlong muscle (Stationary)
Component& frame = model->updBodySet().updComponent(saveFrameName);
PhysicalFrame* physFrame = PhysicalFrame::safeDownCast(&frame);
context->addPathPoint(dTRIlong->updGeometryPath(), 3, *physFrame);
context->addPathPoint(dTRIlong->updPath<GeometryPath>(), 3, *physFrame);
assert(pathPoints.getSize() == origSize);
std::string pathAfterReinsertionInXML = dTRIlong->updGeometryPath().dump();
std::string pathAfterReinsertionInXML = dTRIlong->updPath<GeometryPath>().dump();
std::cout << pathAfterReinsertionInXML << endl;

// Test changing type to ConditionalPathPoint
ConditionalPathPoint* newPoint = new ConditionalPathPoint();
AbstractPathPoint& oldPoint = pathPoints.get(2);
newPoint->setCoordinate(model->getCoordinateSet().get(0));
newPoint->setParentFrame(oldPoint.getParentFrame());
context->replacePathPoint(dTRIlong->updGeometryPath(), oldPoint, *newPoint);
context->replacePathPoint(dTRIlong->updPath<GeometryPath>(), oldPoint, *newPoint);
assert(pathPoints.getSize() == origSize);

std::string pathAfterTypeChangeToViaInXML = dTRIlong->updGeometryPath().dump();
std::string pathAfterTypeChangeToViaInXML = dTRIlong->updPath<GeometryPath>().dump();
std::cout << pathAfterTypeChangeToViaInXML << endl;

// Make a change to a socket that is invalid and verify that we can recover
Expand Down
10 changes: 6 additions & 4 deletions Bindings/Java/tests/TestBasics.java
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,14 @@ public static void testMuscleList() {
Ground ground = model.getGround();
Thelen2003Muscle thelenMuscle =
new Thelen2003Muscle("Darryl", 1, 0.5, 0.5, 0);
thelenMuscle.addNewPathPoint("muscle1-point1", ground, new Vec3(0.0,0.0,0.0));
thelenMuscle.addNewPathPoint("muscle1-point2", ground, new Vec3(1.0,0.0,0.0));
GeometryPath pathThelen = thelenMuscle.initGeometryPath();
adamkewley marked this conversation as resolved.
Show resolved Hide resolved
pathThelen.appendNewPathPoint("muscle1-point1", ground, new Vec3(0.0,0.0,0.0));
pathThelen.appendNewPathPoint("muscle1-point2", ground, new Vec3(1.0,0.0,0.0));
Millard2012EquilibriumMuscle millardMuscle =
new Millard2012EquilibriumMuscle("Matt", 1, 0.5, 0.5, 0);
millardMuscle.addNewPathPoint("muscle1-point1", ground, new Vec3(0.0,0.0,0.0));
millardMuscle.addNewPathPoint("muscle1-point2", ground, new Vec3(1.0,0.0,0.0));
GeometryPath pathMillard = millardMuscle.initGeometryPath();
pathMillard.appendNewPathPoint("muscle1-point1", ground, new Vec3(0.0,0.0,0.0));
pathMillard.appendNewPathPoint("muscle1-point2", ground, new Vec3(1.0,0.0,0.0));
model.addComponent(thelenMuscle);
model.addComponent(millardMuscle);

Expand Down
5 changes: 3 additions & 2 deletions Bindings/Java/tests/TestModelBuilding.java
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,9 @@ public static void main(String[] args) {
0.6, // Optimal fibre length
0.55, // Tendon slack length
0.0); // Pennation angle
biceps.addNewPathPoint("origin", hum, ArrayDouble.createVec3(new double[]{0, 0.8, 0}));
biceps.addNewPathPoint("insert", rad, ArrayDouble.createVec3(new double[]{0, 0.7, 0}));
GeometryPath pathBiceps = biceps.initGeometryPath();
pathBiceps.appendNewPathPoint("origin", hum, ArrayDouble.createVec3(new double[]{0, 0.8, 0}));
pathBiceps.appendNewPathPoint("insert", rad, ArrayDouble.createVec3(new double[]{0, 0.7, 0}));

PrescribedController cns = new PrescribedController();
cns.addActuator(biceps);
Expand Down
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). TODO: add API changes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Careful, avoid changing the API in such a way that isn't doesn't match a 4.4 --> 4.5 transition. OpenSim users are acclimatized to version bumps mostly only changing something quite trivial (e.g. the caller must now provide a State to this or that method).

Not saying don't do it, but if there's any mechanism for avoiding it I'd strongly recommend doing that rather than having to eat the time overhead of supporting people complaining about their scripts being broken.

Copy link
Member Author

Choose a reason for hiding this comment

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

Per your suggestions, I'm opting to revert API changes since supporting getGeometryPath and updGeometryPath is straight-forward anyway. But I agree, in the end, I'd prefer minimal API changes for those reasons.


v4.4.1
======
- IMU::calcGyroscopeSignal() now reports angular velocities in the IMU frame.
Expand Down
54 changes: 30 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,36 @@ 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.

AbstractPath& path = muscBase.updPath();
if (auto* pGeometryPath = dynamic_cast<GeometryPath*>(&path)) {
auto& thisGeometryPath = actu->initGeometryPath();

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());
}
thisGeometryPath.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());
}
thisGeometryPath.updWrapSet().adoptAndAppend(pathWrap);
}
geomPath.updWrapSet().adoptAndAppend(pathWrap);
}

std::string actname = actu->getName();
Expand Down Expand Up @@ -1074,14 +1080,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
Loading
Loading