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 deprecation warning #145

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rovo89
Copy link
Contributor

@rovo89 rovo89 commented Aug 18, 2024

Compiling mower_logic showed a warning about that constructor being deprecated. It would either call setEuler() or setRPY(). Testing in the simulator, setRPY(0.0, 0.0, yaw) seems to record the docking orientation fine, while setEuler(yaw, 0.0, 0.0) produced crap. That fits to the fact that I saw various getRPY() calls in the code base. But another pair of eyes would definitely help.

@quelqundev
Copy link

quelqundev commented Aug 19, 2024

Hi @rovo89 👋

The use of setEuler() is done only if TF2_EULER_DEFAULT_ZYX is set.

I searched for this définition in open_mower_ros compilation configuration with no results.

I searched on github "#define TF2_EULER_DEFAULT_ZYX" and i found only 3 repo using this definition.

To me, it confirms that you choose to use the good fonction setRPY().

I would understand that you want someone to check "on the ground"... 😁

@ClemensElflein ClemensElflein added the enhancement New feature or request label Aug 19, 2024
@ClemensElflein
Copy link
Owner

Good point I think that's used in multiple places though, since during compilation it comes up a lot

@rovo89
Copy link
Contributor Author

rovo89 commented Aug 19, 2024

The use of setEuler() is done only if TF2_EULER_DEFAULT_ZYX is set.

Hm, really? It's #ifndef TF2_EULER_DEFAULT_ZYX, so setEuler() is used if not defined:
https://github.com/ros/geometry2/blob/6df9e94363061a24ba890a6ff29771a96b0d756b/tf2/include/tf2/LinearMath/Quaternion.h#L54-L58

That confused me a lot, because I didn't find references to TF2_EULER_DEFAULT_ZYX either... but the reality check worked only for setRPY(). That's why I hope "someone" who is more familiar with tf2 can judge what's the correct function mathematically.

@quelqundev
Copy link

Well i'm sorry to have done this misreading...😅

Yeah then i understand that there is a need to check more in detail. 👌

@rovo89
Copy link
Contributor Author

rovo89 commented Sep 2, 2024

I did some emperical tests, the strongest one being that I modified Quaternion.h and changed the #else branch to call setRPYxxx. It still compiled, while doing the same thing in the #ifndef branch complained about the unknown function. So setEuler it is... but that raises more questions.

The constructor is defined as:

ROS_DEPRECATED Quaternion(const tf2Scalar& yaw, const tf2Scalar& pitch, const tf2Scalar& roll)

And it passes on the arguments to setEuler() in the same order. But the call in OM is with (0.0, 0.0, yaw). Looks mixed up, because whatever is in the yaw variable is actually used as roll. Maybe it's used wrong in all places, especially those which read back the values, and two times wrong gives a right?

@rovo89
Copy link
Contributor Author

rovo89 commented Sep 3, 2024

Finally got confirmation that setRPY(0.0, 0.0, yaw) is the right thing to do:
https://discord.com/channels/958476543846412329/958481126882680902/1280476699661701162

Nevertheless, I'm converting this PR to draft so I can check for other instances of the deprecated call.

@rovo89 rovo89 marked this pull request as draft September 3, 2024 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants