cgal-swig-bindings icon indicating copy to clipboard operation
cgal-swig-bindings copied to clipboard

insert_constraint error on Constrained_Delaunay_Triangulation_plus_2

Open Jiunixo opened this issue 2 years ago • 6 comments

Hello, We have a project using CGAL through cgal-bindings with CGAL 4.14.3. While upgrading the project to CGAL 5.4, we encountered an error in CDTP2.insert_constraint().

I reproduced the issue by adapting the example of the wiki conforming.py. (The original example works perfectly in our environment).

Code to reproduce failure :

from CGAL.CGAL_Kernel import Point_2
from CGAL.CGAL_Triangulation_2 import Constrained_Delaunay_triangulation_plus_2
from CGAL.CGAL_Triangulation_2 import Constrained_Delaunay_triangulation_plus_2_Vertex_handle
from CGAL import CGAL_Mesh_2
cdt=Constrained_Delaunay_triangulation_plus_2()
#construct a constrained triangulation
va = cdt.insert(Point_2( 5., 5.))
vb = cdt.insert(Point_2(-5., 5.))
vc = cdt.insert(Point_2( 4., 3.))
vd = cdt.insert(Point_2( 5.,-5.))
ve = cdt.insert(Point_2( 6., 6.))
vf = cdt.insert(Point_2(-6., 6.))
vg = cdt.insert(Point_2(-6.,-6.))
vh = cdt.insert(Point_2( 6.,-6.))
cdt.insert_constraint(va,vb) # => ERROR
cdt.insert_constraint(vb,vc)
cdt.insert_constraint(vc,vd)
cdt.insert_constraint(vd,va)
cdt.insert_constraint(ve,vf)
cdt.insert_constraint(vf,vg)
cdt.insert_constraint(vg,vh)
cdt.insert_constraint(vh,ve)
print("Number of vertices before: {}".format(cdt.number_of_vertices()))
#make it conforming Delaunay
CGAL_Mesh_2.make_conforming_Delaunay_2(cdt)
print("Number of vertices after make_conforming_Delaunay_2: {}".format(cdt.number_of_vertices()))
#then make it conforming Gabriel
CGAL_Mesh_2.make_conforming_Gabriel_2(cdt)

print("Number of vertices after make_conforming_Gabriel_2: {}".format(cdt.number_of_vertices()))

Error message : TypeError: 'Constrained_Delaunay_triangulation_plus_2_Vertex_handle' object is not iterable Additional information: Wrong number or type of arguments for overloaded function 'Constrained_Delaunay_triangulation_plus_2_insert_constraint'. Possible C/C++ prototypes are: Constrained_triangulation_plus_2_wrapper< CGAL_CDTplus2,Constrained_Delaunay_triangulation_2_wrapper< CGAL_CDTplus2,SWIG_Triangulation_2::CGAL_Vertex_handle< CGAL_CDTplus2,Point_2 >,SWIG_Triangulation_2::CGAL_Face_handle< CGAL_CDTplus2,Point_2 > >,SWIG_Triangulation_2::CGAL_Vertex_handle< CGAL_CDTplus2,Point_2 > >::insert_constraint(Polygon_2 const &) Constrained_triangulation_plus_2_wrapper< CGAL_CDTplus2,Constrained_Delaunay_triangulation_2_wrapper< CGAL_CDTplus2,SWIG_Triangulation_2::CGAL_Vertex_handle< CGAL_CDTplus2,Point_2 >,SWIG_Triangulation_2::CGAL_Face_handle< CGAL_CDTplus2,Point_2 > >,SWIG_Triangulation_2::CGAL_Vertex_handle< CGAL_CDTplus2,Point_2 > >::insert_constraint(Wrapper_iterator_helper< Point_2 >::input,bool) Constrained_triangulation_plus_2_wrapper< CGAL_CDTplus2,Constrained_Delaunay_triangulation_2_wrapper< CGAL_CDTplus2,SWIG_Triangulation_2::CGAL_Vertex_handle< CGAL_CDTplus2,Point_2 >,SWIG_Triangulation_2::CGAL_Face_handle< CGAL_CDTplus2,Point_2 > >,SWIG_Triangulation_2::CGAL_Vertex_handle< CGAL_CDTplus2,Point_2 > >::insert_constraint(Wrapper_iterator_helper< Point_2 >::input)

Investigation : CGAL : We haven't see any changes on this method between CGAL 4.14.3 and CGAL 5.4 in CGAL documentation. cgal_bindings :

  • In the former cgal_bindings version working with CGAL 4.14.3, CDTP2.insert_constraint() calls Internal_Contrained_triangulation_2_Internal_Constrained_Delaunay_triangulation_2_Constrained_Delaunay_triangulation_plus_2.insert_constraint() which works.
  • With the last version of cgal-bindings and CGAL 5.4, CDTP2.insert_constraint() calls Constrained_Delaunay_triangulation_plus_2.insert_constraint() which raises the error above. We note that in the old bindings this method does not exist in CGAL_Triangulation_2.py.

We keep on investigating to find a solution and any help is welcome.

Thanks in advance. Jean.

Jiunixo avatar Apr 06 '22 10:04 Jiunixo

@sloriot It seems there is a sort of confusion: the Python code should call: insert_constraint(Vertex_handle va, Vertex_handle vb) but instead it calls insert_constraint(PointIterator first, PointIterator last, bool close=false)

lrineau avatar Apr 06 '22 12:04 lrineau

I suspect it is a bug with SWIG. I tried 4.3.3 with the 4.x branch and it does not work either. Maybe it was working for you but with a earlier version of SWIG. The issue is because of overloads. It I rename the function in the base wrapper class, it works. I don't know yet what is the best solution (without breaking the API).

sloriot avatar Apr 07 '22 19:04 sloriot

Thank you for your return. Our version of swig was and is still 4.0.1.

I've tried to add SWIG_CGAL_FORWARD_CALL_2(void,insert_constraint,Vertex_handle,Vertex_handle) in Constrained_triangulation_plus_2.h. The first insert_constraint works but the second one raises a new exception : SystemError: <built-in function Mesh_2_Constrained_Delaunay_triangulation_plus_2_insert_constraint> returned a result with an error set

Jiunixo avatar Apr 08 '22 10:04 Jiunixo

I've got a new track. Our project is using a fork of cgal-swig-bindings developed by Logilab : https://github.com/logilab/cgal-swig-bindings On the actual Head of official cgal-swig-bindings, I've reverted the commit https://github.com/logilab/cgal-swig-bindings/commit/d423fe33ab33a6cbcba0246b6acd943501bf138a as Logilab did on the last commit of their fork. Adapting the code a little, the cdt plus example works.

Obviously, it's not very satisfying to revert a commit which is probably useful for other users. But by analysing the changes introduced on this commit, it can give us some tips on how to solve the problem.

In first analysis, the problem could be related with the introduction of Contraint_id_wrapper, as the suppression of this class in Triangulation_2 package, solves the problem.

Jiunixo avatar Apr 08 '22 17:04 Jiunixo

As I said, SWIG is confused by the overload insert_constraint(Wrapper_iterator_helper<Point_2>::input input, bool closed=false) which get introduced in the aforementioned commit. The easiest fix would be to renamed that function but it would mean breaking the compatibility.

sloriot avatar Apr 11 '22 06:04 sloriot

We found this workaround which works : cdt.insert_constraint([va.point(), vb.point()])

I hope it can help anyone who encounters same problem. For us, this issue can be closed.

Jiunixo avatar Sep 27 '23 08:09 Jiunixo