Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convex_hull_3: Make it minimal. #3226

Merged
merged 5 commits into from Jul 18, 2018

Conversation

maxGimeno
Copy link
Contributor

Summary of Changes

This PR adds some initialization steps to the convex_hull_3 function so it returns a minimal hull.
For this, it also adds a need for Has_on_negative_side_3 to the ConvexHullTraits_3 Concept.

Release Management

@lrineau lrineau added Not yet approved The feature or pull-request has not yet been approved. Small feature labels Jul 12, 2018
`bool operator()(Plane_3 h, Point_3 q)`, which determines if the point
`q` is on the negative side of the halfspace `h`.
*/
typedef unspecified_type Has_on_negative_side_3;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now the traits requires both Has_on_positive_side_3 and Has_on_negative_side_3. I suggest that both are replaced by Oriented_side_3 that provides:

Oriented_side operator() (const Kernel::Plane_3 &h, const Kernel::Point_3 &p)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sloriot, do you agree ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I'm working on it

@lrineau lrineau added this to the 4.14-beta milestone Jul 12, 2018
change plane's orientation instead


//take extreme points to begin with.
internal::Convex_hull_::init_iterators(minx, maxx, miny, it, points);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compilation error in Travis:
https://travis-ci.org/CGAL/cgal/jobs/403155410#L1983

The minimal testing after such a change is running the test suite of Convex_hull_3!

@sloriot sloriot force-pushed the Convex_hull_3-Make_minimal_hull-GF branch from 9028a2c to 0037edc Compare July 13, 2018 14:56
@sloriot
Copy link
Member

sloriot commented Jul 16, 2018

I think the patch is correct.

I focus only on the case when the convex hull is 3D as otherwise the 2D algorithm is used. The algorithm works as follow: 4 points with a high chance to be on the convex hull are first selected and an initial convex hull is built using these 4 points (these points are not coplanar). Then, iterating over the 4 faces of the convex hull, we select points that are on the positive side of the supporting plane of each face and assign those points to the face. The set of faces with points assigned are collected in the set of so-called pending faces. Then the algorithm simply picks one pending face from the set while it is not empty. For each picked face, it looks for the point assigned to the face that is the furthest from the supporting plane of the face. Then this point is used in find_visible_set() to look for all the faces visible (on or on the positive side) from the further point, by diffusion starting from the current face. Visible faces are removed from the set of pending faces and new faces are created by staring the hole of faces removed using the furthest point. All the points assigned to the faces removed must then be assigned to the new faces like at initialization, and potentially new pending faces are added to the set.

The reason why only find_visible_set() needs to be modified (by changing has_on_positive_side() to !has_on_negative_side()) is that it is in this function that a face is removed from the current convex hull. The has_on_positive_side() test used to assigned a point to a current convex hull face does not need (and must not use) the boundary condition because if a point that is on the convex hull is coplanar with a current face, then it has to be on the positive side of another face (otherwise it is not on the convex hull). Then the current face, it is coplanar with will be invalidated at a future call to find_visible_set() using the non-strict predicate version.

@sloriot sloriot added Bug Ready to be tested Pkg::Convex_hull_3 and removed Not yet approved The feature or pull-request has not yet been approved. Small feature labels Jul 16, 2018
@sloriot sloriot modified the milestones: 4.14-beta, 4.13-beta Jul 16, 2018
@sloriot
Copy link
Member

sloriot commented Jul 16, 2018

By reworking the order of the points when creating the plane, I could get rid of the has_on_negative() so no change in the traits is required.

@lrineau lrineau self-assigned this Jul 16, 2018
@lrineau
Copy link
Member

lrineau commented Jul 16, 2018

This PR conflicts with #3116. Which one should be tested and integrated first?

@sloriot
Copy link
Member

sloriot commented Jul 17, 2018

They are independent, but this one being smaller it is easier to start by this one.

@lrineau lrineau merged commit e885074 into CGAL:master Jul 18, 2018
@lrineau lrineau deleted the Convex_hull_3-Make_minimal_hull-GF branch July 18, 2018 16:48
@lrineau
Copy link
Member

lrineau commented Jul 18, 2018

Successfully tested in CGAL-4.13-Ic-66, and then merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convex_hull_3 is not minimal
3 participants