Add class `Scholz2015GeometryPath` #4156

nickbianco · 2025-09-17T20:05:52Z

Fixes issue #4113

Overview

Scholz2015GeometryPath is a geometry-based path wrapping algorithm based on the publication "A fast multi-obstacle muscle wrapping method using natural geodesic variations" by Scholz et al. (2015). This class encapsulates SimTK::CableSpan, the Simbody implementation of the core wrapping algorithm, and provides support for using this method to define the geometry for path-based OpenSim components (e.g., Muscle, PathSpring, etc.). This class can be used as a replacement for GeometryPath, providing improved computational performance and stability in wrapping solutions.

Below is a simple example that includes most of Scholz2015GeometryPath's features. Scholz2015GeometryPath defines the path of a PathSpring component, which applies forces to a double pendulum model.

#include <OpenSim/OpenSim.h>

using namespace OpenSim;

int main() {

    Model model = ModelFactory::createDoublePendulum();
    model.setUseVisualizer(true);

    // Create a PathSpring with a Scholz2015GeometryPath.
    auto* spring = new PathSpring();
    spring->setName("path_spring");
    spring->setRestingLength(0.25);
    spring->setDissipation(0.75);
    spring->setStiffness(10.0);
    spring->set_path(Scholz2015GeometryPath());
    model.addComponent(spring);

    // Configure the Scholz2015GeometryPath. We will update the path after
    // adding it to the PathSpring, so that the Socket connections in each
    // Station (i.e., path point) remain valid.
    Scholz2015GeometryPath& path = spring->updPath<Scholz2015GeometryPath>();
    path.setName("path");

    // Add a path point connected to ground. Since this is the first path point,
    // it defines the origin of the path.
    path.addPathPoint(model.getGround(), SimTK::Vec3(0.05, 0.05, 0.));

    // Add a second path point, creating a straight line segment between the
    // ground and body "b0".
    path.addPathPoint(model.getComponent<Body>("/bodyset/b0"),
            SimTK::Vec3(-0.5, 0.1, 0.));

    // Create a ContactCylinder to use as a wrapping obstacle to the path. The
    // cylinder has radius 0.15 m and is attached to body "b0".
    auto* obstacle = new ContactCylinder(0.15,
            SimTK::Vec3(-0.2, 0.2, 0.), SimTK::Vec3(0),
            model.getComponent<Body>("/bodyset/b0"));
    model.addComponent(obstacle);

    // Before we add the obstacle to the path, we must provide a "contact hint"
    // to initialize the wrapping solver. The contact hint is a point on the
    // surface of the obstacle, expressed in the obstacle's frame. The point
    // does not have to lie on the contact geometry's surface, nor does it have
    // to belong to a valid cable path. The choice of the contact hint will
    // determine which side of the cylinder the path wraps around.
    SimTK::Vec3 contact_hint(0., 0.15, 0.);
    path.addObstacle(*obstacle, contact_hint);

    // At least one path point must follow an obstacle (or list of obstacles)
    // in a Scholz2015GeometryPath. Since this is the last path point we are
    // adding, it defines the insertion of the path.
    path.addPathPoint(model.getComponent<Body>("/bodyset/b1"),
            SimTK::Vec3(-0.5, 0.1, 0.));

    // Use the "minimum length" algorithm, which solves for a path that
    // minimizes the total length of the path.
    path.setAlgorithm("MinimumLength");

    // Initialize the system.
    SimTK::State state = model.initSystem();
    model.updVisualizer().updSimbodyVisualizer().setBackgroundType(
            SimTK::Visualizer::SolidColor);
    model.updVisualizer().updSimbodyVisualizer().setCameraTransform(
            SimTK::Vec3(0.2, -0.8, 3.0));
    model.updVisualizer().updSimbodyVisualizer().setCameraFieldOfView(0.9);

    // Simulate.
    Manager manager(model);
    manager.initialize(state);
    manager.integrate(20.0);
}

Running this example produces the following motion. The double pendulum model responds to the spring forces applied by the PathSpring and the motion damps out over time due to the non-zero dissipation parameter.

exampleScholz2015GeometryPath.mp4

Brief summary of changes

Added the class Scholz2015GeometryPath which derives from AbstractGeometryPath. The helper classes Scholz2015GeometryPathPoint and Scholz2015GeometryPathObstacle help internally maintain the organization of path wrapping obstacles and via points during serialization and deserialization.
Scholz2015GeometryPathPoint stores a Station defining a point on a PhysicalFrame in the model which the path must pass through.
Scholz2015GeometryPathObstacle stores a Socket to a ContactGeometry and a contact hint. The ContactGeometry reference is used internally to generate a shared pointer to a SimTK::ContactGeometry, which SimTK::CableSpan needs to define wrapping obstacles.
Scholz2015GeometryPathPoint and Scholz2015GeometryPathObstacle both derive from Scholz2015GeometryPathElement. This allows us to store an ordered list of path points and obstacles in the path_elements property (i.e., the path elements will appear in the same order in XML as added by the user in the API).
Updates Model to include a unique pointer to SimTK::CableSubsystem, the Simbody subsystem to which all SimTK::CableSpan objects belong.
Adds exampleScholz2015GeometryPath.cpp, which is the same as the example in the Overview section above. Python and MATLAB versions of this example are also included.
MomentArmSolver is updated to support any AbstractGeometryPath, allowing moment arm calculations for Scholz2015GeometryPath.
Various formatting and whitespace "drive-by" changes are applied.

Testing I've completed

Several tests for Scholz2015GeometryPath have been added via testScholz2015GeometryPath.cpp. Here is an example of a set of regression tests where a pendulum is suspended by a Scholz2015GeometryPath element hanging over a wrap element. Similar to the example above, the path is connected to a PathSpring, so we test that we receive the expected path length and tension after the motion damps out.

hanging_pendulum_Scholz2015GeometryPath.mp4

The C++, Python, and Matlab examples have been tested locally on my Arm64 Mac.

Looking for feedback on...

API design, test coverage, and anything that might improve both the user and developer experience with Scholz2015GeometryPath.

CHANGELOG.md (choose one)

updated.

This change is

Scholz2015GeometryPath is a geometry-based path wrapping algorithm based on the publication "A fast multi-obstacle muscle wrapping method using natural geodesic variations" by Scholz et al. (2015). This class encapsulates `SimTK::CableSpan`, the Simbody implementation of the core wrapping algorithm, and provides support for using this method to define the geometry for path-based OpenSim components (e.g., `Muscle`, `PathSpring`, etc.). This class can be used as a replacement for `GeometryPath`, providing improved computational performance and stability in wrapping solutions.

nickbianco · 2025-09-17T20:26:32Z

@adamkewley, do you have bandwidth/interest to be a second reviewer here? Your perspective as it relates to OpenSim Creator integration would be valuable.

@carmichaelong, @tkuchida, @pepbos, would be happy to get any input you have here too!

adamkewley · 2025-09-18T12:22:19Z

At a quick glance, looks great @nickbianco

I'll try and review it soon 🚀

carmichaelong

Looks great overall. I skimmed over everything, with an eye more towards usage/documentation rather than details of the implementation.

Reviewable status: 0 of 26 files reviewed, 4 unresolved discussions (waiting on @adamkewley and @aymanhab)

OpenSim/Tests/Wrapping/testScholz2015GeometryPath.cpp line 144 at r1 (raw file):

    // Check that pendulum is at rest with the expected length.
    const double expectedLength = 0.81268778;

was this calculated by hand or empirically by letting the simulation settle?

OpenSim/Tests/Wrapping/testScholz2015GeometryPath.cpp line 206 at r1 (raw file):

TEST_CASE("Moment arms") {

    // Randomly choose the radius of the cylinder within a valid range.

Is it better to randomize and test one configuration, or test over a pre-defined set of values?

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 94 at r1 (raw file):

 * `Scholz2015GeometryPathObstacle`s via the `obstacles` property. The order of
 * the obstacles in the list is important, as it defines the order in which
 * the path wraps over the obstacles.

I'm assuming that the order should align with paths that start from the origin and ending with the insertion? (e.g., you can't flip origin and insertion and get the same interactions/answers)

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 247 at r1 (raw file):

 *
 * The contact hint is a `SimTK::Vec3` defining a point on the surface in local
 * surface frame coordinates. The point will be used as a starting point when

I initially had a comment here wanting more info on this, but the comment with addObstacle() is great.

Separately, maybe The point will be used as a starting point... -> The point will be used to provide an initial guess when solving for the cable path? (I think my brain kept stumbling on The point and starting point)

adamkewley

Hi,

Great work. I've had some (personal) issues building it, so haven't managed to test it locally yet, but it must be acknowledged that it's a major milestone to have something working within the OpenSim API/ecosystem etc. already!

My main point of contention is in the data and API that end-users will see. The underlying SimTK::CableSpan was designed to be mathematically correct, full of features, and so on but (imo) it doesn't adequately account for how OpenSim users are likely to actually think about a path.

In effect, the underlying simbody implementation appears to be (correct me), colloquially:

A sequence of path segments
Each path segment may interact with 0..n obstacles, specified on a per-segment basis
Each path segment only collides (if at all) with the specified obstacles in the specified order
- e.g. If it misses obstacle[0] and then hits obstacle[1] it cannot hit obstacle[0] afterwards - even if the path is clipping through obstacle[0] afterwards.
Each obstacle may have a hint point that initializes where the path initially tries to go (e.g. one side of a cylinder, inside a torus)

These details (especially w.r.t. ordering entries in a list) are things that the user now needs to think about. However, a user's mental model is going to be closer to (imo):

A sequence of path points form a path
The path may interact with 0..n obstacles, specified on a per-path basis
The path collides with the obstacles along it.
Each obstacle may have a hint point (as above)

There's some limitations to this, but (imo) they could be worked around:

It is undefined/unsupported that a path would wrap around one obstacle, wrap around another obstacle, and then wrap around the first obstacle again.
If that must be supported, the user could explicitly add another obstacle with the same ContactGeometry. This would let them specify a second hit point on the return journey. It would require exposing a enforce_in_order_wrapping property, which would make the behavior closer to the underlying simbody implementation - without the complexity of having to specify every obstacle on every path segment.

@adamkewley reviewed 26 of 26 files at r1, all commit messages.
Reviewable status: all files reviewed, 32 unresolved discussions (waiting on @aymanhab, @carmichaelong, and @nickbianco)

OpenSim/Tests/Wrapping/testScholz2015GeometryPath.cpp line 206 at r1 (raw file):

Previously, carmichaelong wrote…

Is it better to randomize and test one configuration, or test over a pre-defined set of values?

Imo it's better to avoid randomization altogether unless there's a guarantee that the randomness is reproducible on all target architectures/OSes when the test fails (e.g. a seed passed via an environment variable or similar)

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

// SCHOLZ 2015 GEOMETRY PATH OBSTACLE
//=============================================================================
Scholz2015GeometryPathObstacle::Scholz2015GeometryPathObstacle() : Component() {

I don't think it's necessary to state the Component() base class initializer here

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

        double tension, ForceConsumer& forceConsumer) const {

    if (tension < 0.) {

Or !(tension >= 0.0), which would handle the case of tension being NaN (also, beware that >=0.0 includes -0.0)

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

    }

    // Forces applied to each obstacle body.

If this is actually the hot path, I'd consider changing _obstacleIndexMap to a std::vector, so that you can iterate over a dense sequence of the information rather than heap-allocated lists of associations that the implementation doesn't actually use as an O(1) lookup very much.

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

    // Forces applied to each obstacle body.
    for (const auto& [index, frame] : _obstacleIndexMap) {
        SimTK::CableSpanObstacleIndex ix(index);

Cant the map/vector be constructed with these index types, rather than converting them here?

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

// MODEL COMPONENT INTERFACE
//=============================================================================
void Scholz2015GeometryPath::extendFinalizeFromProperties() {

... and there's at least one curve segment
... and the curve segments are connected to one another
... and the origin point corresponds to the first point in the first segment
... and the insertion point corresponds to the last point in the last segment

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

    } else if (get_algorithm() == "MinimumLength") {
        _algorithm = SimTK::CableSpanAlgorithm::MinimumLength;
    }

else... throw an error message? The checkPropertyValueIsInSet might be redundant if the next thing the implementation is doing is iterating through the options anyway

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

}

void Scholz2015GeometryPath::extendAddToSystem(

Ultimately, I'm assuming the property/component design of the new path is mostly dictated by this function? Here's where a more-automatic implementation would have to slice up the path, load in hit points, etc?

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

    SimTK::CableSubsystem& cables = system.updCableSubsystem();
    SimTK::CableSpan cable(cables,

And this is (I guess) why there's an explicit origin/insertion point? It feels like OpenSim users (very high-level) don't need to be aware of a detail that simbody happens to need.

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

        insertion.get_location());

    for (int iseg = 0; iseg < getProperty_segments().size(); ++iseg) {

This is where the sequence temporal-coupling issue actually emerges. It's a detail from simbody and the CablePath API design that has leaked out, and now all of the users have to worry about it. Is there absolutely no way of simplifying the path slicing down such that the user doesn't have to think about it?

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

        // Add the via point if this is not the last segment.
        if (iseg < getProperty_segments().size() - 1) {

Wait... hold up...

This implementation never actually uses the origin field in any path segment? If that's the case, then your minimal data structure is actually:

struct Path {
    PathPoint origin;  // direct child

    struct PathSegment {
        PathPoint location;  // direct child

        struct Obstacle { CollisionGeometry geom; Optional<Vec3> hint;  };
        Obstacle[] obstacles;
    };

    PathSegment[] segments;
};

Same inputs, 1:1 compatible with Simbody, etc.

What I'm suggesting is that users will may find it annoying to figure out the obstacles and hint fields. If there's any way, at runtime, to figure out whether there's >= 2 points and which path segments initially interact with which obstacles, and the likely order the obstacles will need to be in, then this becomes:

struct Path {
PathPoint[] points;
struct Obstacle { CollisionGeometry geom; Optional hint; };
Obstacle[] obstacles;
};

Which has the limitation that it isn't as powerful as the underlying implementation, which allows obstacles to be repeated (A B A A B) and for that repetition to have different hit points, etc. This is a similar limitation to the original GeometryPath, but it's really up for debate whether it's likely that a biological system is going to wrap back over the same geometry at a different point after liftoff (maybe?)

However, even just simplifying the implementation to the reduced 1:1-with-simbody version would, imo, make it easier to port models over to the new implementation.

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

    if (fixed) { return; }

    getCableSpan().calcDecorativePathPoints(s,

This would almost certainly need to match GeometryPath (coloring, hiding, etc.)

Bindings/Java/Matlab/examples/Wrapping/exampleScholz2015GeometryPath.m line 55 at r1 (raw file):

% has been added to the PathSpring, so that the Socket connections in each
% Station remain valid.
path.setOrigin(model.getGround(), Vec3(0.05, 0.05, 0.));

CORRECTION: this comment doesn't necessarily matter in this case because the CurveSegments are responsible for the sequence, but I'll keep this comment here because it might be the first thing someone's going to think when they see the API.

Wouldn't it be possible to have the user provide all points as a sequence? Separating the origin/insertion overlooks the fact that you're already traversing an implicit sequence: [origin, via_points..., insertion].

It's important because, if you swap the two, you don't swap the other elements of the sequence. Instead, you end up with: [insertion, via_points..., origin]. So API need to know the underlying sequence anyway when manipulating the path.

OpenSim/Simulation/RegisterTypes_osimSimulation.cpp line 203 at r1 (raw file):

    Object::registerType( FunctionBasedPath());
    Object::registerType( Scholz2015GeometryPathObstacle());
    Object::registerType( Scholz2015GeometryPathSegment());

CORRECTION: Similar to the above: it wasn't obvious, at first glance, why there's this separation between curve segments and sequences of points. It makes sense for the SimTK CableSpan implementation but doesn't make as much sense when my OpenSim experience (muscles, etc.) has almost always been "a sequence of path points"

Seems like this is an internal class rather than something that API users need to be interested in. I.e. end-users should only care about supplying a sequence of stations and obstacles.

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 68 at r1 (raw file):

// PROPERTIES
//=============================================================================
    OpenSim_DECLARE_PROPERTY(contact_hint, SimTK::Vec3,

Is this strictly required, or optional?

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 74 at r1 (raw file):

// METHODS
//=============================================================================
    // CONSTRUCTION

Mixes grammar: CONSTRUCTION <-> ACCESS, CONSTRUCTOR(S) <-> ACCESSOR(S)

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 96 at r1 (raw file):

 * the path wraps over the obstacles.
 *
 * Users should not create instances of this class directly, nor should they

CORRECTION: same as previous. Keeping it here because it was my first thought when skimming through. It's important to understand that end-users need a very very clear message of what they need to do in order to (e.g.) build a muscle with this design.

If it's the case that users don't actually explicitly handle path segments (because they're an internal concern of the cable wrapping implementation) then users should only see a sequence of stations and obstacles.

E.g. a ScholzPath should be as close as: ScholzPath { Station points[]; Obstacle contactHintPlusUnderlyingGeom[]; }; as possible

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 153 at r1 (raw file):

 * A concrete class representing a path object that begins at an origin point
 * fixed to a body, wraps over geometric obstacles and passes through
 * frictionless via points fixed to other bodies, and terminates at an insertion

Via points must be attached to other bodies?

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 274 at r1 (raw file):

 * not create or manage `Scholz2015GeometryPathSegment` objects directly.
 *
 * ## Path Ordering

(THIS IS WHY THERE'S CORRECTIONS ABOVE)

I'm think this API design involves quite a few pieces, which is why I think I'm not fully understanding why there's certain decisions around separating stations, path segments, etc.

From what I understand, the design has:

ContactGeometry, somewhere else in the model
A top-level path class (Scholz2015GeometryPath)
A top-level origin Station (Scholz2015GeometryPath::origin)
A top-level insertion Station (Scholz2015GeometryPath::insertion
Top-level via_point Stations
Top-level segments, which may refer to any of {origin,via_points,insertion} but, coding-wise, may refer to any Station in the model.
Obstacles, which are effectively a reference to the ContactGeometry, plus a contact hint
Other stuff related to the internals of the algorithm

And, as a direct result of this design, every time a caller calls addViaPoint the implementation is internally going to create a new Scholz2015GeometryPathSegment and assign the currently-unassigned obstacles to that segment, if I understand that correctly? Any left-over (unassigned) obstacles will end up in the final path segment?

There's several things to consider from a design PoV when using this via the API or loading it directly from an XML file:

The origin/insertion/viapoints don't appear to be strictly required. A Scholz2015GeometryPathSegment loaded from an osim can potentially socket to any Station in the model. The stations are essentially just a holding spot for "Stations this path handles". You could just as equally use the native ComponentSet of the path's base class (Component) to handle ownership.
The sequencing of the Stations doesn't appear to matter because sequence is actually encoded by Scholz2015GeometryPathSegment
Because Scholz2015GeometryPathSegment` is strictly a two-point segment, and the implementation isn't responsible for slicing it up, the caller/XML can declare disjoint path segments (e.g. you could add path segment A-B then E-F)

I'd strongly consider what the simplest UX could be and code around it at runtime, rather than directly exposing end-users to all of these decisions. Otherwise, you're asking them to keep quite a lot of concerns in-mind (e.g. the various station collections, obstacles, hittests are local to the contact geometry O_o) to save having to do some juggling at system-building time.

E.g. users are already familiar with providing a sequence of points and providing all of the obstacles they think it should interact with. An alternate design would be to piggyback on similar concepts available from GeometryPath, but perhaps provide a ContactHintPathPoint with the explanation that "it's kind of like a via-point, but only during initialization, and only if collision is detected during initialization". That way, the user provides what they are already familiar with providing (a sequence of points, plus a list of obstacles), then you can mess around internally to forward the correct information to the underlying CableSpan API.

Just my 2c but, knowing how users build models, anything even slightly complicated will be tough to rollout. There is also a meta concern/desire to port existing models to the new algorithm, which will be significantly easier to do if the new design is essentially the old one with a couple of extra steps.

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 322 at r1 (raw file):

        "The algorithm used to compute the path. Options: 'Scholz2015' "
        "(default) or 'MinimumLength'.");
    OpenSim_DECLARE_PROPERTY(curve_segment_accuracy, double,

What does "accuracy" mean here? Length?

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 328 at r1 (raw file):

        "The smoothness tolerance used to compute the optimal path "
        "(default: 0.1 degrees).");
    OpenSim_DECLARE_PROPERTY(solver_max_iterations, int,

I'd really be careful about exposing algorithm etc. directly in the public API.

This information will be encoded into the osim and thrown onto SimTK potentially for a very long time: are you confident that algorithm tweaking numbers will be just as relevant in 10 years?

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 385 at r1 (raw file):

     * Add an obstacle to the path.
     *
     * The provided `ContactGeometry` is used internally to provide an

Do public API users actually need to know what's happening internally? Their job is (effectively) to stuff properties into Components, compile, and get outputs.

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 413 at r1 (raw file):

     *
     * The via point is added as a `Station` that is connected to the last path
     * segment's insertion. The via point is automatically connected to the

Is this logically also the case when the last path segment's insertion is the final insertion point? Juggling between the various datastructures/segments makes it unclear.

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 445 at r1 (raw file):

     * See `SimTK::CableSpan::CableSpanAlgorithm` for more details.
     */
    void setAlgorithm(std::string algorithm);

const std::string& or std::string_view

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 453 at r1 (raw file):

    /**
     * %Set the accuracy used by the numerical integrator when computing a

What kind of accuracy is it, though? Is it literally an arbitrary number or am I providing a length tolerance?

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 457 at r1 (raw file):

     */
    void setCurveSegmentAccuracy(double accuracy);

Just as a general point: this class is generally leaking a lot of implementation details. Do we really want people writing MATLAB scripts that query the number of solver iterations a cable wrapping algorithm actually used? Maybe, no idea. However, keep in mind that OpenSim puts almost everything into the public API for a very long time.

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 524 at r1 (raw file):

     * @param ix The index of the obstacle in the path.
     */
    bool isInContactWithObstacle(const SimTK::State& state,

Seems rather internal? Why would a public API user have/know the underlying SimTK::CableSpanObstacleIndex?

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 631 at r1 (raw file):

    mutable SimTK::ResetOnCopy<SimTK::CableSpanIndex> _index;

    using ObstacleIndexMap = std::unordered_map<int,

Not important, but it might be an idea to just expand the ForceConsumer API if it means you don't have to store raw pointers in every path. Raw pointers are, in general, where all of the really really fun bugs tend to hide.

OpenSim/Tests/Wrapping/testScholz2015GeometryPath.cpp line 1005 at r1 (raw file):

    CHECK_THAT(path->getLength(state),
        Catch::Matchers::WithinRel(5.6513, 1e-3));
}

This is an impressive amount of testing.

I think it's hard for me to review it because there's an element of "lets manually cook up a situation and check the answers", which is completely fine as long as you think it's good enough to spot any problems.

…sary calls to Component constructor; various comment, test, and error-checking improvements

nickbianco

Thanks for the careful and thorough review @adamkewley! I've done a first pass through your comments, mostly just responding with some of my thoughts to digest everything.

I think you've hit on the core challenge: how do we define an OpenSim component that 1) satsifies the SimTK::CableSpan API, 2) is serializable/deserializable, and 3) as closely as possible resembles path building for users who are experienced with GeometryPath. Some of your suggestions are easy improvements to implement, but other suggestions, especially

The path may interact with 0..n obstacles, specified on a per-path basis

are more challenging, since GeometryPath and SimTK::CableSpan use fundamentally different algorithms. GeometryPath maintains a list of path points that grows and shrinks based on the number of intersected obstacles. (Side note: I suspect a lot of GeometryPath's poor performance stems from expensive management of this dynamically changing list of path points, rather then the wrapping calculations themselves) .

SimTK::CableSpan on the other hand maintains a fixed number of path points based on the number of via points and obstacles, in an expected order. The main exception is the touchdown/liftoff scenario, but that is handled by storing independent path-error Jacobians representing all possible touchdown/liftoff cases, and each of these Jacobians require a fixed path order. Added via points define multiple independent subproblems, one for each path segment. To support dynamic "path slicing", we would need to update SimTK::CableSpan to handle all possible combinations of subproblems, order of wrapping surfaces within each subproblem, and touchdown/liftoff cases. But currently, ordering is important and necessarily part of the user interface.

As a starting point, I will try to simplify what is already here and bring it closer to the OpenSim user's expectations. Your comments about exposing too many SimTK::CableSpan settings are good, although some of these settings are entangled with the current test suite, so I need to think on that for a bit. I agree that some of the station properties could probably be hidden or replaced by plain subcomponents to prevent unexpected path point socket connections from other stations in the model.

It is undefined/unsupported that a path would wrap around one obstacle, wrap around another obstacle, and then wrap around the first obstacle again.

This is actually not supported by SimTK::CableSpan anyway, and the workaround is the same: add a second obstacle in the same spot as the first one to support the re-wrap. I doubt that needed a path to wrap a single obstacle multiple times will be relevant for many paths anyway

Reviewable status: all files reviewed, 32 unresolved discussions (waiting on @adamkewley, @aymanhab, and @carmichaelong)

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 68 at r1 (raw file):

Previously, adamkewley (Adam Kewley) wrote…

Is this strictly required, or optional?

It is optional in the Simbody implementation, so we could make it optional here too and have two addObstacle() signatures in Scholz2015GeometryPath. I think in the majority of cases, you would want an contact hint for each obstacle in a path to ensure consistent wrapping behavior in a model. If we do make it optional, then there is probably some follow-up work to be done to automatically select a "good" initial point based on known contact geometry and current path configuration.

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 74 at r1 (raw file):

Previously, adamkewley (Adam Kewley) wrote…

Mixes grammar: CONSTRUCTION <-> ACCESS, CONSTRUCTOR(S) <-> ACCESSOR(S)

Done.

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 94 at r1 (raw file):

Previously, carmichaelong wrote…

I'm assuming that the order should align with paths that start from the origin and ending with the insertion? (e.g., you can't flip origin and insertion and get the same interactions/answers)

As currently written, flipping the origin and insertion would be problematic since the insertion point is specifically associated with the via point. However, as @adamkewley points out below, we could simply remove the origin point since it is not used at all and rename the insertion point "location" or "via point".

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 153 at r1 (raw file):

Previously, adamkewley (Adam Kewley) wrote…

Via points must be attached to other bodies?

Good catch. They do not need to be attached to "other" (i.e., different from the origin/insertion) bodies, but they do need to be attached to a body. This means that, currently, equivalents to MovingPathPoint and ConditionalPathPoint are not supported. I feel strongly that via points should be attached to bodies (to avoid issues like this), but for now that is just a personal opinion. We will have to address the needs of users who want to convert from models with these types of path points eventually.

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 247 at r1 (raw file):

Previously, carmichaelong wrote…

I initially had a comment here wanting more info on this, but the comment with addObstacle() is great.

Separately, maybe The point will be used as a starting point... -> The point will be used to provide an initial guess when solving for the cable path? (I think my brain kept stumbling on The point and starting point)

Done.

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 322 at r1 (raw file):

Previously, adamkewley (Adam Kewley) wrote…

What does "accuracy" mean here? Length?

This controls the step size of the geodesic integrator, so yes, it should be in length units, but it's not 100% clear from the Simbody documentation. But I think it's likely we hide this setting, at least for now.

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 445 at r1 (raw file):

Previously, adamkewley (Adam Kewley) wrote…

const std::string& or std::string_view

The argument is passed to the property internally via set_algorithm(std::move(algorithm)). If this is problematic for some reason, I'm happy to switch to the typical const std::string& and set_algorithm(algorithm) combo.

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 453 at r1 (raw file):

Previously, adamkewley (Adam Kewley) wrote…

What kind of accuracy is it, though? Is it literally an arbitrary number or am I providing a length tolerance?

Same as above: I need to dig further into Simbody to confirm, but it should be a length tolerance since this controls the step size of the geodesic integrator.

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 457 at r1 (raw file):

Previously, adamkewley (Adam Kewley) wrote…

Just as a general point: this class is generally leaking a lot of implementation details. Do we really want people writing MATLAB scripts that query the number of solver iterations a cable wrapping algorithm actually used? Maybe, no idea. However, keep in mind that OpenSim puts almost everything into the public API for a very long time.

I'm not sure either. Most users probably don't need access to these settings, but having some control could be useful. Some of the tests (which were lifted from the SimTK::CableSpan implementation) currently rely on having access to smoothness and integrator settings. I think I would be fine removing them, but I'd need to rejigger the tests a bit.

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 631 at r1 (raw file):

Previously, adamkewley (Adam Kewley) wrote…

Not important, but it might be an idea to just expand the ForceConsumer API if it means you don't have to store raw pointers in every path. Raw pointers are, in general, where all of the really really fun bugs tend to hide.

I found a reasonable alternative that relies solely on indexing.

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

Previously, adamkewley (Adam Kewley) wrote…

I don't think it's necessary to state the Component() base class initializer here

Done.

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

Previously, adamkewley (Adam Kewley) wrote…

Or !(tension >= 0.0), which would handle the case of tension being NaN (also, beware that >=0.0 includes -0.0)

Hmm, I wonder if it would be preferred to allow the NaN through (and subsequently crash and burn) so it doesn't silently skip force calculations when an invalid tension is provided. Although, hopefully a NaN tension would be error-handled somewhere else. I'll definitely add a unit test for this scenario.

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

Previously, adamkewley (Adam Kewley) wrote…

If this is actually the hot path, I'd consider changing _obstacleIndexMap to a std::vector, so that you can iterate over a dense sequence of the information rather than heap-allocated lists of associations that the implementation doesn't actually use as an O(1) lookup very much.

Done.

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

Previously, adamkewley (Adam Kewley) wrote…

Cant the map/vector be constructed with these index types, rather than converting them here?

Hashing with std::unordered_map apparently isn't supported with Simbody's unique integer types, but I've switched to std::vector and it works fine.

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

Previously, adamkewley (Adam Kewley) wrote…

else... throw an error message? The checkPropertyValueIsInSet might be redundant if the next thing the implementation is doing is iterating through the options anyway

Done.

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

Previously, adamkewley (Adam Kewley) wrote…

Ultimately, I'm assuming the property/component design of the new path is mostly dictated by this function? Here's where a more-automatic implementation would have to slice up the path, load in hit points, etc?

Yes, you're correct. Supporting serialization/deserialization while satisfying the SimTK::CableSpan API is the main challenge here. I'm not sure what an automatic implementation would look like off the top of my head (I hadn't really considered that as a possibility honestly).

I think you're essentially suggestion an equivalent behavior to GeometryPath::applyWrapObjects(), where, every time a path is computed, the geometry engine adds path points along intersected obstacles; the list of path points expands and shrinks as the path interacts with obstacles. SimTK::CableSpan is fundamentally different: the number of path points (including those created on surfaces) is always fixed and always in the same order, and the path-error Jacobian relies on this fixed structure. (Different sized Jacobians are dynamically constructed to account for touchdown/liftoff cases scenarios and via points divide the SimTK::CableSpan into multiple subproblems for each path segment). In other words, the wrapping algorithm is closely tied to (and basically necessitates) a fixed path ordering. If we wanted more dynamic/automatic path slicing behavior, then we would need to make significant changes to SimTK::CableSpan.

All that said, I will think about this and leave additional thoughts in related comments.

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

Previously, adamkewley (Adam Kewley) wrote…

And this is (I guess) why there's an explicit origin/insertion point? It feels like OpenSim users (very high-level) don't need to be aware of a detail that simbody happens to need.

I guess that users don't need to specify an explicit origin and insertion point; we could just require a minimum of two points when defining a path. I personally like having an explicit origin and insertion, but you make a good point that users will be coming from GeometryPath which uses PathPointSet.

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

Previously, adamkewley (Adam Kewley) wrote…

This is where the sequence temporal-coupling issue actually emerges. It's a detail from simbody and the CablePath API design that has leaked out, and now all of the users have to worry about it. Is there absolutely no way of simplifying the path slicing down such that the user doesn't have to think about it?

As I mentioned above, I'm not sure (yet) of how to automatically slice up a path such that obstacles find/detect the correct path segment they belong to. A solution based on the default configuration of the model would mean that path behavior could change based on different model configurations. I could be wrong, but I'm not seeing a way how users won't have to think about where in the path an obstacle should be active.

As a side note, even if users do need to worry about the order of via points and obstacles, I do think that there is probably too much talk about path segments, which should be mostly hidden from the user, in the documentation. The problem there is that Scholz2015GeometryPathSegment and Scholz2015GeometryPathObstacle will appear in the XML file, and as much as I would like to believe users won't directly modify XML, I know they will :'(.

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

This implementation never actually uses the origin field in any path segment? If that's the case, then your minimal data structure is actually:

You're totally right -- I think I had wrote the path segment class expecting a use for both origin and insertion, but there's no reason to keep it if it's unused. I'll try to work in the minimal data structure you've suggested (and see if I can simplify/remove some of the properties/sockets along the way).

If there's any way, at runtime, to figure out whether there's >= 2 points and which path segments initially interact with which obstacles

The key here is "initially interact", and how to define that. If it's defined by the model configuration, then changing the default model configuration could change the path definition during serialization.

Which has the limitation that it isn't as powerful as the underlying implementation, which allows obstacles to be repeated (A B A A B) and for that repetition to have different hit points, etc.

SimTK::CableSpan allows multiple wraps over the same obstacle (i.e., A A A) but not repeated wraps over the same obstacle after another obstacle has been encountered (i.e., A B A). I think that the large majority of scenarios that OpenSim will care about will not involve either case of multiple wrapping or even touchdown/liftoff.

However, even just simplifying the implementation to the reduced 1:1-with-simbody version would, imo, make it easier to port models over to the new implementation.

I would argue that the current implementation is quite close to the Simbody implementation, at least in terms of defining a path via the API. The additional classes to enable serialization is major difference though.

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

Previously, adamkewley (Adam Kewley) wrote…

This would almost certainly need to match GeometryPath (coloring, hiding, etc.)

Agreed. I think that could be addressed in a follow up PR.

OpenSim/Tests/Wrapping/testScholz2015GeometryPath.cpp line 144 at r1 (raw file):

Previously, carmichaelong wrote…

was this calculated by hand or empirically by letting the simulation settle?

Empirically, by letting it settle. So this is a regression test, not a unit test.

OpenSim/Tests/Wrapping/testScholz2015GeometryPath.cpp line 206 at r1 (raw file):

Previously, adamkewley (Adam Kewley) wrote…

Imo it's better to avoid randomization altogether unless there's a guarantee that the randomness is reproducible on all target architectures/OSes when the test fails (e.g. a seed passed via an environment variable or similar)

Done.

OpenSim/Tests/Wrapping/testScholz2015GeometryPath.cpp line 1005 at r1 (raw file):

Previously, adamkewley (Adam Kewley) wrote…

This is an impressive amount of testing.

I think it's hard for me to review it because there's an element of "lets manually cook up a situation and check the answers", which is completely fine as long as you think it's good enough to spot any problems.

FWIW, a lot of it is a one-to-one recreation from the Simbody tests. That doesn't necessarily make it easier to review, but I feel pretty good about this test coverage, at least for the core Scholz2015GeometryPath functionality.

adamkewley · 2025-09-20T12:16:50Z

@nickbianco , thanks for the responses, I'll try and check them next week.

Just as a quick musing while you're working on this. One thought I had was that it's completely fine to have a design that allows users to fully exercise the complexity of the underlying CableSpan, because that unlocks everything, but what if you actually ship two path components?:

A SimplifiedScholzGeometryPath (or similar), which allows a sequence of points and an optional path obstacle (i.e. geometry + optional hit hint) - perhaps with the assumption that all path segments in this simplified path may interact with the obstacle?
A ScholzGeometryPath (full fat), which is essentially what we're discussing here - save maybe redesigning it to be one origin point and 1..n via/segment points (to avoid duplication), where each segment-point would hold { location, obstacles[] } or something.

That way, you might actually be able to side-step the UX issues for the majority of cases ("just use the simplified one") and provide a gateway drug to something that can potentially do the harder stuff, suitable for an interesting experiment, paper, etc. UX-wise, I imagine there's (at the very least) a way of writing a conversion from simplified -> full-fat so that users can click a button and immediately have a path that behaves the same way but can now be modified on a per-segment, multi-obstacle, etc. basis?

aymanhab

Great work @nickbianco I've been going through the description and comments from @adamkewley and my general leaning would be toward practical musculoskeletal modeling situations which are well addressed by this implementation as far as I can see.

Reviewable status: 18 of 26 files reviewed, 32 unresolved discussions (waiting on @adamkewley, @carmichaelong, and @nickbianco)

aymanhab

I'd like to point out that the old wrapping system allowed for specifying a range of points where wrapping can occur, most model builders who really know what they were doing were specifying these tightly rather than leaving it to the code to sort out. If we check published models we should get a feel for how common these were specified by users.

Reviewable status: 18 of 26 files reviewed, 32 unresolved discussions (waiting on @adamkewley, @carmichaelong, and @nickbianco)

nickbianco

@adamkewley and @aymanhab: as a step towards a more user-friendly API, I've redesigned Scholz2015GeometryPath such that a path can be constructed from a list of path points (i.e., addPathPoint()) and obstacles (i.e., addObstacle()). This design is closer to the GeometryPath API and (I think) generally more intuitive. Along the way I've simplified some of the data structures and implemented various minor improvements.

Reviewable status: 17 of 26 files reviewed, 32 unresolved discussions (waiting on @adamkewley and @carmichaelong)

Bindings/Java/Matlab/examples/Wrapping/exampleScholz2015GeometryPath.m line 55 at r1 (raw file):

Previously, adamkewley (Adam Kewley) wrote…

CORRECTION: this comment doesn't necessarily matter in this case because the CurveSegments are responsible for the sequence, but I'll keep this comment here because it might be the first thing someone's going to think when they see the API.

Wouldn't it be possible to have the user provide all points as a sequence? Separating the origin/insertion overlooks the fact that you're already traversing an implicit sequence: [origin, via_points..., insertion].

It's important because, if you swap the two, you don't swap the other elements of the sequence. Instead, you end up with: [insertion, via_points..., origin]. So API need to know the underlying sequence anyway when manipulating the path.

I've redesigned the API so that users provide path points as a sequence.

OpenSim/Simulation/RegisterTypes_osimSimulation.cpp line 203 at r1 (raw file):

Previously, adamkewley (Adam Kewley) wrote…

CORRECTION: Similar to the above: it wasn't obvious, at first glance, why there's this separation between curve segments and sequences of points. It makes sense for the SimTK CableSpan implementation but doesn't make as much sense when my OpenSim experience (muscles, etc.) has almost always been "a sequence of path points"

Seems like this is an internal class rather than something that API users need to be interested in. I.e. end-users should only care about supplying a sequence of stations and obstacles.

Scholz2015GeometryPathSegment is needed for de/serialization, so we can store the ordered list of obstacles per path segment. It is internal in the sense that you do not have to manage path segments directly when using the API, but it will appear in the XML. I suppose this could be achieved in a different way, but somehow we will need to know which obstacles are associated with which path segment.

Perhaps we could collapse this information into the Scholz2015GeometryPathSegment class, and add a property that denotes after which path point the obstacle is active.

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 96 at r1 (raw file):

Previously, adamkewley (Adam Kewley) wrote…

CORRECTION: same as previous. Keeping it here because it was my first thought when skimming through. It's important to understand that end-users need a very very clear message of what they need to do in order to (e.g.) build a muscle with this design.

If it's the case that users don't actually explicitly handle path segments (because they're an internal concern of the cable wrapping implementation) then users should only see a sequence of stations and obstacles.

E.g. a ScholzPath should be as close as: ScholzPath { Station points[]; Obstacle contactHintPlusUnderlyingGeom[]; }; as possible

The API is now closer to this design: addPathPoint() and addObstacle(). However, ordering still matters and understanding which segment that obstacle applies to is useful. Perhaps we can design the class API and documentation so that users can easily understand the notion of a path segment, but not have to worry about managing them.

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 274 at r1 (raw file):

Previously, adamkewley (Adam Kewley) wrote…

(THIS IS WHY THERE'S CORRECTIONS ABOVE)

I'm think this API design involves quite a few pieces, which is why I think I'm not fully understanding why there's certain decisions around separating stations, path segments, etc.

From what I understand, the design has:

ContactGeometry, somewhere else in the model

A top-level path class (Scholz2015GeometryPath)

A top-level origin Station (Scholz2015GeometryPath::origin)

A top-level insertion Station (Scholz2015GeometryPath::insertion

Top-level via_point Stations

Top-level segments, which may refer to any of {origin,via_points,insertion} but, coding-wise, may refer to any Station in the model.

Obstacles, which are effectively a reference to the ContactGeometry, plus a contact hint

Other stuff related to the internals of the algorithm

And, as a direct result of this design, every time a caller calls addViaPoint the implementation is internally going to create a new Scholz2015GeometryPathSegment and assign the currently-unassigned obstacles to that segment, if I understand that correctly? Any left-over (unassigned) obstacles will end up in the final path segment?

There's several things to consider from a design PoV when using this via the API or loading it directly from an XML file:

The origin/insertion/viapoints don't appear to be strictly required. A Scholz2015GeometryPathSegment loaded from an osim can potentially socket to any Station in the model. The stations are essentially just a holding spot for "Stations this path handles". You could just as equally use the native ComponentSet of the path's base class (Component) to handle ownership.

The sequencing of the Stations doesn't appear to matter because sequence is actually encoded by Scholz2015GeometryPathSegment

Because Scholz2015GeometryPathSegment` is strictly a two-point segment, and the implementation isn't responsible for slicing it up, the caller/XML can declare disjoint path segments (e.g. you could add path segment A-B then E-F)

I'd strongly consider what the simplest UX could be and code around it at runtime, rather than directly exposing end-users to all of these decisions. Otherwise, you're asking them to keep quite a lot of concerns in-mind (e.g. the various station collections, obstacles, hittests are local to the contact geometry O_o) to save having to do some juggling at system-building time.

E.g. users are already familiar with providing a sequence of points and providing all of the obstacles they think it should interact with. An alternate design would be to piggyback on similar concepts available from GeometryPath, but perhaps provide a ContactHintPathPoint with the explanation that "it's kind of like a via-point, but only during initialization, and only if collision is detected during initialization". That way, the user provides what they are already familiar with providing (a sequence of points, plus a list of obstacles), then you can mess around internally to forward the correct information to the underlying CableSpan API.

Just my 2c but, knowing how users build models, anything even slightly complicated will be tough to rollout. There is also a meta concern/desire to port existing models to the new algorithm, which will be significantly easier to do if the new design is essentially the old one with a couple of extra steps.

I've tried to take this all into account with the recent API changes in the last commit. This brings us closer to the GeometryPath API, and I think is generally an improvement. As I've mentioned

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 413 at r1 (raw file):

Previously, adamkewley (Adam Kewley) wrote…

Is this logically also the case when the last path segment's insertion is the final insertion point? Juggling between the various datastructures/segments makes it unclear.

The recent changes should make this clearer.

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 524 at r1 (raw file):

Previously, adamkewley (Adam Kewley) wrote…

Seems rather internal? Why would a public API user have/know the underlying SimTK::CableSpanObstacleIndex?

Done.

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

Previously, adamkewley (Adam Kewley) wrote…

... and there's at least one curve segment
... and the curve segments are connected to one another
... and the origin point corresponds to the first point in the first segment
... and the insertion point corresponds to the last point in the last segment

Done.

…bstacles

… are stored as an ordered list of path elements

nickbianco · 2025-09-29T19:31:14Z

I've taken the redesign one step further: now both Scholz2015GeometryPathPoint (new class) and Scholz2015GeometryPathObstacle derive from Scholz2015GeometryPathElement, an abstract base class. An ordered list of Scholz2015GeometryPathElements are stored in the new property path_elements, which means that path points and obstacles will appear in XML in the same order as added by the user via the API.

The drawback of this approach is the inclusion of the empty abstract base class Scholz2015GeometryPathElement. I would have preferred to use something like std::variant to store the list of path points and obstacles instead, but something like the following does not seem to play well with OpenSim's property system:

using Scholz2015GeometryPathElement = 
        std::variant<Scholz2015GeometryPathPoint, Scholz2015GeometryPathObstacle>;
OpenSim_DECLARE_LIST_PROPERTY(path_elements, Scholz2015GeometryPathElement,
        "The list of elements (path points or obstacles) defining the path.");

In any case, I still think this is an improvement, especially from a user perspective (e.g., if a user does decide to inspect or modify XML, it should be clearer how the path is constructed).

aymanhab

I like the uniformity of treatment but worry a lot about where the PathPoints end up in the hierarchy since there's downstream code that depends on the points implementing the contract of AbstractPathPoint. Will see if this shows up but wanted to document my initial thoughts..

@aymanhab reviewed 1 of 8 files at r2.
Reviewable status: 16 of 26 files reviewed, 32 unresolved discussions (waiting on @adamkewley, @carmichaelong, and @nickbianco)

aymanhab

Generally I like the approach, the main issues I have are: PathPoints that are not types of OpenSim::AbstractPathPoint and the exposure of way too many knobs/tolerances/iterationCounts that users or API users have no way to initialize or tune.

Reviewable status: 16 of 26 files reviewed, 40 unresolved discussions (waiting on @adamkewley, @carmichaelong, and @nickbianco)

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 322 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

This controls the step size of the geodesic integrator, so yes, it should be in length units, but it's not 100% clear from the Simbody documentation. But I think it's likely we hide this setting, at least for now.

Please hide, too many decisions for us or users to make in the dark.

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 385 at r1 (raw file):

Previously, adamkewley (Adam Kewley) wrote…

Do public API users actually need to know what's happening internally? Their job is (effectively) to stuff properties into Components, compile, and get outputs.

Agreed 👍

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 68 at r6 (raw file):

OpenSim_DECLARE_CONCRETE_OBJECT(Scholz2015GeometryPathPoint,
        Scholz2015GeometryPathElement);
public:

Reiterating my concern that the API for AbstractPathPoint, would have to be duplicated/reimplemented here. Examples: getLocation(state), getParentFrame, isActive, all the socket wiring... to be its own branch of the hierarchy feels problematic. Why not use socket to connect to a separate PathPoint similar to how Obstacles are connected to ContactGeometry?

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 74 at r6 (raw file):

    OpenSim_DECLARE_PROPERTY(station, Station,
            "The Station representing this path point.");

Calling this a "path point" is now very confusing!

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 154 at r6 (raw file):

 * curved segment over each obstacle, and straight segments connecting path
 * points to obstacles. If no obstacle lies between two path points, the points
 * will be connected by a straight line segment. The path is computed as a

Again these have to be qualified as this special kind of path point that is not actually related to all other PathPoint(s) in the class hierarchy even though the name implies it!

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 318 at r6 (raw file):

        "The maximum number of solver iterations for finding the optimal path "
        "(default: 50).");

Is that 50 regardless of all the other knobs/options? Justification?

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 332 at r6 (raw file):

     */
    void addPathPoint(const PhysicalFrame& frame, const SimTK::Vec3& location);

I like that you kept the same interface to add a "Point"

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 459 at r6 (raw file):

     */
    void setSolverMaxIterations(int maxIterations);

Compared to the old wrapping, none of these parameters or knobs were ever exposed, too many knobs invite users to make up numbers that end up problematic downstream.

Bindings/Java/Matlab/examples/Wrapping/exampleScholz2015GeometryPath.m line 49 at r6 (raw file):

% adding it to the PathSpring, so that the Socket connections in each
% Station (i.e., path point) remain valid.
path = Scholz2015GeometryPath.safeDownCast(spring.updPath());

Seems awkward, and potentially error prone to "require" updating the path after adding it to the PathSpring because of our internal book-keeping.

Bindings/Java/Matlab/examples/Wrapping/exampleScholz2015GeometryPath.m line 82 at r6 (raw file):

% Use the "minimum length" algorithm, which solves for a path that
% minimizes the total length of the path.

As educational example, not sure what are the other options for algorithm, a pointer or mention would be useful.

adamkewley · 2025-10-02T06:16:38Z

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 459 at r6 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Compared to the old wrapping, none of these parameters or knobs were ever exposed, too many knobs invite users to make up numbers that end up problematic downstream.

👍

adamkewley

Just had a quick scan through and checked Reviewable: I haven't read it comprehensively this time but hopefully the first review conveyed enough information (about the intended design, etc.)

@adamkewley reviewed 6 of 9 files at r5, 4 of 4 files at r6, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @adamkewley, @aymanhab, @carmichaelong, and @nickbianco)

…; simplify addObstacle() docs

nickbianco

Thanks @ayman and @adamkewley. I've removed all of the accessors (i.e., "knobs") to solver-specific SimTK::CableSpan settings and settled on some default settings that seem to perform well across all tests and examples. @aymanhab, I'm not sure I fully understand your concerns related to path points; I've left some comments below.

Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @adamkewley, @aymanhab, and @carmichaelong)

Bindings/Java/Matlab/examples/Wrapping/exampleScholz2015GeometryPath.m line 49 at r6 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Seems awkward, and potentially error prone to "require" updating the path after adding it to the PathSpring because of our internal book-keeping.

I will look into an alternative for this.

Bindings/Java/Matlab/examples/Wrapping/exampleScholz2015GeometryPath.m line 82 at r6 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

As educational example, not sure what are the other options for algorithm, a pointer or mention would be useful.

Done.

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 68 at r6 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Reiterating my concern that the API for AbstractPathPoint, would have to be duplicated/reimplemented here. Examples: getLocation(state), getParentFrame, isActive, all the socket wiring... to be its own branch of the hierarchy feels problematic. Why not use socket to connect to a separate PathPoint similar to how Obstacles are connected to ContactGeometry?

I'm not sure I understand or share your concerns here. Are you suggesting that we should reuse PathPoint here? I was operating under the assumption that PathPoint was mostly tailored to GeometryPath and tried to implement the minimal set of data structures needed, while staying fully uncoupled from the old wrapping code.

The API for AbstractPathPoint is very similar to what Station provides, with some extra methods that may not be necessary for
Scholz2015GeometryPath.

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 74 at r6 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Calling this a "path point" is now very confusing!

How so? Should we call the property path_point?

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 154 at r6 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Again these have to be qualified as this special kind of path point that is not actually related to all other PathPoint(s) in the class hierarchy even though the name implies it!

The comments here are referring to a "path point" in a general sense. I'm not sure this is the place to go into the difference between PathPoint and Scholz2015GeometryPath.

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 318 at r6 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Is that 50 regardless of all the other knobs/options? Justification?

Done.

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 332 at r6 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

I like that you kept the same interface to add a "Point"

Done.

OpenSim/Simulation/Model/Scholz2015GeometryPath.h line 459 at r6 (raw file):

Previously, adamkewley (Adam Kewley) wrote…

👍

Done.

aymanhab · 2025-10-03T16:50:12Z

I totally sympathize @nickbianco just that the mental picture we engrained in the minds of API users has been this (where classes ending in PathPoint are some type of AbstractPathPoint

not any more ... At a higher level, classes "should" represent abstractions not tied to specific implementations so the fact that we can't reuse any of the classes in the PathPoint hierarchy in another implementation of the Path is a clear symptom of a problem. Definitely not of your making.

adamkewley

Just had another read-through: seems like sane-enough changes but I can completely sympathize with @aymanhab that having something that seems to resemble a point abstraction hanging around in a codebase that already has Point, PathPoint, Station, AbstractPathPoint, Marker, ConditionalPathPoint etc. is unfortunate!

@adamkewley reviewed 6 of 6 files at r7, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @aymanhab and @carmichaelong)

nickbianco

Thanks @aymanhab, I better understand your concerns now, and agree that this is an unfortunate consequence of the existing class hierarchy. It would be reasonable to make Scholz2015GeometryPathPoint a part of the existing hierarchy, but currently that would break my current implementation, since it needs to derive from Scholz2015GeometryPathElement (unless we resort to a solution involving multiple inheritance, which I wouldn't want to do). I'll need to think about an alternative solution; any suggestions in the meantime would be helpful.

Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @aymanhab and @carmichaelong)

aymanhab · 2025-10-06T17:57:29Z

Thanks for understanding and for your flexibility @nickbianco 🥇 I hate to stand in the way of progress so I'd be ok with approving this PR while working on the issue with PathPoints. I'd just point out that there're many places (within the rather complicated path editor code in the GUI, and visualization/visual-editing support code, that heavily use PathPoints and so may need to be rewritten/rewired to account for the new classes/design which is definitely cleaner/simpler. Have you considered using mixins of the PathElement into the PathPoint class thus fitting in the hierarchy while supporting the use of the PathPoint as a PathElement? Just let me know what you decide. Thank you for the nice work 👍

nickbianco requested a review from aymanhab September 17, 2025 20:22

nickbianco marked this pull request as ready for review September 17, 2025 20:23

nickbianco requested a review from adamkewley September 18, 2025 14:14

carmichaelong reviewed Sep 18, 2025

View reviewed changes

adamkewley requested changes Sep 19, 2025

View reviewed changes

Remove raw pointers in via point/obstacle index lists; remove unneces…

c574766

…sary calls to Component constructor; various comment, test, and error-checking improvements

nickbianco commented Sep 19, 2025

View reviewed changes

aymanhab reviewed Sep 24, 2025

View reviewed changes

aymanhab requested review from adamkewley and carmichaelong September 25, 2025 19:54

aymanhab reviewed Sep 25, 2025

View reviewed changes

nickbianco commented Sep 25, 2025

View reviewed changes

Constructing Scholz2015GeometryPaths from a list of path points and o…

4d58303

…bstacles

nickbianco force-pushed the scholz2015_geometry_path branch from ddcc9bd to 4d58303 Compare September 25, 2025 21:36

Refactor Scholz2015GeometryPath so that all path points and obstacles…

9cce2f4

… are stored as an ordered list of path elements

aymanhab reviewed Sep 30, 2025

View reviewed changes

aymanhab requested changes Oct 1, 2025

View reviewed changes

adamkewley requested changes Oct 2, 2025

View reviewed changes

Remove accessors for solver settings and encode conservative defaults…

ac96e46

…; simplify addObstacle() docs

nickbianco commented Oct 2, 2025

View reviewed changes

adamkewley approved these changes Oct 6, 2025

View reviewed changes

nickbianco commented Oct 6, 2025

View reviewed changes

Add class Scholz2015GeometryPath #4156

Are you sure you want to change the base?

Add class Scholz2015GeometryPath #4156

Uh oh!

Conversation

nickbianco commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Brief summary of changes

Testing I've completed

Looking for feedback on...

CHANGELOG.md (choose one)

Uh oh!

nickbianco commented Sep 17, 2025

Uh oh!

adamkewley commented Sep 18, 2025

Uh oh!

carmichaelong left a comment

Choose a reason for hiding this comment

Uh oh!

adamkewley left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickbianco left a comment

Choose a reason for hiding this comment

Uh oh!

adamkewley commented Sep 20, 2025

Uh oh!

aymanhab left a comment

Choose a reason for hiding this comment

Uh oh!

aymanhab left a comment

Choose a reason for hiding this comment

Uh oh!

nickbianco left a comment

Choose a reason for hiding this comment

Uh oh!

nickbianco commented Sep 29, 2025

Uh oh!

aymanhab left a comment

Choose a reason for hiding this comment

Uh oh!

aymanhab left a comment

Choose a reason for hiding this comment

Uh oh!

adamkewley commented Oct 2, 2025

Uh oh!

adamkewley left a comment

Choose a reason for hiding this comment

Uh oh!

nickbianco left a comment

Choose a reason for hiding this comment

Uh oh!

aymanhab commented Oct 3, 2025

Uh oh!

adamkewley left a comment

Choose a reason for hiding this comment

Uh oh!

nickbianco left a comment

Choose a reason for hiding this comment

Uh oh!

aymanhab commented Oct 6, 2025

Uh oh!

Uh oh!

Add class `Scholz2015GeometryPath` #4156

Add class `Scholz2015GeometryPath` #4156

nickbianco commented Sep 17, 2025 •

edited

Loading

adamkewley left a comment •

edited

Loading