CGAL / cgal

The public CGAL repository, see the README below
https://github.com/CGAL/cgal#readme
Other
4.87k stars 1.38k forks source link

Linear kernel documentation vs Minkowski sum code #2010

Open chgans opened 7 years ago

chgans commented 7 years ago

Issue Details

  1. In it's section "Extensive Example", the Linear Kernel documentation claims that one doesn't need a custom point with a CGAL::Origin ctor, this is not true, at least when using minkowski sum as Point_2 Minkowski_sum_by_reduced_convolution_2::get_point(...) calls directly Point_2(Origin)

Which means that when using minkowski, you cannot use your custom Point class without explicitely depending on CGAL headers.

Not sure if the documentation needs an update or if minkowski sum needs a fix, or if actually there is no point in trying to use your own Point class with polygons?

2. Fixing the above isn't enough to use minkowski with the 'Extensive Example' code, one has to fix MyConstruct_coord_iterator. eg. from:

class MyConstruct_coord_iterator {
public:
  const double* operator()(const MyPointC2& p)
  {
    return &p.x();
  }
  const double* operator()(const MyPointC2& p, int)
  {
    const double* pyptr = &p.y();
    pyptr++;
    return pyptr;
  }
};

to

class MyConstruct_coord_iterator {
public:
    typedef const double *result_type;

  result_type operator()(const MyPointC2& p) const
  {
    return &p.x();
  }

  result_type operator()(const MyPointC2& p, int) const
  {
    result_type pyptr = &p.y();
    pyptr++;
    return pyptr;
  }
};

result_type was missing, and the operators were not const-correct.

Source Code

I took the files from Kernel_23/examples/Kernel_23/, and started to poke around with polygons and minkowski sums, didn't modify anything else than the above changes to MyConstruct_coord_iterator.

#include "MyKernel.h"

#include <CGAL/Polygon_2.h>
#include <CGAL/minkowski_sum_2.h>

typedef MyKernel<double> Kernel;
typedef Kernel::Point_2 Point;
typedef CGAL::Polygon_2<Kernel> Polygon;
typedef CGAL::Polygon_with_holes_2 <Kernel> Polygon_with_holes;

int main(int /*argc*/, char **/*argv*/)
{
    Polygon polygon1;
    polygon1.push_back(MyPointC2(0, 0));
    polygon1.push_back(MyPointC2(10, 0));
    polygon1.push_back(MyPointC2(10, 10));
    polygon1.push_back(MyPointC2(0, 10));

    Polygon polygon2;
    polygon2.push_back(MyPointC2(-1, 1));
    polygon2.push_back(MyPointC2(-1, -1));
    polygon2.push_back(MyPointC2(1, -1));
    polygon2.push_back(MyPointC2(1, 1));

    // This fails to compile 
    Polygon_with_holes polygon3 = CGAL::minkowski_sum_2(polygon1, polygon2);

    return 0;
}

Environment

KUbuntu 64 bits, defaults packages.

lrineau commented 7 years ago

Efi, Andreas, would you mind having a look at this bug report? It was created by a CGAL user a few weeks ago.

efifogel commented 7 years ago

There are two parts

  1. Minkowski_sum_by_reduced_convolution_2::get_point(...)

  2. MyConstruct_coord_iterator

  3. In the code of the get_point() member function we have:

... Point_2(ORIGIN) ...

I don't understand the problem. I guess that using a custom point, also implies that ORIGIN must be defined. Is this a problem? Perhaps it only should be documented.

  1. I think that we should add the missing type definition.


    //) o /__ // (__ ( ( ( (/ (/-(-'(/ /

On 19 May 2017 at 12:20, Laurent Rineau notifications@github.com wrote:

Efi, Andreas, would you mind having a look at this bug report? It was created by a CGAL user a few weeks ago.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/CGAL/cgal/issues/2010#issuecomment-302653416, or mute the thread https://github.com/notifications/unsubscribe-auth/AGoWuPVmRXXA48Rx8q3CZUFXDgfzfI_kks5r7V7tgaJpZM4MwrCy .

chgans commented 7 years ago

Hi,

Sorry i'm not actively using cgal anymore for now.

AFAIR, the point of this ticket was simply that you cannot use the "Extensive Example" code with the Minkowski code.

I wouldn't have open this ticket if one of the following were true:

Chris

efifogel commented 7 years ago

Don't get me wrong. I'm sure there is a problem, and we thank you for reporting it. I hope you get to actively use CGAL soon again...


//) o /__ // (__ ( ( ( (/ (/-(-'(/ /

On 22 May 2017 at 16:16, Christian Gagneraud notifications@github.com wrote:

Hi,

Sorry i'm not actively using cgal anymore for now.

AFAIR, the point of this ticket was simply that you cannot use the "Extensive Example" code with the Minkowski code.

I wouldn't have open this ticket if one of the following were true:

  • The documentation didn't say "You dont need a CGAL::Origin ctor"
  • Minkowski didn't use "Point_2(Origin)"

Chris

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/CGAL/cgal/issues/2010#issuecomment-303096746, or mute the thread https://github.com/notifications/unsubscribe-auth/AGoWuJ2Cw0DOVTMKTOA6FfZC2-qSyLBAks5r8YqTgaJpZM4MwrCy .

chgans commented 7 years ago

No problem! ;)

kabirkedia commented 3 years ago

Hi, I was looking into the issue and found that the following issues are to be addressed:-

  1. fixing the MyConstruct_coord_iterator.h file
  2. Fixing the Minkowski sum documentation to say that an origin needs to be defined while using your own point class

Am I right?