CGAL / cgal

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

3D Triangulation operator== #1261

Open myirci opened 8 years ago

myirci commented 8 years ago

Hi all, I want to report a minor bug and propose minor improvements for the overloaded equality operator function ( Source File: "Triangulation_3.h", Function: operator==).

CGAL Version 4.8.1


1) The following line given below ( Source File: "Triangulation_3.h", Function: operator==)

 if (dim < 1)
        return true;

should be changed with

if (dim < 0)
        return true;

A 3D triangulation with dimension 0 has a single finite vertex which has to be checked for the equality of two triangulations. For instance, the current operator== function returns true for the following two triangulations, which should return false.

Triangulation T1, T2;
T1.insert(Point(0,0,0));
T2.insert(Point(1,1,1));

Furhermore,

// Special case for dimension == 1.
if (dim == 1) {

should be changed with

if (dim < 2) {

That is, the curly bracket should be for dim = 0 and dim = 1. Since a triangulation with dimension 1 has two finite vertices, it should be enough to compare the vertices of two triangulations for equality.


2) For an improvement, std::map data structure can be replaced with

std::map<Vertex_handle1, Vertex_handle2> Vmap;
std::map<Cell_handle1, Cell_handle2> Cmap; 
std::unordered_map<Vertex_handle1, Vertex_handle2, CGAL::Handle_hash_function>   Vmap;
std::unordered_map<Cell_handle1, Cell_handle2, CGAL::Handle_hash_function>   Cmap;

3) The following lines are redundant for the function (test_next()) that is called form operator==.

if (t2.is_infinite(vn2))
          return false; // vn1 can't be infinite,
          // since it would have been registered.

Since initially infinite vertices are mapped to eachother and vn1 is queried in the map. Therefore, if vn2 is infinite, it is handled when vn1 is found in the map and these lines never runs.


Thank you.

Regards,

Murat Yirci

sgiraudot commented 7 years ago

@MoniqueTeillaud as package maintainer, what do you think of these different suggestions?

lrineau commented 7 years ago

@myirci commented on Jul 11, 2016, https://github.com/CGAL/cgal/issues/1261#issue-164889443:

Since a triangulation with dimension 1 has two finite vertices, it should be enough to compare the vertices of two triangulations for equality

A triangulation with dimension 1 can have more than two vertices: it can have two vertices or many more, as long as they are all colinear. But the rest of the sentence is true: it is sufficient to compare the set of points, because there is only one way to "triangulate" a set of colinear points, as far as I know.

MoniqueTeillaud commented 7 years ago

I agree with Laurent.

About another comment:

A 3D triangulation with dimension 0 has a single finite vertex which has to be checked for the > equality of two triangulations.

Agreed, this is a bug. The case of dimension 0 should be treated as for dimension 1: compare the point(s).

(sorry for the previous version of my answer, which I erased. I had not read the comment correctly)

lrineau commented 7 years ago

@MoniqueTeillaud, in https://github.com/CGAL/cgal/issues/1261#issuecomment-316949374:

A 3D triangulation with dimension 0 has a single finite vertex which has to be checked for the > equality of two triangulations.

The infinite vertex is just the infinite vertex. There is nothing to compare, the two triangulations have no finite vertex, they are equal.

No. A Triangulation_3 of dimension()==0 has exactly one finite vertex (plus the infinite vertex). An empty Triangulation_3 has dimension()==-1.

That is said here in the manual, in the subsection Degenerate Dimensions.

MoniqueTeillaud commented 7 years ago

(Laurent, I updated my comment, see its new version + the parenthesis...)