Closed adamkewley closed 4 months ago
Downstream throwaway PR created in opensim-creator
that uses this PR, so that I can play with StationDefinedFrame
as a user-facing feature:
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).
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 MobilizedBodyIndex
es 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.
Downstream updated with this latest commit:
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
OSC appears to be fine with adding/using StationDefinedFrame
s in the limited case that they are used for (e.g.) joint parents. They still cannot be used in longer chains of PhysicalOffsetFrame
s, 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.
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
My general conclusions from a longer discussion on this:
StationDefinedFrame
, rather, it'll mean it can be used in more situations)Details:
The new traversal resembles/matches the old PhysicalOffsetFrame
one
The concerns about certain conditions not working (e.g. chained PhysicalOffsetFrame
s 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 PhysicalFrame
s is hard-coded to only assign the index to the direct parent if it happens to be a PhysicalOffsetFrame
:
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_cast
s for PhysicalOffsetFrame
and StationDefinedFrame
is a code smell). There is also the idea of re-engineering the graph traversal entirely to account for Socket
s, but I am going to keep that separate.
@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!
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 PhysicalOffsetFrame
s, 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)
Adds a new component,
StationDefinedFrame
, which is aPhysicalFrame
that has its position/orientation automatically derived fromStation
s 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 withOffsetFrame
s). More information available on theStationDefinedFrame
's comment string.Brief summary of changes (WIP)
StationDefinedFrame
componenttestStationDefinedFrame.cpp
) to ensureStationDefinedFrame
can be added to a model and used in JointstestStationDefinedFrame.cpp
) to ensure that it works with different topologies etc.Model.cpp
:Frame
sStationDefinedFrame
s need to be topologically sorted w.r.t.PhysicalOffsetFrame
etc. whenextendAddToSystem
is called3697 generalizes the topological sorting further to also sort w.r.t. all
Component
s in the model. This was broken out into a separate PR because it's to cover a broader range of frame use-casesStationDefinedFrame
(BasicModelWithStationDefinedFrame.osim
)Testing I've completed
Looking for feedback on...
CHANGELOG.md (choose one)
Demo
Available from GitHub Actions builds of this PR:
Lets you add
StationDefinedFrame
into a model. You can thenAdd Body
and choose theStationDefinedFrame
as the parent. BEWARE, THOUGH: there's a bug inopensim-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 onopensim-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 asOpenSim::Joint
frames. This is because of various technical issues, such as the fact that we built on top ofOpenSim::Point
, rather thanOpenSim::Station
. It also requires baking the resulting model intoPhysicalOffsetFrame
s, 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
Q: Why
StationDefinedFrame
, rather than (e.g.)PointDefinedFrame
?A: OSC has components such as
EdgeDefinedFrame
andPointDefinedFrame
, but that was a bad idea.Point
s may be defined in frames that are indirectly dependent on other frames that are defined using thePoint
s, 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 aSimTK::State
is available (Point
is only usable after aSimTK::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,Station
s aren't computed, which means that concepts in OSC (e.g.Midpoint
) can't be used with aStationDefinedFrame
. Doing so requires defining a new virtual API called (e.g.)RigidPoint
, whichStation
(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
andab_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
, andpoint_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 thatStationDefinedFrame
aims for is to ask the user for 3/4 locations, followed by allowing the user to tweakab_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?
StationDefinedFrame
's base frame is derived from one of theStation
sStationDefinedFrame
's definition spanned multiple base frames then its transform would depend on the relative motion of those base frames - 💥This change is![Reviewable](https://reviewable.io/review_button.svg)