opensim-org / opensim-core

SimTK OpenSim C++ libraries and command-line applications, and Java/Python wrapping.
https://opensim.stanford.edu
Apache License 2.0
759 stars 308 forks source link

Fix chained pofs or sdfs in model #3711

Closed adamkewley closed 4 months ago

adamkewley commented 4 months ago

Fixes an issue found during #3694 where chaining PhysicalOffsetFrames or StationDefinedFrames does not work.

The reason it doesn't work is because the previous implementation does things like dynamic_casting to PhysicalOffsetFrame followed by hard-setting its parent's MobilizedBodyIndex. That approach doesn't take into account that (e.g.) the parent may, itself, be a PhysicalOffsetFrame (a correct implementation should also iterate through the whole chain). It also doesn't account for other dependent frame types (e.g. StationDefinedFrame).

To work around this, the following changes were necessary:

API CHANGES (NONE)

setMobilizedBodyIndexis nowpublic, rather thanprotected` (it is still labelled as advanced, though).

A new method was added, setMobilizedBodyIndexOf, which keeps the existing API backwards-compatible but lets frames set eachovers indices (required)

Testing I've completed

CHANGELOG.md (choose one)


This change is Reviewable

adamkewley commented 4 months ago

Downstream PR of opensim-creator with this change, for basic sanity checks:

Binary builds available via Actions

adamkewley commented 4 months ago

StationDefinedFrames_Advanced.osim is a much more comprehensive test of how chains of PhysicalOffsetFrames and StationDefinedFrames can interplay:

StationDefinedFrame_Advanced_AnnotatedScreenshot

Not obvious from this screenshot, but the topology here is something like:

Little bit pathological, but the idea is that users might be combining mesh data, landmarks, etc. to build frames dynamically, so the engine needs to handle mixed-, chained-, etc. frames.

adamkewley commented 4 months ago

The implementation was updated to mean that there's now no user-facing API changes at all: what was protected is still protected. The only changes are additions extendSetMobilizedBodyIndex and setMobilizedBodyIndexOf(someotherframe), which are the bare-minimum necessary to ensure that downstream PhysicalFrame implementations can customize their own index behavior and also set indices on other frames when it makes sense.

adamkewley commented 4 months ago

Thanks @pepbos