Add initial implementation of StationDefinedFrame

[adamkewley]

Adds a new component, StationDefinedFrame, which is a PhysicalFrame that has its position/orientation automatically derived from Stations in the model.

The utility of a StationDefinedFrame is that it more closely (note: not entirely closely) matches ISB's Grood-Suntay-style frame definitions. Those definitions are usually given in terms of fixed axes, cross products between points in space, etc., rather than as a hard (position, orientation) tuple (which is effectively what we're currently doing with OffsetFrames). More information available on the StationDefinedFrame's comment string.

Brief summary of changes (WIP)

• Adds StationDefinedFrame component
• Adds basic tests (testStationDefinedFrame.cpp) to ensure StationDefinedFrame can be added to a model and used in Joints
• Adds edge-case/logic tests (testStationDefinedFrame.cpp) to ensure that it works with different topologies etc.
• Changes Model.cpp:
  • The way in which the component ordering is computed was refactored to use a general topological sort on the Frames
  • The reason this is necessary is because StationDefinedFrames need to be topologically sorted w.r.t. PhysicalOffsetFrame etc. when extendAddToSystem is called
  • [WIP] Fix Model::extendConnectToModel graph traversal #3697 generalizes the topological sorting further to also sort w.r.t. all Components in the model. This was broken out into a separate PR because it's to cover a broader range of frame use-cases
• Adds a basic example of a model that uses a StationDefinedFrame (BasicModelWithStationDefinedFrame.osim)

Testing I've completed

• Used in an experimental branch of OpenSim Creator
• Whatever the unit test suite does

Looking for feedback on...

• Anything

CHANGELOG.md (choose one)

• Updated.

---

Demo

Available from GitHub Actions builds of this PR:

• Feature station defined frames rolled out to OpenSim Creator ComputationalBiomechanicsLab/opensim-creator#823

Lets you add StationDefinedFrame into a model. You can then Add Body and choose the StationDefinedFrame as the parent. BEWARE, THOUGH: there's a bug in opensim-core (mentioned+tested in the added unit test suites, requires #3697 to fix it) where chaining two dependent frames as a joint parent doesn't work (e.g. Ground <-- PoF <-- PoF <-- Joint --> Body does not work, even on opensim-core/main, when I tried).

---

Extra Information

The reason StationDefinedFrame is closer, but not the same as the ISB's frame definitions is because ISB's definitions also include concepts such as "the centroid between two condyls" or "the center of the femoral head", or "the cross product between this edge and another cross product edge".

OpenSim Creator's specialized Frame Definition UI (available on the splash screen) has allowances for this, and was built with concepts such as Edge, Midpoint, etc. However, OSC doesn't support using the frames as OpenSim::Joint frames. This is because of various technical issues, such as the fact that we built on top of OpenSim::Point, rather than OpenSim::Station. It also requires baking the resulting model into PhysicalOffsetFrames, for compatibility with non-OSC codebases.

Adding support for these concepts can come later to OpenSim in the form of StationDefinedCentroid, RigidPoint, etc. This PR is a very stripped down (mathematically, the bare minimum you can get away with) implementation that doesn't rely on other concepts (otherwise, this PR would contain 5 new classes).

---

F&Q

ℹ️ These questions/changes popped up during development, so I have written them here in case people ask in a few years time.

Q: Why StationDefinedFrame, rather than (e.g.) PointDefinedFrame?

A: OSC has components such as EdgeDefinedFrame and PointDefinedFrame, but that was a bad idea.

Points may be defined in frames that are indirectly dependent on other frames that are defined using the Points, creating a circular dependency. Additionally, Point is incompatible with simbody's system creation: the base frames, and relative orientation within those base frames, needs to be known before a SimTK::State is available (Point is only usable after a SimTK::State is available).

Station is defined as rigidly fixed with respect to a base frame and is guaranteed have a state-independent motion with respect to that frame. However, Stations aren't computed, which means that concepts in OSC (e.g. Midpoint) can't be used with a StationDefinedFrame. Doing so requires defining a new virtual API called (e.g.) RigidPoint, which Station (and computed locations) would derive from. That would be a later PR.

Q: Why "three triangle points, plus a location"

A: Guarantees that the implementation can create a right-handed coordinate system at a given offset w.r.t. a base frame. We tried designs like "pick two orthogonal edges", but it's error-prone if the user doesn't select two actually-orthogonal edges. This design ensures that--so long as none of the 3 points are co-located--the implementation will be able to render a Transform from the inputs.

Q: Why are ab_axis and ab_x_ac_axis exposed as user-editable properties?

A: Practical ease-of-use. The user may be able to swap around the point_a, point_b, and point_c sockets to achieve a similar effect, but that's quite a lot of faffing around - especially if swapping sockets isn't easy. The proposed UX that StationDefinedFrame aims for is to ask the user for 3/4 locations, followed by allowing the user to tweak ab_axis/ab_x_ac_axis in the property editor of OpenSim Creator or OpenSim GUI until they have the orientation they desire.

Q: Why must all stations be defined w.r.t. the same base frame?

• A1: StationDefinedFrame's base frame is derived from one of the Stations
• A2: It guarantees that the resulting frame transform is state-independent. If the StationDefinedFrame's definition spanned multiple base frames then its transform would depend on the relative motion of those base frames - 💥

---

This change is 

[adamkewley]

Downstream throwaway PR created in opensim-creator that uses this PR, so that I can play with StationDefinedFrame as a user-facing feature:

• Feature station defined frames rolled out to OpenSim Creator ComputationalBiomechanicsLab/opensim-creator#823

[adamkewley]

The PR built, passed tests, etc. and I was able to load a very basic example model with the topology:

• Ground <-- 4 x Stations <-- StationDefinedFrame <-- PinJoint --> PhysicalOffsetFrame --> Body

Into OSC, yielding a model that has a frame that can be modified by moving the associated stations around, or by changing (e.g.) ab_axis:

The rotation + position of the pictured frame is entirely controlled by the locations of the 4 stations (the origin one being separate from the points in this example - but it doesn't have to be).

[adamkewley]

The implementation now mostly works, but there appears to be a bug (or a mis-use in the test suite) that makes pathological use-cases not work as intended.

E.g. this kind of topology appears to be broken in opensim-core/main and this PR:

• Ground <-- PhysicalOffsetFrameA <-- PhysicalOffsetFrameB <-- Joint --> Body

Because, when the Joint is being added to the system, it ends up asking for MobilizedBodyIndexes that aren't yet available (note: the same problem doesn't happen if there's only one PhysicalOffsetFrame, which implies immediate connectees are handled).

The model in the test suite (failing) topology with many dependent frames/stations, and I'm going to look into why this, or the simpler example above, don't work.

[adamkewley]

Downstream updated with this latest commit:

• Feature station defined frames rolled out to OpenSim Creator ComputationalBiomechanicsLab/opensim-creator#823

I'll test it in the UI to see if it's behaving itself (within the known constraints caused by model graph traversal issues) and then un-WIP this

[adamkewley]

OSC appears to be fine with adding/using StationDefinedFrames in the limited case that they are used for (e.g.) joint parents. They still cannot be used in longer chains of PhysicalOffsetFrames, or as joint children, because of graph traversal bugs. However, fixing those won't change the general implementation strategy and user-facing API of StationDefinedFrame, so this should be good for review/shipping now.

[pepbos]

Very nice. Actually have little to comment.

Really liked the test suite.

[pepbos]

typo: recursive

[pepbos]

It no longer throws right? But it does the fallback + warning?

[nickbianco]

Agreed with @pepbos, very nice!

A few minor comments to clarify the axis properties, but otherwise .

Reviewed 6 of 7 files at r1, 1 of 3 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @adamkewley)

---

OpenSim/Simulation/Model/StationDefinedFrame.h line 107 at r3 (raw file):

public:
    OpenSim_DECLARE_PROPERTY(ab_axis, std::string, "The frame axis that points in the direction of `point_b - point_a`. Can be `-x`, `+x`, `-y`, `+y`, `-z`, or `+z`. Must be orthogonal to `ab_x_ac_axis`.");
    OpenSim_DECLARE_PROPERTY(ab_x_ac_axis, std::string, "The frame axis that points in the direction of `(point_b - point_a) x (point_c - point_a)`. Can be `-x`, `+x`, `-y`, `+y`, `-z`, or `+z`. Must be orthogonal to `ab_axis`.");

After reading the doc string I at first thought this information would be optional, but it is necessary to "register" the axes created by the points to the frame axes. Consider making a small note of this above.

---

OpenSim/Simulation/Model/StationDefinedFrame.h line 124 at r3 (raw file):

        const Station& pointC,
        const Station& originPoint
    );

Optional: Would there be value in adding a constructor that only requires the four points and uses your default ab_axis and ab_x_ac_axis values? Or do you think this could cause confusion later on for the user?

Similar to my comment above about the properties, I was suprised that abAxis and abXacAxis were needed here after reading the doc string. If the purpose of those two arguments are clarified then perhaps another constructor is superfluous.

---

OpenSim/Simulation/Test/testStationDefinedFrame.cpp line 85 at r3 (raw file):

        static_assert(std::is_base_of<Joint, T>::value, "T must inherit from Joint");
        return EmplaceGeneric<T>(model, std::mem_fn(&Model::addJoint), std::forward<Args>(args)...);
    }

+1

Bookmarking this as a future improvement to Model to avoid raw pointers when model building.

[adamkewley]

Apart from fixing the review comments, the model graph traversal algorithm needs to be re-checked: there was some discussion about whether it introduces different behavior for PoFs

[adamkewley]

My general conclusions from a longer discussion on this:

• Fix the review comments
• Fix the hard-coded assignment in https://github.com/opensim-org/opensim-core/blob/main/OpenSim/Simulation/SimbodyEngine/Joint.cpp#L741
• Ship (albeit, with some known quirks related to graph traversals - but fixing the quirks wouldn't affect the backwards compatibility of StationDefinedFrame, rather, it'll mean it can be used in more situations)
• Separate PR: Look into a new base class for computed frames
• Separate PR: Look into a new graph traversal technique that handles things like chains of PoFs, etc. generically

Details:

• The new traversal resembles/matches the old PhysicalOffsetFrame one

• The concerns about certain conditions not working (e.g. chained PhysicalOffsetFrames as a Joint parent) are bugs that are present in the original code. I can reproduce them by (e.g.) failing to load those cases in other versions of OpenSim (e.g. if you create a joint to a PoF to a PoF then that'll fail - even here). The reason it initially failed is because of socket traversal issues (fixed by making sockets lazier), and the reason it later fails is because

• The reason why StationDefinedFrame cannot be used as a joint child is because the way in which body indices are assigned to PhysicalFrames is hard-coded to only assign the index to the direct parent if it happens to be a PhysicalOffsetFrame:

• https://github.com/opensim-org/opensim-core/blob/main/OpenSim/Simulation/SimbodyEngine/Joint.cpp#L741

That code will need to be changed to account for both transitive dependencies (e.g. chains of PoFs would have this issue, but StationDefinedFrame is more likely to have it because of its transitive dependencies on other frames) and to account for the fact that PhysicalOffsetFrame isn't the only thing that can be a dependent frame.

Further discussion was around the idea that we might need another interface between PhysicalOffsetFrame and PhysicalFrame, e.g. called PhysicalComputedFrame or similar, so that there is one uniform interface to downcast against (having two dynamic_casts for PhysicalOffsetFrame and StationDefinedFrame is a code smell). There is also the idea of re-engineering the graph traversal entirely to account for Sockets, but I am going to keep that separate.

[adamkewley]

@nickbianco

Optional: Would there be value in adding a constructor that only requires the four points and uses your default ab_axis and ab_x_ac_axis values? Or do you think this could cause confusion later on for the user?

I'd prefer it to be explicit for now: if users start demanding it, then it's easy enough to patch in, though!

[adamkewley]

Merging this, because the feature works, the model graph traversal is equivalent to the previous code etc.

However, there's a few issues with how opensim-core traverses dependent frames, such as PhysicalOffsetFrames, which means that you can't (e.g.) chain them on either side of a joint.

I have patches for those issues, but will PR them separately to prevent this PR from becoming a death-walk - thanks @pepbos and @nickbianco for reviewing it (the patches should be less treacherous :D)