Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Generalize Kernel and Point Type #2
base: master
Are you sure you want to change the base?
Generalize Kernel and Point Type #2
Changes from 2 commits
ade2860
4e25312
7a6e098
bfbc1d2
d3cb770
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple questions:
(a) it doesn't seem like
Point
should be able to be specified separately fromKernel
?(b) are there any restrictions on the types of kernels that can be used? I would be fairly surprised if an inexact kernel could be used with Nef polyhedra....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(a) In an ideal world only
Point
would be specified and we would derive theKernel
from the point, but my reading of https://doc.cgal.org/latest/Kernel_23/classCGAL_1_1Point__2.html doesn't show a way to do this.(b) I'd be surprised as well. The purpose of this PR is really to allow different point types; the different kernels is just a side-effect based on (a).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you trying to use a point type other than
Kernel::Point_2
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if you really want to just specify
Point
, you need something like:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, okay, so, based on our Google Chat discussion, there's no reason to specify point type and kernel type separately. Following up with additional change request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I'm pretty sure that the points associated with each vertex will be
CGALKernel::Point_2
and notCGALPoint
, so this is not going to preserve IDs. If this doesn't work for you, you're going to need to provide a custom kernel that uses your custom points.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elfprince13 what makes you say that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because
CGAL::Nef_polyhedron_2
is templated on the kernel type and not the point type, so the internal data structures have no way of knowing about the ids. In principle, that doesn't preclude an interface based on some kind of type-erasure mechanics, but I know enough about CGAL's zero-cost abstractions design philosophy to tell you that's not what's happening here.Have you actually run tests to confirm if a no-op simplification on an already simple polygon preserves IDs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I have not, but that certainly sounds de rigueur.