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

Remove semantics check classes and related methods #704

Merged
merged 2 commits into from
Jun 26, 2020
Merged

Conversation

traversaro
Copy link
Member

These classes were deprecated in iDynTree 1.0, see #622 for more details.

Sorry for the huge PR, but as all the different classes were related to one another, it was difficult to split the PR in smaller chunks.
The relevant commit is d293786, while 997237b is the commit full of autogenerated MATLAB bindings files.

As the modification are quite extensive, I am afraid reviewers will need to trust a bit me on this. However, a quick general sanity check would be useful.

These classes were deprecated in iDynTree 1.0, see
#622 for more details.
Copy link
Member

@diegoferigo diegoferigo left a comment

Choose a reason for hiding this comment

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

This is a huge work, thanks @traversaro. I agree that this was somehow needed. We never managed to exploit this code that much. Personally, even after few years spent around iDynTree's code, I am still struggling to connect the dots in the middle of the semantic sections.

I agree that this simplifies a potential support of AD, and currently is perhaps a better aim that could become really useful to many.

What I didn't get exactly from this PR is all the class names changes. Can you please super briefly recap these modification? It could be useful as implicit documentation.

  • I see a concrete GeomVector3. How does it differ from *Vector*?
  • It is not very clear to me why the set|getMeasurement have been removed. I'm not familiar with that code and I could be missing a step.

@traversaro
Copy link
Member Author

traversaro commented Jun 26, 2020

I see a concrete GeomVector3. How does it differ from Vector?

Basically, all the semantics aware such as LinearMotionVector3, LinearForceVector3, AngularMotionVector3 and AngularForceVector3 had a lot of concrete methods for operation that are allowed on linear and angular parts of 6D velocity and force vectors, that only permitted physically meaningful operations, so it was only possible to compute the dot product between a LinearMotionVector3 and LinearForceVector3, while it was not possible to compute the dot product between a LinearMotionVector3 and AngularMotionVector3. Other examples of similar "semantics" methods where cross , operator+ , operator- . While those methods were not really used outside of iDynTree, they were quite used inside iDynTree, so to reduce the amount of change I created a new GeomVector3 that inherits from Vector3 and add all these methods typical of 3D vectors. All LinearMotionVector3, LinearForceVector3, AngularMotionVector3 and AngularForceVector3 are now typedef of GeomVector3, so to avoid API breaks (actually now the API permits operation that were not legal before).

It is not very clear to me why the set|getMeasurement have been removed. I'm not familiar with that code and I could be missing a step.

It is basically a change due to the fact that now AngularVelocity and LinearAcceleration are now the same type (they are both typedef of GeomVector3) and so they can't be used to overload the method, so I deleted them because the Vector3 set|getMeasurement provide the same functionality, or actually even more, as now it is possible to read the data from a Gyroscope with a LinearAcceleration object (that is actually just a GeomVector3).

@diegoferigo
Copy link
Member

Great! Thanks for the explanation, now it's clear :)

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