Vector3::Normalize returns Vector3, Vector[24]::Normalize return void

[osrf-migration]

Original report (archived issue) by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

---

We should make these consistent.

[osrf-migration]

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).

---

Actually, there is a bit of different behavior that can be expected.

For example, we added Transpose and Transposed methods to the matrix classes in pull request #74:

public: void Matrix3::Transpose()
public: Matrix3<T> Matrix3::Transposed() const

The Transpose method acts in place on an existing object, while Transposed is const, so it doesn't affect the base object, but returns a new value.

I think we could do a similar thing for Normalize with the vector classes and perhaps Quaternion as well:

• Normalize for in-place modification of an object
• Normalized for creating a new object

I realize this doesn't directly address the title of this issue, which is about return values, but changing that will break API/ABI, so we can wait until the next major release to change the return type if we decide to do so.

[osrf-migration]

Original comment by Amitoj (Bitbucket: amtj).

---

I tried the bug, please have a look.
https://bitbucket.org/amtj/ign-math/commits/all

Regards.

[osrf-migration]

Original comment by Louise Poubel (Bitbucket: chapulina, GitHub: chapulina).

---

Here's the complete diff between your branch and this one.

I think it looks like a good start. We should probably also add Vector3::Normalized. And there should be tests for all new functions, see this for example.

[pxalcantara]

I think we could do a similar thing for Normalize with the vector classes and perhaps Quaternion as well:

I think this bug is fixed in the Vector class, Vector3, Vector2 and, Vector4. About the Quaternion to solve this issue, we should add Normalized to the class?

[chapulina]

I think this is still inconsistent:

https://github.com/ignitionrobotics/ign-math/blob/b5306cc9ea8e7ac2075547b227eb5e6c738efb3e/include/ignition/math/Vector3.hh#L133

https://github.com/ignitionrobotics/ign-math/blob/b5306cc9ea8e7ac2075547b227eb5e6c738efb3e/include/ignition/math/Vector2.hh#L102

https://github.com/ignitionrobotics/ign-math/blob/b5306cc9ea8e7ac2075547b227eb5e6c738efb3e/include/ignition/math/Vector4.hh#L149

I agree with the solution proposed by @scpeters above, the problem is that it will be too disruptive. We can't tick-tock deprecate Vector3::Normalize(), we would have to change the return type from Vector3 to void on ign-math7. I think we need to balance the API improvement with the impact on users. Maybe since a lot will change in ign-math7 due to #101, maybe it's a good time to also make this change? What do you think, @scpeters ?

About the Quaternion to solve this issue, we should add Normalized to the class?

Nice catch, it already has void Normalize, so we could add Quaternion<T> Normalized.

https://github.com/ignitionrobotics/ign-math/blob/b5306cc9ea8e7ac2075547b227eb5e6c738efb3e/include/ignition/math/Quaternion.hh#L280-L281

[pxalcantara]

Nice catch, it already has void Normalize, so we could add Quaternion Normalized.

So, I'm planning to work on the implementation of the Quaternion<T> Normalized method.