CGAL
Add a Bbox class with dimension as parameter
Summary of Changes
For the upcoming Frechet Distance package we need a dD bounding box. We do not have that in Kernel_d, and there is such a class in ign's Distributed Delaunay Triangulation package to come (link). This PR copies it to the Kernel package (with the ok of @mbredif), but in the CGAL namespace.
This draft pull request is to get a discussion started.
- In the initial version even the number type is a template parameter. Do we want to keep that?
- Do we want to make
Bbox_2/3atypedefof this new class? - We have to go through the API and probably enrich it with what
Bbox_2offers additionally. - Do we have to add functions in
Kernel_d?
TODO
- [ ] create a small feature and get it approved
Release Management
- Affected package(s): Kernel_23
- Feature/Small Feature (if any): tbd
- Link to compiled documentation (obligatory for small feature) wrong link name to be changed
- License and copyright ownership: LGPL , owned by IGN
@mglisse I am also wondering why there is no bounding box class in Epick_d etc. Apparently there is no need for it so far, but would you like to see it integrated? With functors etc.
@mglisse I am also wondering why there is no bounding box class in
Epick_detc.
I remember thinking about it, maybe discussing it, but I don't remember the content...
Apparently there is no need for it so far,
That may have played a big role :wink: (it doesn't seem to be part of the Kernel_d concept)
but would you like to see it integrated? With functors etc.
If you have a use for it, we probably should.
To be sure I understand: the current bbox_23 always use double, whatever the kernel, making them not really kernel objects. Is the plan the same for the new bbox, i.e. ignore the generality offered by the template parameter and only use it with double (otherwise it becomes Iso_rectangle_2/Iso_cuboid_3)?
It looks like the kernel integration can be done later in a separate PR (this one adds the class), which gives me a bit of time to think about how that fits with the design of NewKernel_d.
We also need a constructor taking an iterator range of coordinates. And maybe also for a range of pairs of coordinates.
The former is needed when constructing a bbox from a point with floating point coordinates. The latter when constructing
it from exact coordinates transformed to a std::pair<double,double> using CGAL::to_double().
I just realize that this would correspond more to what you do for the I/O operators. But do we want a constructor for a range of low and a second range for high? Seems not intuitive.
As Epick_d may have a dynamic and a static dimension, depending on a template parameter, the functor Epick_d::Construct_bbox_d() must return either a bbox type with dynamic and static dimension, respectively.
@mglisse how can I obtain for Epick_d if it has a dynamic dimension or not? I want to add a Compute_bbox in function_objects_cartesian.h and when I want to specify the result_type I have to provide the appropriate Bbbox type which has a specialization for the dynamic bbox.
It would maybe a solution to put the Bbox type inside Epick_d.
In fact there is the nested type Dimension, So the new Bbox class should also use the Dimension_tag and Dynamic_dimension_tag instead of a partial specialization with 0 for dynamic.
I am wondering if it is not just something missing in Lazy_cartesian.h as I now got it working for Epick_d.
In fact there is the nested type
Dimension
Yes, that's also what we use in Gudhi to access the compile-time dimension of a CGAL Kernel. It is even documented in the Kernel_d concept :four_leaf_clover: .
@sloriot and I, we are blocked with getting Epick_d.cpp compiled. There is probably a specialization we have to add, but we have no idea what exactly,
@sloriot and I, we are blocked with getting
Epick_d.cppcompiled. There is probably a specialization we have to add, but we have no idea what exactly,
I mentioned looking at Point_dimension_tag in Lazy_cartesian.h. Something like
--- a/NewKernel_d/include/CGAL/NewKernel_d/Lazy_cartesian.h
+++ b/NewKernel_d/include/CGAL/NewKernel_d/Lazy_cartesian.h
@@ -309,24 +309,15 @@ struct Lazy_cartesian :
template<class T,class D> struct Functor<T,D,Construct_tag> {
typedef Lazy_construction2<T,Kernel> type;
};
- template<class D> struct Functor<Point_dimension_tag,D,Misc_tag> {
- typedef typename Get_functor<Approximate_kernel, Point_dimension_tag>::type FA;
- struct type {
- FA fa;
- type(){}
- type(Kernel const&k):fa(k.approximate_kernel()){}
- template<class P>
- int operator()(P const&p)const{return fa(CGAL::approx(p));}
- };
- };
- template<class D> struct Functor<Vector_dimension_tag,D,Misc_tag> {
- typedef typename Get_functor<Approximate_kernel, Vector_dimension_tag>::type FA;
+ template<class T,class D> struct Functor<T,D,Misc_tag> {
+ // By default (Point_dimension, Construct_bbox), apply to the approximate object
+ typedef typename Get_functor<Approximate_kernel, T>::type FA;
struct type {
FA fa;
type(){}
type(Kernel const&k):fa(k.approximate_kernel()){}
- template<class V>
- int operator()(V const&v)const{return fa(CGAL::approx(v));}
+ template<class P...>
+ decltype(auto) operator()(P&&...p)const{return fa(CGAL::approx(std::forward<P>(p))...);}
};
};
template<class D> struct Functor<Linear_base_tag,D,Misc_tag> {
(completely untested) If that still doesn't work, I'll checkout the branch to experiment, but not right now.
@mglisse in the meantime I achieved to make it compile and run. Not sure if that's the right way to do it.
@mglisse in the meantime I achieved to make it compile and run. Not sure if that's the right way to do it.
See https://github.com/mglisse/cgal/tree/Kernel_23-Bbox_d-ign, where I added commit https://github.com/mglisse/cgal/commit/316ab90c981f46542dc64a1fa95e70232db53147 .
@mglisse in the meantime I achieved to make it compile and run. Not sure if that's the right way to do it.
See https://github.com/mglisse/cgal/tree/Kernel_23-Bbox_d-ign, where I added commit mglisse@316ab90 .
That's indeed much simpler. I did not see that MISC macro. Thanks!