Skip to content

Add a precondition checking for degeneracies before calling intersection #4654

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sloriot
Copy link
Member

@sloriot sloriot commented Apr 15, 2020

Provided the fact that I have to update some tests, maybe we do not want that (or at least turn the precondition into a warning).

@sloriot
Copy link
Member Author

sloriot commented Apr 15, 2020

TODO: I did not run yet the tests for do_intersect

@sloriot
Copy link
Member Author

sloriot commented Oct 6, 2020

@CGAL/geometryfactory I'd be happy to get some feedback on this PR

@lrineau
Copy link
Member

lrineau commented Oct 6, 2020

I find that strange that Do_intersect_3 or Intersect_3 would require non-generated inputs. CGAL is supposed to deal with degenerated inputs, and be robust. I would prefer that the intersection code deal with the degenerated cases.

@sloriot
Copy link
Member Author

sloriot commented Oct 6, 2020

Note that the initial motivation was to not penalize users that have clean data. Everywhere in CGAL, we expect the input to be clean. We could of course have more generic versions.

@lrineau
Copy link
Member

lrineau commented Oct 6, 2020

Note that the initial motivation was to not penalize users that have clean data. Everywhere in CGAL, we expect the input to be clean. We could of course have more generic versions.

That is not true. Historically, CGAL algorithms were designed and coded to be robust to degenerated cases. See Triangulation_2, Triangulation_3, Nef, were the input can be very degenerated. A 3D Delaunay triangulation can even deal with a set of aligned points.

Only recently in CGAL, we have introduced new algorithms that are more constructions-based, and not predicates-based, and less tolerant to degenerated cases: CDT with intersections, Mesh_2/Mesh_3, Straight Skeleton, most of PMP...

I think the kernel should be robust. Maybe we can introduce new functors to deal with non-degenerated cases. But how far? Should the non-degenerated intersection code deal with coplanar triangles and segments?

@sloriot
Copy link
Member Author

sloriot commented Oct 6, 2020

You are mixing things up. Generic positions and degenerate input are two different things.

@maxGimeno
Copy link
Contributor

@lrineau @sloriot When do you think this PR will be ready for testing ?

@lrineau
Copy link
Member

lrineau commented Oct 16, 2020

I think we want both:

  • for general purpose, our functors should deal with degenerate objects when that it possible (a degenerated Plane_2/Plane_3 is completely unusable), as they did before,
  • but I understand that, for performance reason, we would like other "instances" of kernel functors that can assume non-degenerated objects.

@lrineau
Copy link
Member

lrineau commented Oct 16, 2020

@lrineau @sloriot When do you think this PR will be ready for testing ?

We are still discussing.

@sloriot
Copy link
Member Author

sloriot commented Oct 16, 2020

I'm pretty sure I can easily do a mechanism handling the forwarding of degenerate inputs automatically, but the hardest part is to find a name for the function that supports degenerate input. Alternatively, we could have a global switch enabling degenerate inputs for do_intersect, I guess the initial if should be negligible compared to the tests to be done after.

@lrineau
Copy link
Member

lrineau commented Oct 16, 2020

Should not K::Intersect_[23] and K::Do_intersect_[23] deal, by default, with degenerated objects?

Actually, for backward compatibility, they should behave exactly as they do in CGAL-5.1. I do not know how that is documented.

Then we should invent other names for the variant that behaves differently.

At the level of the global functions, it could be dispatched using a tag. Actually, if our kernels were like NewKernel_d, it could also be a special tag at the level of kernel functors. Is not it, @mglisse?

@mglisse
Copy link
Member

mglisse commented Oct 16, 2020

Actually, if our kernels were like NewKernel_d, it could also be a special tag at the level of kernel functors. Is not it, @mglisse?

Functor<K, Intersection_tag, No_degeneracy_tag> or Functor<K, Intersection_without_degeneracy_tag>? I created the first one thinking I might use it as Functor<K, Orientation_tag, Inexact_tag> for structural filtering in Triangulation, but I never reached that point.
(NewKernel_d is missing Intersection_d)

@maxGimeno
Copy link
Contributor

ping

@sloriot sloriot modified the milestones: 5.2-beta, 5.3-beta Oct 26, 2020
@maxGimeno maxGimeno removed this from the 5.3-beta milestone Mar 3, 2021
@maxGimeno maxGimeno added the rm: not for next release Indicate to the release team that a PR should not be merged before the next release branch is forked label Mar 3, 2021
@sloriot sloriot added this to the 5.4-beta milestone Mar 3, 2021
@sloriot sloriot mentioned this pull request Mar 3, 2021
2 tasks
@MaelRL MaelRL assigned MaelRL and unassigned MaelRL May 14, 2021
@MaelRL MaelRL added the Not yet approved The feature or pull-request has not yet been approved. label May 14, 2021
@lrineau lrineau removed the rm: not for next release Indicate to the release team that a PR should not be merged before the next release branch is forked label Jul 7, 2021
@sloriot sloriot modified the milestones: 5.4-beta, 5.5-beta1 Sep 23, 2021
@MaelRL MaelRL modified the milestones: 5.5-beta, 5.6-beta Mar 31, 2022
@MaelRL MaelRL modified the milestones: 5.6-beta, 5.7-beta Mar 23, 2023
@janetournois janetournois modified the milestones: 6.0-beta, 6.1-beta May 16, 2024
@MaelRL MaelRL modified the milestones: 6.1-beta, 6.2-beta Mar 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Conflicts Enhancement Not yet approved The feature or pull-request has not yet been approved. Pkg::Kernel_23 Under discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants