Fix the ear clipping

[smallchimney]

Ear clipping method get sometimes wrong result on some concave polygon.

The original isEar() method only check whether there is any point in the triangle. It will get wrong result if the triangle don't have inside point but is a concave point of the polygon. Check the triangle each time in isEar() will fix this bug.

[smallchimney]

All vertices in the gif above are on the plane, when move vertex 1 above the line of v2-v6, the triangle v2-v6-v1 will be regard as a ear.

[SergioRAgostinho]

@smallchimney Thank you. Can you post the same gif animation, with your fix implemented?

[SergioRAgostinho]

@taketwo what's our policy on API changes of non-virtual protected methods? Purist side: still breaks the API since our consumers might have derived classes from this and use this method. Pragmatic side: unlikely.

[taketwo]

I think generallry we can approach "protected" API stability in a relaxed way, i.e. accept breakage without deprecation cycle. That said, isn't it super easy to keep area() method around and thus definitely not break anyone's code?

[SergioRAgostinho]

I haven't checked yet. I decided to read a little on ear clipping triangulation before having a look with some proper eyes to the PR. I was failing to get the jargon of the OP. ^^

[SergioRAgostinho]

The code is not used at all, so there's no reason to keep it around. I would remove it.

[smallchimney]

here is the fix implemented effect

[SergioRAgostinho]

OK! After a more careful review I've reached an opinion on what to preserve and what not to preserve :)

• From your first commit I'm ok in keeping it all, plus the small fix you added on the second commit.
• From the second commit I'm only interested in the removal of the area method and the changes to the unit test.

I'm explicitly keeping out the triangulate refactoring and all those changes from push_back to emplace_back in the cases where an actual copy is taking place.

Ideally, your fix and the changes to unit test should all in the first commit, the removal of the area method, on a separate commit.

[SergioRAgostinho]

The code is not used at all, so there's no reason to keep it around. I would remove it.

[smallchimney]

I'm sorry for I didn't have a clear explanation in my commit log and code annotation, because my English is not very good.
As far as I know at present，there are two preconditions for Ear-Clipping to segment correctly：

1. The vertices should be input in right order;
2. No edge should cross by another edge;

To explain the first point, let's see the simple polygon:
4--3
| .. |
1--2
This is a simple polygon, from vertex 1 to vertex 4, we can say this polygon is anti-clockwise. In this case, the Ear-Clipping will failed to segment, so the original code use area return an signed dimension. If the dimension return negative, the polygon input will be regard as in wrong order, and will reverse the vertices in this case. For the simple polygon above, the vertices will be reverse to 4-3-2-1, and this works fine.
However, this is just one simple polygon, if we set polygon as this:
4--3
| .. |
5..2
| .. |
6--1
And move V1 above the linestrip of V2-V6, the concave triangle 2-6-1 will be regard as an ear, which means a part of the polygon. This is because the original method only check whether there is point in the triangle to judge a ear. And my quick fix is avoid this case, check whether the edge 1-6 to edge 1-2 is in clockwise. There are 360 degree in both side of the two edges, pick the less degrees side as move trajectory. This will return anti-clockwise when V1 is below V2-V6 and skipped in this turn, because it calculate the inner angle; and will return clockwise when V1 is above V2-V6, because it calculate outside angle.
I thought that the order check is in isEar method now, so I remove the area for order check in my first commit, but I'm wrong. It's still necessary to reverse the vertices or the Ear-Cliping will not segment correctly. Still in above polygon with reverse order 6-5-4-3-2-1, all the triangle will be regard as concave triangle because the method only works in anti-clockwise vertices. In this case, the method will go into infinite loops and return with no segment.

from 6-1 to 6-5 is anti-clockwise? true
6-1 crossing 6-5 is 2
check for triangle (1, 6, 5): false
from 5-6 to 5-4 is anti-clockwise? false
5-6 crossing 5-4 is 0
check for triangle (6, 5, 4): false
from 4-5 to 4-3 is anti-clockwise? true
4-5 crossing 4-3 is 2
check for triangle (5, 4, 3): false
from 3-4 to 3-2 is anti-clockwise? true
3-4 crossing 3-2 is 2
check for triangle (4, 3, 2): false
from 2-3 to 2-1 is anti-clockwise? false
2-3 crossing 2-1 is 0
check for triangle (3, 2, 1): false
from 1-2 to 1-6 is anti-clockwise? true
1-2 crossing 1-6 is 2
check for triangle (2, 1, 6): false
from 6-1 to 6-5 is anti-clockwise? true
6-1 crossing 6-5 is 2
check for triangle (1, 6, 5): false
from 5-6 to 5-4 is anti-clockwise? false
5-6 crossing 5-4 is 0
check for triangle (6, 5, 4): false
from 4-5 to 4-3 is anti-clockwise? true
4-5 crossing 4-3 is 2
check for triangle (5, 4, 3): false
from 3-4 to 3-2 is anti-clockwise? true
3-4 crossing 3-2 is 2
check for triangle (4, 3, 2): false
from 2-3 to 2-1 is anti-clockwise? false
2-3 crossing 2-1 is 0
check for triangle (3, 2, 1): false
from 1-2 to 1-6 is anti-clockwise? true
1-2 crossing 1-6 is 2
check for triangle (2, 1, 6): false

What if I reverse the usage of area to make vertices always be 1-2-3-4-5-6 in above case? This looks nice, but actually area return 0 for both order 1-2-3-4-5-6 or 6-5-4-3-2-1. And in more complex polygon, it works worse.
During the following fixing, I found that even with same observation angle, the concept of vertices order is still relative:

The vertex 12-1-2-3 can be seen as in clockwise, and go on, the vertex 5-6-7-8-9-10 can be seen as in anti-clockwise. Is there are some good area() method to make all those complex case return correct sign? I'm not good at this, so I change my mind. There are always two order for one polygon, and one will return correct segment result, another will go into infinite loops. So I just make a try for the vertices input, to check whether it will go into infinite loops.
To be honest, there are no more complex polygons in 3D in my case, I just check the cube segment result for that. And when all points on the same plane, it works fine in all my case

[smallchimney]

1. If we don't refactoring the triangulate, we will still get wrong result when the input is in wrong order;
2. If we don't refactoring the triangulate, I recommend to continue use area for order checking, although it may be failed in some case;
3. Could you please talk about the disadvantage of emplace_back? In my opinion this is just an enhance implement of push_back, and do no harm when replace with push_back.

[stale]

This pull request has been automatically marked as stale because it hasn't had
any activity in the past 60 days. Commenting or adding a new commit to the
pull request will revert this.

Come back whenever you have time. We look forward to your contribution.

[smallchimney]

It seems the code has been merged in jsk-ros-pkg/jsk_recognition#2447, so I will close this PR.

[smallchimney]

I made a mistake, the changes was not merged into PCL