Generalize Kernel and Point Type #2
Conversation
include/PolygonSimplification.hpp
Outdated
| class PolySimp { | ||
| using CGALKernel = Kernel; | ||
| using CGALPoint = Point; | ||
| using CGALPolygon = CGAL::Polygon_2<CGALKernel, std::vector<Point>>; |
There was a problem hiding this comment.
A couple questions:
(a) it doesn't seem like Point should be able to be specified separately from Kernel?
(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.
(a) In an ideal world only Point would be specified and we would derive the Kernel 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.
Why are you trying to use a point type other than Kernel::Point_2?
There was a problem hiding this comment.
But if you really want to just specify Point, you need something like:
namespace detail {
template<class Point>
struct cgal_infer_point_kernel { using type = void; }
template<class Kernel>
struct cgal_infer_point_kernel< Point_2<Kernel> > { using type = Kernel; }
}
template<class Point, class Kernel = typename detail::cgal_infer_point_kernel<Point>::type >
/* ... */There was a problem hiding this comment.
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.
| using Edge = BareType<decltype(*edges.begin())>; | ||
| std::vector<CGALPoint> points; | ||
| // As long as we are using a Bounded_kernel, we don't need to worry that the vertex is actually a ray. | ||
| std::transform(edges.begin(), edges.end(), std::back_inserter(points), [](const Edge &edge){ return edge.vertex()->point(); }); |
There was a problem hiding this comment.
FWIW, I'm pretty sure that the points associated with each vertex will be CGALKernel::Point_2 and not CGALPoint, 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.
@elfprince13 what makes you say that?
There was a problem hiding this comment.
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.
No, I have not, but that certainly sounds de rigueur.
Co-authored-by: Thomas Dickerson <[email protected]>
|
This now works in integration testing with other code, but a no-op test as mentioned by @elfprince13 should be performed (but may not block merging this PR). |
Allows different CGAL kernel and point types to be used than the values previously hard-coded, making this more generally useful. In order to provide templating, converts
PolySimpfrom a namespace to a class, but tries to retain the intended structure as much as possible.