Add setter/getter for Pose's each element - Issue 35

[pxalcantara]

Solved #35

Add methods to Pose3.hh to get X,Y,Z,Roll,Pitch and Yaw directly, as suggested in the issue description :

• pose.Pos.X() -> pose.X()
• pose.Pos.Y() -> pose.Y()
• pose.Pos.Z() -> pose.Z()
• pose.Rot.Roll() -> pose.Roll()
• pose.Rot.Pitch() -> pose.Pitch()
• pose.Rot.Yaw() -> pose.Yaw()

[pxalcantara]

@chapulina Here we go! first PR!! \o/

[chapulina]

Thanks for the PR! ✌️ It's looking good. My only suggestion would be to also add tests for the mutable functions. I think the current tests are only covering the const ones.

Here's an example:

https://github.com/ignitionrobotics/ign-math/blob/fdbd226d70885e85e265d7c61cfa9014bee1a33a/src/Color_TEST.cc#L187-L194

[scpeters]

are you able to merge this branch with ign-math6? it is currently out of date with the base branch

[pxalcantara]

[DO NOT MERGE] @chapulina @scpeters
This code is not finished. The approach used to implement the mutable X(), Y() and Z() can not be used in Roll(), Pitch() and Yaw(). When this approach is used I got these errors:

That is expected because the Quaternion.Roll() , Pitch() and Yaw() are const : https://github.com/ignitionrobotics/ign-math/blob/ca49e983bc3939b6626e78f036ff5ea9bf10d270/include/ignition/math/Quaternion.hh#L423

How should I solve this point? I thought in some ways but, all of them I need to edit the library Quaternion.hh and, also, there is pattern and architecture questions, so I decided to ask

[chapulina]

humm nice catch

In fact, this reminds me that we've been deprecating the mutable accessors in favor of Set* functions, see the Migration guide and issue #101 .

So I think the solution is to substitute the mutable functions with Set versions.

[pxalcantara]

Humm, nice, so, maybe, stop working with this issue and help with this functionality of the Quaternion.hh ? After that continue to working here?

The idea is, also, to use this code style to Pose.hh and Vector3.hh ?

[chapulina]

Oh I don't think you need to address the entire issue. I'd just change the new functions you're adding. Remove all the mutable functions and add:

Pose3d::SetX
Pose3d::SetY
Pose3d::SetZ
Pose3d::SetRoll
Pose3d::SetPitch
Pose3d::SetYaw

[pxalcantara]

Ok, good, should I remove Pos() and Rot() mutable, as well, and change for SetPos() and SetRot(), or is it better to do this in another PR?

[chapulina]

should I remove Pos() and Rot() mutable, as well, and change for SetPos() and SetRot()

Good call, yes, please!

[scpeters]

Ok, good, should I remove Pos() and Rot() mutable, as well, and change for SetPos() and SetRot(), or is it better to do this in another PR?

those API's are already released in ign-math6, so I wouldn't remove them in this PR

[chapulina]

those API's are already released in ign-math6, so I wouldn't remove them in this PR

Ah sorry, thank you Steve. Let's only remove the ones being added by this PR.

[scpeters]

This code is not finished. The approach used to implement the mutable X(), Y() and Z() can not be used in Roll(), Pitch() and Yaw().

Quaternion doesn't allow modification of Euler angles independently; it only allows them to all be set at once. I would suggest simply adding Get functions for roll, pitch, yaw in this PR and address setting them in a separate pull request

[pxalcantara]

This code is not finished. The approach used to implement the mutable X(), Y() and Z() can not be used in Roll(), Pitch() and Yaw().

Quaternion doesn't allow modification of Euler angles independently; it only allows them to all be set at once. I would suggest simply adding Get functions for roll, pitch, yaw in this PR and address setting them in a separate pull request

Ok, It was I thought, and also using the Euler method would not be a good idea now since it is deprecated, so I'll do your suggestion!
Thanks @scpeters and @chapulina

[pxalcantara]

@scpeters @chapulina sorry for the delay. Is this the way that you suggested? About the failure of the CI test, I couldn't understand the error, what should I do to fix it?

[scpeters]

it looks like the build machines had an error; I'll start the jobs again

[scpeters]

@osrf-jenkins run tests please

[pxalcantara]

Thank you @scpeters!

[chapulina]

Looks great, thanks for iterating, @pxalcantara !