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

Expose more simbody methods needed by advanced users #3618

Merged
merged 4 commits into from
Nov 21, 2023

Conversation

aymanhab
Copy link
Member

@aymanhab aymanhab commented Nov 20, 2023

Fixes issue #3414, #3490

Brief summary of changes

Added 3 methods previously not accessible to advanced users.

Testing I've completed

Diffed resulting Model.java files and the three methods were added and are utilizing other existing classes

Looking for feedback on...

Documentation/description of the methods and if there're other related questions/concerns

CHANGELOG.md (choose one)

  • updated.

This change is Reviewable

@aymanhab
Copy link
Member Author

@nickbianco not sure if you have a minute to review this. Thank you

Copy link
Member

@nickbianco nickbianco left a comment

Choose a reason for hiding this comment

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

One quick question.

@@ -522,7 +522,21 @@ OpenSim_OBJECT_NONTEMPLATE_DEFS(Model, ModelComponent);
GeneralForceSubsystem allocated by this %Model. **/
SimTK::GeneralForceSubsystem& updForceSubsystem()
{ return *_forceSubsystem; }

/** (Advanced) Get read only access to internal Simbody RigidBodyForces at Dynamics stage **/
const SimTK::Vector_<SimTK::SpatialVec>& getRigidBodyForces(const SimTK::State& state)
Copy link
Member

Choose a reason for hiding this comment

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

Does SimTK::Vector_<SimTK::SpatialVec> get wrapped properly in the bindings? I don't see a %template for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it does here's the signature from the newly generated Model.java
public VectorOfSpatialVec getGravityBodyForces(State state) {
....
}

Copy link
Member

Choose a reason for hiding this comment

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

Should we create our own SWIG template to follow the convention for other vectors like VectorVec3? Perhaps VectorSpatialVec? I definitely wouldn't know to search for the signature VectorOfSpatialVec without knowing it existed already. (I'm assuming VectorOfSpatialVec is auto-generated by SWIG).

@aymanhab
Copy link
Member Author

Failure is the other intermittent osx failure unrelated to this PR.

@aymanhab
Copy link
Member Author

@nickbianco The name of the class is not swig generated, it's defined here

namespace SimTK {

since at least 2015

I agree there's inconsistency with other naming conventions but it's hard to tell why it was done this way or who is using these classes so a rename would be breaking backward compatibility. We can discuss further at dev meeting

@nickbianco
Copy link
Member

@aymanhab gotcha, not sure how I missed that while searching. In that case, it doesn't make sense to potentially break backwards-compatibility with this PR.

Copy link
Member

@nickbianco nickbianco left a comment

Choose a reason for hiding this comment

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

LGTM.

@aymanhab
Copy link
Member Author

Thanks @nickbianco Much appreciated 👍

@aymanhab aymanhab merged commit 3b2c8b8 into main Nov 21, 2023
5 of 6 checks passed
@aymanhab aymanhab deleted the expose_simbody_extra_methods branch November 21, 2023 19:01
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.

2 participants