EarClipping algorithm improved + typing error in Clipper3D

[magro11]

Due to a commit-mistake, I mixed two different pull request in this one:

• In clipper3D.h there are two spelling mistakes using Clipper3d instead of Clipper3D as template parameters. This does not compile with MSVC2010.
• EarClipping algorithm failed for polygons, where are vertices have the same x or y component: http://www.pcl-users.org/earClipper-isEar-for-flat-triangles-in-XY-plane-td4028199.html. In the new implementation, the earClipping is calculated in 3D instead of projecting all points to the XY-plane.

Sorry about mixing about two different things, but the first one is really tiny.

[aichim]

Hi Marc,

Could you please add an unit test to show that the old implementation failed and this one now works where the old one worked + the new case?

Thanks!

[magro11]

I try to prepare an unit test for the earClipping algorithm. I also put a spelling correction into this commit by mistake that led to a compiler error with MSVC 2010; I don't think that there is a unit test necessary.

For the unit-test, is it right to add a new source file in \pcl\test\surface and implement the test based on gtest there?

[magro11]

So, now I added a test EarClippingCubeTest to the existing "test_ear_clipping". When creating a unit cube aligned along the x,y and z axes, only the two faces (bottom and top), perpendicular to the xy-plane is recognized by the old implementation. Therefore the test already fails with the old implementation concerning the number of resulting triangles. The unit test already contained one more complex polygon that is split into triangles with the old and new implementation. I hope that helps.

In general, I think that both implementations are not very robust if a polygon does not fully lie in one plane, like it is the case for the first test "EarClipping". Some slight deviations are ok, but if the points are somewhere connected in space it becomes difficult. However, this is a general problem of the ear clipping algorithm and starts with the problem, that the area of such a non-planar polygon is difficult to determine.

[rbrusu]

Why is there a piece of commented code in the commit?
/*bool

•  sameSide (const Eigen::Vector3f &p1, const Eigen::Vector3f &p2, const Eigen::Vector3f &a, const Eigen::Vector3f &b) const...*/
  

[magro11]

Hi Radu,

right now, I commented this part, because there are two possible implementations in the method isInsideTriangle. The first (currently active) implementation is faster however difficult to understand. Nevertheless, this is the common method also used by other rendering algorithms. The second (commented) method, is more straigt-forward and better to understand and uses the commented method sameSide.

It is no problem for me to totally delete the second, slower and currently commented method.

Cheers

[jspricke]

Hi Marc,

please don't mix two things in one pull request but rather use different branches. I will cherry pick the 5b59df8 into master and next, as this seems to be an important fix. The rest needs some more time, I think.

[jspricke]

@magro11 Could you update the pull request according to the comments? Thanks a lot!

[magro11]

Hi Jochen,
with my commit 09882a8 I updated the necessary files from ear_clipping and its corresponding test with the respect to the comments above and I adapted it to comply with PCL 1.7.
Unfortunately (I'm not very familiar with the whole pull request mechanism), I merge my forked repository with the current version of PCL. This leads to the merge commit e177aa0. Is this a problem for integrating the ear_clipping-commits into the main branch of PCL?

[jspricke]

@magro11 please don't include merges in your pull request. Just do a force push to your branch.

[magro11]

So now, I reverted my old stuff and put all my changes concering the earClipping algorithm to the commit f52192a. Hope this helps.
Cheers
Marc

[jspricke]

+1 from me, @aichim, @rbrusu are you fine with merging this? Thanks @magro11 for updating the patch.

[rbrusu]

Looks good to me!

[aichim]

Should be good I guess. If you tested it and it improves the results.