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

Deprecate semantics support in core data structures #622

Merged
merged 3 commits into from
Jan 9, 2020

Conversation

traversaro
Copy link
Member

The idea of optionally embed semantics information in the core data structures of iDynTree (in which frame a position is expressed, w.r.t. to which frame the derivative of a velocities is taken) was something we worked back in 2015 with @nunoguedelha, see the "Semantics check" section in #29 .

While being an interesting feature that address a clear user need, it was never effectivly used for the following reasons:

  • A lot of computations in estimators/controllers does not use iDynTree classes, but rather Matlab code or semantics-less C++ libraries such as Eigen .
  • The semantics was only provided for position and velocities, and we then realized that it would be non-trivial to properly extend them to forces, inertia and accelerations that are the quantities tipically used in Dynamics.

While the presence of this code is mostly harmless at the moment, it drastically complicates the readability of the library both for new users of iDynTree interesting in contributing functionalities, and also for experienced users that may want to refactor the core of iDynTree to add new functionalities such as auto-diff. For this reason, as discussed with @nunoguedelha even if it makes me sad the best choice is to deprecated this code in iDynTree 1.0, and remove it in iDynTree 2.0 .

This PR deprecates all the methods of the classes that end in Semantics, and all the methods that return semantics classes such as getSemantics or setSemantics . The actual code will be removed during the iDynTree 2.0 development.

Furthermore, reiterate that IDYNTREE_USES_KDL is deprecated and is going to be removed.
@traversaro traversaro force-pushed the deprecate/semantics branch from b7dda8b to a1f7ad1 Compare January 9, 2020 10:42
Copy link
Collaborator

@francesco-romano francesco-romano left a comment

Choose a reason for hiding this comment

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

Why not deprecating the whole class instead of every method?

@traversaro
Copy link
Member Author

Why not deprecating the whole class instead of every method?

The main problem was that some non-deprecated class inherit from the class that we want to deprecated, and I was getting deprecation warnings for each compilation unit that included this class that inherited from the deprecated class (as "inheriting" is a use), creating a lot of false positives. Deprecating just the methods seemed to be a good trade-off to me.

@traversaro traversaro merged commit 9e58051 into devel Jan 9, 2020
@traversaro traversaro deleted the deprecate/semantics branch January 9, 2020 15:52
traversaro added a commit that referenced this pull request Jun 25, 2020
These classes were deprecated in iDynTree 1.0, see
#622 for more details.
traversaro added a commit that referenced this pull request Jun 25, 2020
These classes were deprecated in iDynTree 1.0, see
#622 for more details.
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