Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix chained pofs or sdfs in model #3711

Merged
merged 7 commits into from
Feb 27, 2024
Merged

Conversation

adamkewley
Copy link
Contributor

@adamkewley adamkewley commented Feb 19, 2024

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:

  • Modify PhysicalOffsetFrame's extendAddToSystem to instead set its own body index based on the base frame it's attached to (previously, it would use getParentFrame, which doesn't account for chains of frames)
  • Modify PhysicalOffsetFrame with a new extension member method, extendSetMobilizedBodyIndex:
    • Lets downstream Frame implementations transitively set body indices based on what they know about their frame topology (e.g. a PhysicalOffsetFrame "knows" it has a parent)
  • Remove dynamic_casting to PhysicalOffsetFrame from joint:
    • The joint now just calls setMobilizedBodyIndex
    • Which has its old behavior
    • But PhysicalOffsetFrame/StationDefinedFrame now use the extension point to (e.g.) ensure that the MobilizedBodyIndex is set on frames that they are transitively attached to
    • This new approach is automatically recursive (e.g. calling it on a chain of PoFs will call setMobilizedBodyIndex on each element in the chain, rather than just the parent)

API CHANGES (NONE)

setMobilizedBodyIndexis nowpublic, rather than protected` (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

  • Added a unit test that exercises the problem (that you can't chain POFs together when joints are involved). The previous PoF-chaining tests don't include joints
  • The tests failed
  • I wrote this proposed fix
  • The new test passed, but a "known bug" test in StationDefinedFrame then failed
  • The StationDefinedFrame tests were re-enabled/reversed (as documented)
  • All tests now pass

CHANGELOG.md (choose one)

  • no need to update because it's a patch to a recent feature, broken out into this PR so it can be treated in isolation

This change is Reviewable

@adamkewley
Copy link
Contributor Author

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

Binary builds available via Actions

@adamkewley
Copy link
Contributor Author

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:

  • ground <-- pof
  • pof <-- pof2
  • pof2 <-- FreeJoint
  • FreeJoint --> StationDefinedFrame
  • StationDefinedFrame defined via stations on 3 RGB spheres as stationX --> mesh_pof --> body`
  • StationDefinedFrame <-- FreeJoint2
  • FreeJoint2 --> StationDefinedFrame2
  • StationDefinedFrame2 defined via a mixture of stations defined on RG cubes defined via chains of PoFs onto a cylnder model and a station that's directly defined on the body frame

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
Copy link
Contributor Author

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.

Copy link
Contributor

@pepbos pepbos left a comment

Choose a reason for hiding this comment

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

Well, had to go through some mental transitive loops, but it terminated at an empty extendSetMobilizedBodyIndex() ;)

Was wondering if setMobilizedBodyIndexOf should be static?

Looks good to me :)

OpenSim/Simulation/Model/PhysicalFrame.h Outdated Show resolved Hide resolved
@adamkewley adamkewley merged commit ca00354 into main Feb 27, 2024
6 checks passed
@adamkewley
Copy link
Contributor Author

Thanks @pepbos

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants