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

MobilizedBody::Translation has confusing getMobilizerAcceleration() method #604

Closed
apalazzi opened this issue Jan 4, 2018 · 5 comments
Closed

Comments

@apalazzi
Copy link

apalazzi commented Jan 4, 2018

[Edited by sherm: see my comment below for a revised description.
The original description is here:]

the method MobilizedBody::getMobilizerAcceleration() is overriden in MobilizedBody::Translation but is not declared virtual; if a Translation class is then created with new and its pointer stored in a MobilizedBody* pointer, the call to getMobilizerAcceleration ends up executing MobilizedBody::getMobilizerAcceleration() and not Translation::getMobilizerAcceleration().

That is:

MobilizedBody* mob=new MobilizedBody::Translation();
mob->getMobilizerAcceleration(); // calls MobilizedBody::getMobilizerAcceleration

If the method is supposed to be overriden in subclasses it should be declared as virtual; or, if the Translation::getMobilizerAcceleration method is generic enough it should be moved to MobilizedBody.

@sherm1
Copy link
Member

sherm1 commented Jan 8, 2018

None of the MobilizedBody interface methods are supposed to be virtual. The virtual methods are located in the MobilizedBodyImpl class and its derived classes. There are two problems here:

  1. The MobilizedBody::getMobilizerAcceleration() method is not implemented (it does say that clearly in its documentation but admittedly that's not very helpful!), and
  2. The MobilizedBody::Translation class (like all the other joints) has its own more-specific API. In this case that joint implements a method it calls getMobilizerAcceleration() which uses the same name as the unimplemented MobilizedBody method. It should have been called something more specific like getTranslationAcceleration() so it wouldn't interfere. You can see it isn't actually the same method at all since it has a different return type (Vec3 vs. SpatialVec).

The Translation mobilizer is the only Simbody joint whose API has a method getMobilizerAcceleration(); that is really an API bug and shouldn't be there. All it does is call getUDot(), which is supported by every joint type. Or you can access udots generically with MobilizedBody::getUDotAsVector().

Can you use getUDot() or getUDotAsVector() and forget about using getMobilizerAcceleration() for now? They are not exactly the same thing for some joints because udots are the time derivatives of a mobilizer's velocity coordinates u. For a translation joint the u's are the individual components of the translation vector from the mobilizer's F origin on the parent body to the M origin on the child (that is, V_FM) so the time derivative udot=A_FM, the acceleration of M's origin in F, which is what MobilizedBody::getMobilizerAcceleration() will return when it is implemented. But the mapping from udot to A_FM can be much more complicated for some joints which is presumably why the general getMobilizerAcceleration() method hasn't been implemented yet.

Sherm

@sherm1 sherm1 changed the title MobilizedBody::getMobilizerAcceleration() should be virtual MobilizedBody::Translation has confusing getMobilizerAcceleration() method Jan 8, 2018
@sherm1
Copy link
Member

sherm1 commented Jan 8, 2018

I retitled this issue "MobilizedBody::Translation has confusing getMobilizerAcceleration() method"

sherm1 added a commit that referenced this issue Jan 8, 2018
Re-enables the deprecation warning C4996 in Visual Studio.
Fixes #604. Fixes #607.
sherm1 added a commit that referenced this issue Jan 8, 2018
Deprecates some badly-named methods in MobilizedBody::Translation.
Re-enables the deprecation warning C4996 in Visual Studio.
Fixes #604. Fixes #607.
@sherm1
Copy link
Member

sherm1 commented Jan 8, 2018

Andrea -- I addressed this by deprecating the badly-named methods in MobilizedBody::Translation and replacing them with names that don't conflict with the base class. The problem still exists that the base class method MobilizedBody::getMobilizerAcceleration() is unimplemented, but hopefully this will reduce future confusion.

Sorry for the trouble, and thanks for reporting it.

@apalazzi
Copy link
Author

apalazzi commented Jan 8, 2018

Hi,

thank you for the support and reactivity, and I'm glad I can somehow contribute to this project :)

@sherm1
Copy link
Member

sherm1 commented Jan 8, 2018

@apalazzi, your contributions as a user are much appreciated! FYI, we also welcome code and documentation contributions from the community in case you don't like waiting around for fixes to get done. Please see CONTRIBUTING.md for more info.

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

No branches or pull requests

2 participants