Add functions from Vector3 to Vector2 and Vector4

[osrf-migration]

Original report (archived issue) by Louise Poubel (Bitbucket: chapulina, GitHub: chapulina).

---

For example, there is a Vector3::Normalized, but no such function for Vector2 or Vector4. The same goes for Round, Rounded and other functions.

IssueForNewDevelopers

[pxalcantara]

Hi @chapulina, I'd like to work on this issue. My plan is to work in the Vector2 first, and later, make the changes to Vector4. I have some questions:

1. Which branch should I use as a reference, ign-math6?
2. In the issue description, you mentioned some functions that there is in Vector3 but there isn't in Vector2, for instance, and mentioned "other functions". Comparing both classes I found these functions that are present only in Vector3:

• Normalized
• Round
• Rounded
• Sum
• AbsDot
• Abs
• Perpendicular
• Normal
• DistToLine
• Max(const Vector3 &_v)
• Min(const Vector3 &_v)
• Max()
• Min()
• Correct

Edit: Remove Cross

Your idea is to add all of these functions?

Thank you!

[chapulina]

Thanks for tackling this!

Which branch should I use as a reference, ign-math6?

That's good, since it's the one used from Blueprint. Then we will merge it forward to master.

Your ideia is to add all of this functions?

Maybe some of them don't make much sense, such as Vector4::DistToLine. But for all the functions that have a meaning in 2 / 4 dimensions, yes, I think we should offer a consistent API across all vector classes.

My plan is to work in the Vector2 first, and later, make the changes to Vector4.

That sounds like a good plan. You definitely don't need to do all functions at once, feel free to group them in separate pull requests as you see fit. Also, don't feel like you need to address this whole issue - it's already a big help if you implement a subset of the functions.

[luccosta]

Hi @chapulina, I would like to work in this issue too.

In your experience, what is the best way to divide work like that? I was thinking in take the Vector4 functions while @pxalcantara is at Vector2 functions, but I'm worried that the functions are similar and we end up doing rework.

Like he did with Vector2, I compared Vector3 and Vector4 finding these functions that are present only in Vector3:

• Abs
• AbsDot
• Correct
• Distance (T_x, T_y, T_z, T_w)
• Dot
• Max (const Vector3< T > &_v)
• Max ()
• Min (const Vector3< T > &_v)
• Min ()
• Normalized
• operator<
• Round
• Rounded
• Sum

[pxalcantara]

@luccosta I think we can do this way because the functions need to be added in both classes anyway, so I don't think that is reworked, let's do this way ;)
Maybe don't add DistToLine, as she said, don't make much sense for Vector4

[pxalcantara]

@chapulina Thank you

How about the size of each PR, one for each function is too small, or is ok? Since will have the function implementation and the Unit test for this function?

[pxalcantara]

@chapulina, here it goes the first small PR, how about the size, too small? The link of the contributing.md is off so I have no reference. To continue the implementations I have some questions.
The implementation of Vector2.Normalize() is different from the Vector3.Normalize():

1. The implementation of the Vector2.hh should be left like this?
2. Considering other functions like Round(), Cross() , should I keep the same pattern of implementation?

Thanks!

[chapulina]

here it goes the first small PR, how about the size, too small?

Thanks! I think that size works fine for us, it's quicker to review. It's usually a balance of how inconvenient it is for you to keep a bunch of parallel branches and how easy it is for us to review the PRs. In general, it makes sense to group similar functions, so you could put all the Min / Max functions together for example.

The implementation of Vector2.Normalize() is different from the Vector3.Normalize():

Good point, I think that Vector2::Normalize should use T instead of double for d.

Considering other functions like Round(), Cross() , should I keep the same pattern of implementation?

I think Round makes sense to implement the same way. Cross is not well defined in 2D though, so I think we can skip this one for now until we have a need for it. How does that sound?

[pxalcantara]

Ok, It sounds good but, what I meat about the implementation of Normalize() is that, in Vector3 it return a copy of the object and in Vector3 it is void

So in the next implementation like "Set", as Round() should I return a copy or use void ?

[chapulina]

what I meat about the implementation of Normalize() is that, in Vector3 it return a copy of the object and in Vector3 it is void

Oh yes, I looked too quickly and didn't notice that. There's an issue for that, #34 . I think it can be considered out of the scope for this issue and you can follow up there if you're interested.

Round() should I return a copy or use void ?

Ok, I see the issue here now too. I vote for the new Round to change the value in place, and Rounded to return a copy. What do you think, @scpeters ?

[pxalcantara]

Oh yes, I looked too quickly and didn't notice that. There's an issue for that, #34 . I think it can be considered out of the scope for this issue and you can follow up there if you're interested.

Humm, nice! I agree that it is out of the scope for this issue!
If I would like to tackle that and, once it should be ported backward, I should do it to master right ?

Ok, I see the issue here now too. I vote for the new Round to change the value in place, and Rounded to return a copy. What do you think, @scpeters ?

I like the proposal of Round change the value in place and Rounded return a copy.

[pxalcantara]

Now I'm gonna work in the functions:

- Max(const Vector3 &_v)
- Min(const Vector3 &_v)
- Max()
- Min()

[scpeters]

Ok, I see the issue here now too. I vote for the new Round to change the value in place, and Rounded to return a copy. What do you think, @scpeters ?

I agree; that sounds good

[luccosta]

Now I'm gonna work in

- Abs()
- Dot(const Vector3 &_v)
- AbsDot(const Vector3 &_v)

[ximenesfel]

Hi @chapulina, I would like to work in this issue too.

I taked with @pxalcantara and I will implement the following functions:

• AbsDot
• Abs
• Correct

[luccosta]

In research I found that Cross product only exists in 3 or 7 dimensional vectors, and Perpendicular has a different meaning in 4 dimensions, so I was thinking in remove those.

[luccosta]

The Vector3::operator< implementation dont make much sense for me.

This type of comparison wouldn't match better a length comparison?

As implement in other way breaks the initial objective of the Issue to getting a consistent API across vector classes, I will implement equally for now.

[pxalcantara]

In research I found that Cross product only exists in 3 or 7 dimensional vectors, and Perpendicular has a different meaning in 4 dimensions, so I was thinking in remove those.

As Cross only make sense for 3 dimensions, I'll remove from the list of functions to be added to Vector2.

[akshatpandya]

I see this issue is still open. I'd also like to contribute to this issue. @pxalcantara @luccosta @chapulina I'd like some guidance as to which functions do I start working on?

[pxalcantara]

Hi @akshatpandya Really nice. I the beginning of the post there is a list of functions that should be implemented from Vector3 to Vector4 class. I think you can start for anyone that is not implemented yet and you feel comfortable with it. If you'd like to work on Round() or Rounded(), just take a look at the conversation that starts here about the implementation method.

[akshatpandya]

Thanks for the reply, @pxalcantara . I'll start with the list above to implement from Vector3 to Vector4 class.
From the list above, it seems, Normal(), Cross(), Perpendicular() and DistToLine() are remaining.
Cross() and Perpendicular() makes little sense in terms of Vector4 but I think Normal() and be implemented (have a look here)

What do you think? Just like the Normal() in Vector3 that gives normal to 3 Vector3 points, we can give normal to 5 Vector4 points (pentagon).

[pxalcantara]

@akshatpandya Apparently the functions are already implemented in Vector4, here, so, would be better to work on Vector2 list

[akshatpandya]

@pxalcantara Cool, I start with Round() and Rounded() for Vector2.

[pxalcantara]

@akshatpandya Saturday will happen this event to collaborate together with the Ignition. It'll be really nice to see you there 👍

[pxalcantara]

I think Round makes sense to implement the same way. Cross is not well defined in 2D though, so I think we can skip this one for now until we have a need for it. How does that sound?

@chapulina Since the implementation of the methods Perpendiculer, Normal, and DistToLine needs the implementation of the Cross method and It's not well defined in Vector2. How should we do it? Implement in another way or leave also this method without implementation for now?

[chapulina]

Implement in another way or leave also this method without implementation for now?

Distance to line

I think this can be implemented in a way that's specific to 2D. Here are some hints for distance to line (which in our case is a segment defined by 2 vectors). Special care is needed when the point's projection onto the segment is outside of it.

Normal

I think this can also be implemented specifically for 2D. There are some leads here for how to do it. I think we should be sure to normalize the result. Special care needs to be taken with the direction of the normal. For a triangle it's well defined by the right-hand rule.

Perpendicular

Our documentation for Perpendicular is very vague. It returns "an orthogonal vector", which includes an infinity of correct answers. The test is only checking the unit vectors against one possible solution. So I think that we should hold on porting this to Vector2 for now and revisit whether the Vector3 function is actually useful.

https://github.com/ignitionrobotics/ign-math/blob/b5306cc9ea8e7ac2075547b227eb5e6c738efb3e/src/Vector3_TEST.cc#L194-L206

[akshatpandya]

@chapulina I understand that "Distance to Line" can be implemented for Vector2.

For Normal:

Our convention is to return a vector normal to a triangle. Which means, a vector perpendicular to a plane. Such a vector has to be defined by Vector3.

For Perpendicular:

Currently we are returning an orthogonal vector. For Vector2, this can be implemented (from here.
There will be 2 unique unit vectors perpendicular to a line/Vector2 (which can be scaled to give infinite perpendiculars)

So I think we can currently implement, distance to line and perpendicular. And think of what normal would mean for Vector2.

[chapulina]

Our convention is to return a vector normal to a triangle. Which means, a vector perpendicular to a plane. Such a vector has to be defined by Vector3.

Yeah I think the equivalent in 2D would be a unit 2D vector that defines the perpendicular to a line segment defined by 2 points.

There will be 2 unique unit vectors perpendicular to a line/Vector2 (which can be scaled to give infinite perpendiculars)

If we're talking about unit vectors, then I think a normal is probably better defined, like I said above.

Currently we are returning an orthogonal vector.

The problem with the implementation for Vector3 is that it returns just one of infinite possibilities. For example on the first test case above, the perpendicular for (1, 1, 0) is given as (0, 0, -1). Why not (-1, 1, 0) or (-1, 1, 1)? It's true, it's "an" orthogonal vector. I'm not sure how that function is being used.