Merge pull request #3884 from MaelRL/BGL-Fix_FFG_master-GF

BGL: Fix `Face_filtered_graph`'s `is_selection_valid()` (5.0)

BGL/include/CGAL/boost/graph/Face_filtered_graph.h

@@ -463,64 +463,57 @@ struct Face_filtered_graph
 
   /// returns `true` if around any vertex of a selected face,
   /// there is at most one connected set of selected faces.
-  bool is_selection_valid()
+  bool is_selection_valid() const
   {
-    for(vertex_descriptor vd : vertices(*this) )
+    typedef typename boost::graph_traits<Graph>::vertex_descriptor      vertex_descriptor;
+    typedef typename boost::graph_traits<Graph>::halfedge_descriptor    halfedge_descriptor;
+
+    // Non-manifoldness can appear either:
+    // - if 'pm' is pinched at a vertex. While traversing the incoming halfedges at this vertex,
+    //   we will meet strictly more than one border halfedge.
+    // - if there are multiple umbrellas around a vertex. In that case, we will find a non-visited
+    //   halfedge that has for target a vertex that is already visited.
+
+    boost::unordered_set<vertex_descriptor> vertices_visited;
+    boost::unordered_set<halfedge_descriptor> halfedges_handled;
+
+    for(halfedge_descriptor hd : halfedges(*this))
     {
-      face_descriptor first_selected = boost::graph_traits<Graph>::null_face();
-      bool first_unselected_found(false),
-           second_unselected_found(false);
+      CGAL_assertion(is_in_cc(hd));
 
-      //find an unselected face, then find the first selected face.
-      //Find another unselected face, the next selected face must be the first;
-      //else this is not valid.
-      halfedge_descriptor hd = halfedge(vd, _graph);
-      face_descriptor first_tested = boost::graph_traits<Graph>::null_face();
-      while(1) //will break if valid, return false if not valid
+      if(!halfedges_handled.insert(hd).second) // already treated this halfedge
+        continue;
+
+      vertex_descriptor vd = target(hd, *this);
+      CGAL_assertion(is_in_cc(vd));
+
+      // Check if we have already met this vertex before (necessarily in a different umbrella
+      // since we have never treated the halfedge 'hd')
+      if(!vertices_visited.insert(vd).second)
+        return false;
+
+      std::size_t border_halfedge_counter = 0;
+
+      // Can't simply call halfedges_around_target(vd, *this) because 'halfedge(vd)' is not necessarily 'hd'
+      halfedge_descriptor ihd = hd;
+      do
       {
-        face_descriptor fd = face(hd, _graph);
+        halfedges_handled.insert(ihd);
+        if(is_border(ihd, *this))
+          ++border_halfedge_counter;
 
-        if(first_tested == boost::graph_traits<Graph>::null_face())
-          first_tested = fd;
-        else if(fd == first_tested )
+        do
         {
-          //if there is no unselected face, break
-          if(selected_faces[get(fimap, fd)] && !first_unselected_found)
-          break;
-          //if there is no selected face, break
-          else if(!selected_faces[get(fimap, fd)] &&
-             first_selected == boost::graph_traits<Graph>::null_face())
-          break;
+          ihd = prev(opposite(ihd, _graph), _graph);
         }
-
-        if(fd != boost::graph_traits<Graph>::null_face())
-        {
-          if(selected_faces[get(fimap, fd)])
-          {
-            if(first_unselected_found &&
-               first_selected == boost::graph_traits<Graph>::null_face())
-            {
-              first_selected = fd;
-            }
-            else if(second_unselected_found)
-            {
-              if(fd == first_selected)
-                break;
-              else
-                return false;
-            }
-          }
-          else
-          {
-            if(first_selected == boost::graph_traits<Graph>::null_face())
-              first_unselected_found = true;
-            else
-              second_unselected_found = true;
-          }
-        }
-        hd = next(opposite(hd, _graph), _graph);
+        while(!is_in_cc(ihd) && ihd != hd);
       }
+      while(ihd != hd);
+
+      if(border_halfedge_counter > 1)
+        return false;
     }
+
     return true;
   }
 

BGL/test/BGL/test_Face_filtered_graph.cpp

@@ -268,17 +268,20 @@ void test_read(const Graph& g)
   typedef typename boost::graph_traits<Graph>::face_descriptor g_face_descriptor;
   typedef CGAL::Face_filtered_graph<Graph> Adapter;
   CGAL_GRAPH_TRAITS_MEMBERS(Adapter);
+
   std::map<g_face_descriptor, std::size_t> map;
   CGAL::Polygon_mesh_processing::connected_components(g, boost::make_assoc_property_map(map), CGAL::Polygon_mesh_processing::parameters::all_default());
   Adapter fg(g, 0, boost::make_assoc_property_map(map));
+  assert(fg.is_selection_valid());
   assert(CGAL::is_valid_polygon_mesh(fg));
 }
 
 template <typename Graph>
 void
-test(const std::vector<Graph>& graphs)
+test_graph_range(const std::vector<Graph>& graphs)
 {
-  for(Graph p : graphs){
+  for(Graph p : graphs)
+  {
     test_read(p);
     test_vertex_iterators(p);
     test_out_edges(p);
@@ -392,14 +395,75 @@ void test_mesh(Adapter fga)
   CGAL::copy_face_graph(fga, copy);
 }
 
-
-int
-main()
+template <typename PolygonMesh>
+void merge_vertices(typename boost::graph_traits<PolygonMesh>::vertex_descriptor v_keep,
+                    typename boost::graph_traits<PolygonMesh>::vertex_descriptor v_rm,
+                    std::vector<typename boost::graph_traits<PolygonMesh>::face_descriptor>& incident_faces,
+                    PolygonMesh& mesh)
 {
-  test(sm_data());
-#ifdef CGAL_USE_OPENMESH
-  test(omesh_data());
+  typedef typename boost::graph_traits<PolygonMesh>::halfedge_descriptor halfedge_descriptor;
+
+  halfedge_descriptor oh = halfedge(v_keep, mesh), done = oh;
+  do
+  {
+    incident_faces.push_back(face(oh, mesh));
+    oh = opposite(next(oh, mesh), mesh);
+  }
+  while(oh != done);
+
+  halfedge_descriptor h = halfedge(v_rm, mesh);
+  halfedge_descriptor start = h;
+  do
+  {
+    set_target(h, v_keep, mesh);
+    incident_faces.push_back(face(h, mesh));
+    h = opposite(next(h, mesh), mesh);
+  }
+  while(h != start);
+
+  remove_vertex(v_rm, mesh);
+}
+
+void test_invalid_selections()
+{
+  // this creates a non-manifold (pinched) vertex
+  SM mesh;
+  read_a_mesh(mesh, "data/7_faces_triangle.off");
+
+  std::vector<SM::Face_index> face_range;
+  face_range.push_back(SM::Face_index(1));
+  face_range.push_back(SM::Face_index(2));
+  face_range.push_back(SM::Face_index(3));
+
+  CGAL::Face_filtered_graph<SM> bad_fg(mesh, face_range);
+  assert(!bad_fg.is_selection_valid());
+
+  // this creates a non-manifold vertex (multiple umbrellas)
+  clear(mesh);
+  read_a_mesh(mesh, "data/genus3.off");
+  assert(is_valid_polygon_mesh(mesh));
+
+  face_range.clear();
+  merge_vertices(SM::Vertex_index(1337), SM::Vertex_index(87), face_range, mesh);
+
+  CGAL::Face_filtered_graph<SM> bad_fg_2(mesh, face_range);
+  assert(!bad_fg_2.is_selection_valid());
+}
+
+int main()
+{
+  test_graph_range(poly_data());
+
+#if defined(CGAL_USE_SURFACE_MESH)
+  test_graph_range(sm_data());
 #endif
+
+#ifdef CGAL_USE_OPENMESH
+  test_graph_range(omesh_data());
+#endif
+
+  test_invalid_selections();
+
   //Make a tetrahedron and test the adapter for a patch that only contains 2 faces
   typedef CGAL::Face_filtered_graph<SM> SM_Adapter;
   typedef SM::Property_map<boost::graph_traits<SM>::face_descriptor , std::size_t> SM_FCCMap;