fix visitor call in clip + add tests

Polygon_mesh_processing/include/CGAL/Polygon_mesh_processing/clip.h

@@ -522,24 +522,39 @@ generic_clip_impl(
 #ifndef CGAL_PLANE_CLIP_DO_NOT_USE_TRIANGULATION
 template <class PolygonMesh, class Clip_visitor>
 struct Visitor_wrapper_for_triangulate_face
+  : public ::CGAL::Polygon_mesh_processing::Hole_filling::Default_visitor
+//TODO: @afabri --> I shouldn't be the one doing the inheritance...
+//TODO: @afabri --> nothing on edge?
 {
   using face_descriptor = typename boost::graph_traits<PolygonMesh>::face_descriptor;
 
   Clip_visitor& clip_visitor;
   const PolygonMesh& pm;
+  std::vector<face_descriptor> triangulation_faces;
 
   Visitor_wrapper_for_triangulate_face(const PolygonMesh& pm, Clip_visitor& clip_visitor)
     : pm(pm)
     , clip_visitor(clip_visitor)
   {}
 
-  void before_subface_creations(face_descriptor /* f_split */) {}
+  void before_subface_creations(face_descriptor f_split)
-  void after_subface_creations() {}
+  {
+    CGAL_assertion(triangulation_faces.empty());
+    triangulation_faces.push_back(f_split);
+  }
+  void after_subface_creations()
+  {
+    for (face_descriptor f : triangulation_faces)
+    {
+      clip_visitor.before_face_copy(boost::graph_traits<PolygonMesh>::null_face(), pm, pm);
+      clip_visitor.after_face_copy(boost::graph_traits<PolygonMesh>::null_face(), pm, f, pm);
+    }
+    triangulation_faces.clear();
+  }
 
   void after_subface_created(face_descriptor f_new)
   {
-    clip_visitor.before_face_copy(boost::graph_traits<PolygonMesh>::null_face(), pm, pm);
+    triangulation_faces.push_back(f_new);
-    clip_visitor.after_face_copy(boost::graph_traits<PolygonMesh>::null_face(), pm, f_new, pm);
   }
 };
 #endif
@@ -808,9 +823,11 @@ bool clip(PolygonMesh& pm,
                                           .do_not_triangulate_faces(!triangulate)
                                           .throw_on_self_intersection(!allow_self_intersections &&
                                                                       throw_on_self_intersection)
-                                          .visitor(visitor)
+                                          .visitor(std::ref(visitor))
                                           .concurrency_tag(Concurrency_tag()));
 
+  CGAL_assertion(is_valid_polygon_mesh(pm));
+
   if (allow_self_intersections)
     clip_volume=false;
 
@@ -853,15 +870,14 @@ bool clip(PolygonMesh& pm,
 
     for (halfedge_descriptor h : borders)
     {
-      visitor.before_face_copy(boost::graph_traits<PolygonMesh>::null_face(), pm, pm);
-      Euler::fill_hole(h, pm);
-      visitor.after_face_copy(boost::graph_traits<PolygonMesh>::null_face(), pm, face(h, pm), pm);
-
 #ifndef CGAL_PLANE_CLIP_DO_NOT_USE_TRIANGULATION
       if (triangulate)
       {
+        Euler::fill_hole(h, pm); // visitor call done in the triangulation visitor
         if constexpr (!has_visitor)
+        {
           triangulate_face(face(h,pm), pm, parameters::vertex_point_map(vpm).geom_traits(traits));
+        }
         else
         {
           using Base_visitor = std::remove_cv_t<std::remove_reference_t<Visitor_ref>>;
@@ -869,7 +885,14 @@ bool clip(PolygonMesh& pm,
           triangulate_face(face(h,pm), pm, parameters::vertex_point_map(vpm).geom_traits(traits).visitor(visitor_wrapper));
         }
       }
+      else
 #endif
+      {
+        visitor.before_face_copy(boost::graph_traits<PolygonMesh>::null_face(), pm, pm);
+        Euler::fill_hole(h, pm);
+        visitor.after_face_copy(boost::graph_traits<PolygonMesh>::null_face(), pm, face(h, pm), pm);
+      }
+
     }
   }
 
@@ -1141,6 +1164,7 @@ void split(PolygonMesh& pm,
 {
   using parameters::choose_parameter;
   using parameters::get_parameter;
+  using parameters::get_parameter_reference;
 
   using GT = typename GetGeomTraits<PolygonMesh, NamedParameters>::type;
   GT traits = choose_parameter<GT>(get_parameter(np, internal_np::geom_traits));
@@ -1164,12 +1188,10 @@ void split(PolygonMesh& pm,
   if (triangulate && !is_triangle_mesh(pm))
     triangulate = false;
 
-  typedef typename internal_np::Lookup_named_param_def <
+  using Default_visitor = Corefinement::Default_visitor<PolygonMesh>;
-    internal_np::visitor_t,
+  Default_visitor default_visitor;
-    NamedParameters,
+  using Visitor_ref = typename internal_np::Lookup_named_param_def<internal_np::visitor_t, NamedParameters, Default_visitor>::reference;
-    Corefinement::Default_visitor<PolygonMesh>//default
+  Visitor_ref visitor = choose_parameter(get_parameter_reference(np, internal_np::visitor), default_visitor);
-  > ::type User_visitor;
-  User_visitor uv(choose_parameter<User_visitor>(get_parameter(np, internal_np::visitor)));
 
   refine_with_plane(pm, plane, parameters::vertex_oriented_side_map(vos)
                                           .edge_is_marked_map(ecm)
@@ -1177,10 +1199,11 @@ void split(PolygonMesh& pm,
                                           .geom_traits(traits)
                                           .do_not_triangulate_faces(!triangulate)
                                           .throw_on_self_intersection(throw_on_self_intersection)
-                                          .concurrency_tag(Concurrency_tag()));
+                                          .concurrency_tag(Concurrency_tag())
+                                          .visitor(std::ref(visitor)));
 
   //split mesh along marked edges
-  internal::split_along_edges(pm, ecm, vpm, uv);
+  internal::split_along_edges(pm, ecm, vpm, visitor);
 }
 
 

Polygon_mesh_processing/include/CGAL/Polygon_mesh_processing/refine_with_plane.h

@@ -429,7 +429,7 @@ void refine_with_plane(PolygonMesh& pm,
       if (is_border(h, pm)) continue;
       Oriented_side prev_ori = get(vertex_os, source(h, pm)),
                     next_ori = get(vertex_os, target(next(h, pm), pm));
-      if ( prev_ori == ON_ORIENTED_BOUNDARY || next_ori == ON_ORIENTED_BOUNDARY) continue; // skip full edge
+      if (prev_ori == ON_ORIENTED_BOUNDARY || next_ori == ON_ORIENTED_BOUNDARY) continue; // skip full edge
       if (prev_ori!=next_ori) splitted_faces[face(h, pm)].push_back(h); // skip tangency point
     }
   }
@@ -449,7 +449,7 @@ void refine_with_plane(PolygonMesh& pm,
       CGAL_assertion(next(h1,pm)!=h2 && next(h2,pm)!=h1); // the edge does not already exist
       visitor.before_subface_created(pm);
       halfedge_descriptor res = CGAL::Euler::split_face(h1, h2, pm);
-      visitor.after_subface_created(face(res, pm), pm);
+      visitor.after_subface_created(face(h2, pm), pm);
       put(edge_is_marked, edge(res, pm), true);
       visitor.add_retriangulation_edge(res, pm);
 
@@ -458,9 +458,10 @@ void refine_with_plane(PolygonMesh& pm,
         if (!is_triangle(res, pm))
         {
           // TODO: take the criteria in triangulate_faces ?
+          visitor.before_subface_created(pm);
           halfedge_descriptor newh =
             CGAL::Euler::split_face(res, next(next(res, pm), pm), pm);
-          put(edge_is_marked, edge(newh, pm), false);
+          visitor.after_subface_created(face(opposite(newh, pm), pm), pm);
           visitor.add_retriangulation_edge(newh, pm);
         }
         else
@@ -472,8 +473,8 @@ void refine_with_plane(PolygonMesh& pm,
             visitor.before_subface_created(pm);
             halfedge_descriptor newh =
               CGAL::Euler::split_face(res, next(next(res, pm), pm), pm);
-            visitor.after_subface_created(face(newh, pm), pm);
+            visitor.after_subface_created(face(opposite(newh, pm), pm), pm);
-            put(edge_is_marked, edge(newh, pm), false);
+            visitor.add_retriangulation_edge(newh, pm);
           }
         }
       }
@@ -498,11 +499,11 @@ void refine_with_plane(PolygonMesh& pm,
         if constexpr (has_visitor)
         {
           if (face(h1,pm)!=face(h2,pm))
-            visitor.after_subface_created(face(res, pm), pm);
+            visitor.after_subface_created(face(h2, pm), pm);
-        }
-        put(edge_is_marked, edge(res, pm), true);
           visitor.add_retriangulation_edge(res, pm);
         }
+        // put(edge_is_marked, edge(res, pm), true);
+      }
     }
 
     visitor.after_subface_creations(pm);

Polygon_mesh_processing/include/CGAL/Polygon_mesh_processing/triangulate_faces.h

@@ -119,9 +119,10 @@ private:
       if(first)
         first = false;
       else
+      {
         f = add_face(pmesh);
-
         visitor.after_subface_created(f);
+      }
 
       std::array<int, 4> indices = make_array(triangle.first,
                                               triangle.second,

Polygon_mesh_processing/test/Polygon_mesh_processing/test_pmp_clip.cpp

@@ -905,6 +905,237 @@ void test_new_clip()
   }
 }
 
+struct Clip_and_split_visitor
+{
+  using halfedge_descriptor = typename boost::graph_traits<Surface_mesh>::halfedge_descriptor;
+  using face_descriptor = typename boost::graph_traits<Surface_mesh>::face_descriptor;
+  using vertex_descriptor = typename boost::graph_traits<Surface_mesh>::vertex_descriptor;
+
+  Surface_mesh& sm;
+  Surface_mesh::Property_map<Surface_mesh::Face_index, int> fid_map;
+  Surface_mesh::Property_map<Surface_mesh::Vertex_index, int> vid_map;
+  Surface_mesh::Property_map<Surface_mesh::Halfedge_index, int> hid_map;
+
+  int fid=-1;
+  int hid=-1;
+  std::size_t nbf=0;
+  std::size_t nbe=0;
+  std::size_t nb_input_v=sm.number_of_vertices();
+  std::size_t nb_input_v_on=0;
+  std::size_t nb_new_v_on=0;
+
+  Clip_and_split_visitor(Surface_mesh& sm)
+    : sm(sm)
+  {
+    bool is_new=false;
+    std::tie(fid_map, is_new)=sm.add_property_map<Surface_mesh::Face_index,int>("f:id", -1);
+    assert(is_new);
+    int i=0; for (auto f : faces(sm)) put(fid_map, f, i++);
+    std::tie(vid_map, is_new)=sm.add_property_map<Surface_mesh::Vertex_index,int>("v:id", -1);
+    assert(is_new);
+    i=0; for (auto v : vertices(sm)) put(vid_map, v, i++);
+    std::tie(hid_map, is_new)=sm.add_property_map<Surface_mesh::Halfedge_index,int>("h:id", -1);
+    assert(is_new);
+    i=0;
+    for (auto e : edges(sm))
+    {
+      auto h = halfedge(e, sm);
+      put(hid_map, opposite(h, sm), i);
+      put(hid_map, h, i++);
+    }
+  }
+
+  void before_subface_creations(face_descriptor f_split, const Surface_mesh& pm)
+  {
+    fid=get(fid_map, f_split);
+    assert(fid!=-1);
+    assert(&pm==&sm);
+  }
+  void after_subface_creations(const Surface_mesh& pm)
+  {
+    assert(&pm==&sm);
+  }
+  void before_subface_created(const Surface_mesh& pm)
+  {
+    nbf=sm.number_of_faces();
+    assert(&pm==&sm);
+  }
+  void after_subface_created(face_descriptor f_new, const Surface_mesh& pm)
+  {
+    assert(&pm==&sm);
+    assert(get(fid_map, f_new)==-1);
+    put(fid_map, f_new, fid);
+    assert(nbf+1==sm.number_of_faces());
+  }
+
+  void before_edge_split(halfedge_descriptor h, Surface_mesh& pm)
+  {
+    hid=get(hid_map, h);
+    assert(hid!=-1);
+    assert(&pm==&sm);
+    nbe=sm.number_of_edges();
+  }
+  void edge_split(halfedge_descriptor hnew, Surface_mesh& pm)
+  {
+    assert(&pm==&sm);
+    assert(get(hid_map, hnew)==-1);
+    assert(hid!=-1);
+    put(hid_map, hnew, hid);
+    put(hid_map, opposite(hnew, sm), hid);
+    assert(nbe+1==sm.number_of_edges());
+  }
+  void after_edge_split()
+  {
+    assert(nbe+1==sm.number_of_edges());
+  }
+  void add_retriangulation_edge(halfedge_descriptor hnew, const Surface_mesh& pm)
+  {
+    assert(get(hid_map, hnew)==-1);
+    put(hid_map, hnew, -2);
+    put(hid_map, opposite(hnew, sm), -2);
+    assert(&pm==&sm);
+  }
+
+  void intersection_point_detected(std::size_t /* i_id */,
+                                   int /* sdim */,
+                                   halfedge_descriptor h_e,
+                                   halfedge_descriptor h_f,
+                                   const Surface_mesh& tm_e,
+                                   const Surface_mesh& tm_f,
+                                   bool is_target_coplanar,
+                                   bool is_source_coplanar)
+  {
+    assert(is_source_coplanar==false);
+    assert(h_f==boost::graph_traits<Surface_mesh>::null_halfedge());
+    assert(h_e!=boost::graph_traits<Surface_mesh>::null_halfedge());
+    if (!is_target_coplanar)
+      ++nb_new_v_on;
+    else
+      ++nb_input_v_on;
+    assert(&tm_e==&sm);
+    assert(&tm_f==&sm);
+  }
+
+  void new_vertex_added(std::size_t id, vertex_descriptor v, const Surface_mesh& pm)
+  {
+    assert(&pm==&sm);
+    assert(get(vid_map, v)==-1);
+    put(vid_map, v, nb_input_v+id-nb_input_v_on);
+  }
+
+  void before_face_copy(face_descriptor f, const Surface_mesh& src, const Surface_mesh& tgt)
+  {
+    assert(f==boost::graph_traits<Surface_mesh>::null_face());
+    assert(&src==&sm);
+    assert(&tgt==&sm);
+  }
+
+  void after_face_copy(face_descriptor fsrc, const Surface_mesh& src, face_descriptor ftgt, const Surface_mesh& tgt)
+  {
+    assert(fsrc==boost::graph_traits<Surface_mesh>::null_face());
+    assert(ftgt!=boost::graph_traits<Surface_mesh>::null_face());
+    //assert(get(fid_map, ftgt)==-1);
+    put(fid_map, ftgt, -2);
+    assert(&src==&sm);
+    assert(&tgt==&sm);
+    for (auto h : halfedges_around_face(halfedge(ftgt, sm), sm))
+    {
+      if (get(hid_map, h)==-1)
+        put(hid_map, h, -2);
+    }
+  }
+
+  void before_edge_duplicated(halfedge_descriptor h, Surface_mesh& tm)
+  {
+    hid = get(hid_map, h);
+    assert(hid!=-1);
+    assert(&tm==&sm);
+  }
+
+  void after_edge_duplicated(halfedge_descriptor h, halfedge_descriptor new_hedge, Surface_mesh& tm)
+  {
+    assert(hid==get(hid_map, h));
+    assert(&tm==&sm);
+    put(hid_map, new_hedge, hid);
+    put(hid_map, opposite(new_hedge, sm), hid);
+  }
+
+  void before_vertex_copy(vertex_descriptor v, Surface_mesh& src, Surface_mesh& tgt)
+  {
+    assert(&src==&sm);
+    assert(&tgt==&sm);
+    assert(get(vid_map, v)!=-1);
+  }
+  void after_vertex_copy(vertex_descriptor v, Surface_mesh& src, vertex_descriptor nv, Surface_mesh& tgt)
+  {
+    assert(&src==&sm);
+    assert(&tgt==&sm);
+    assert(get(vid_map, v)!=-1);
+    put(vid_map, nv, get(vid_map, v));
+  }
+
+
+
+  void check()
+  {
+    for (auto f :faces(sm))
+    {
+      if (get(fid_map, f)==-1) std::cout << sm.point(source(halfedge(f, sm), sm)) << " " << sm.point(target(halfedge(f, sm), sm)) << " " << sm.point(target(next(halfedge(f, sm), sm), sm)) << "\n";
+      assert(get(fid_map, f)!=-1);
+    }
+    for (auto h :halfedges(sm))
+    {
+      if (get(hid_map, h)==-1)
+        std::cout << sm.point(source(h, sm)) << " " << sm.point(target(h,sm)) << "\n";
+      assert(get(hid_map, h)!=-1);
+    }
+    std::size_t nbv_max=nb_input_v+nb_new_v_on;
+    for (auto v :vertices(sm))
+      assert(get(vid_map, v)!=-1 && get(vid_map, v)<(int)nbv_max);
+  }
+
+};
+
+void test_clip_and_split_with_plane_visitor()
+{
+  auto test_clip =[](std::string fname, const K::Plane_3& plane, bool triangulate, bool clip_volume)
+  {
+    std::cout << "   testing with clip " << fname << " vs " << plane << " (" << triangulate << "," << clip_volume << ")\n";
+    Surface_mesh sm;
+    std::ifstream(fname) >> sm;
+    Clip_and_split_visitor visitor(sm);
+    visitor.check();
+    PMP::clip(sm, plane, params::visitor(std::ref(visitor)).do_not_triangulate_faces(!triangulate).clip_volume(clip_volume));
+    std::ofstream ("/tmp/out.off") << sm;
+    visitor.check();
+  };
+
+  auto test_split =[](std::string fname, const K::Plane_3& plane, bool triangulate)
+  {
+    std::cout << "   testing with split" << fname << " vs " << plane << " (" << triangulate << ")\n";
+    Surface_mesh sm;
+    std::ifstream(fname) >> sm;
+    Clip_and_split_visitor visitor(sm);
+    visitor.check();
+    PMP::split(sm, plane, params::visitor(std::ref(visitor)).do_not_triangulate_faces(!triangulate));
+    visitor.check();
+  };
+
+  auto test = [&](std::string fname, const K::Plane_3& plane)
+  {
+    test_clip(fname, plane, true, true);
+    test_clip(fname, plane, true, false);
+    test_clip(fname, plane, false, false);
+    test_clip(fname, plane, false, true);
+    test_split(fname, plane, false);
+    test_split(fname, plane, true);
+  };
+
+  test(CGAL::data_file_path("meshes/torus_quad.off"), K::Plane_3(0,0,1,0));
+  test(CGAL::data_file_path("meshes/elephant.off"), K::Plane_3(0.137304, -0.293668, 0.945995, 0));
+
+}
+
 int main()
 {
   std::cout << "Surface Mesh" << std::endl;
@@ -933,5 +1164,8 @@ int main()
   std::cout << "running test_new_clip with Polyhedron\n";
   test_new_clip<Polyhedron>();
   std::cout << "Done!" << std::endl;
+  std::cout << "running test_clip_and_split_with_plane_visitor\n";
+  test_clip_and_split_with_plane_visitor();
+  std::cout << "Done!" << std::endl;
   return EXIT_SUCCESS;
 }