Add STL iterator capabilities to surface::RangeIteratorBase<T> and surface::NavigationIteratorBase<T>

include/geometrycentral/surface/halfedge_element_types.h

@@ -333,18 +333,25 @@ typedef RangeSetBase<BoundaryLoopRangeF> BoundaryLoopSet;
 // For implicit-twin meshes, it just does the usual twin.next() and currHe is always an outgoing halfedge. For general
 // meshes, it iterates first through the outgoing, then the incoming halfedges (always stored in currHe).
 struct VertexNeighborIteratorState {
+  VertexNeighborIteratorState() = default;
   VertexNeighborIteratorState(Halfedge currHeOutgoing, bool useImplicitTwin);
 
-  const bool useImplicitTwin;
+  void advance();
+  bool isHalfedgeCanonical() const; // this currently pointing at the one canonical halfedge along an edge
+  bool operator==(const VertexNeighborIteratorState& rhs) const;
+
+  bool useImplicitTwin() const { return useImplicitTwinFlag; }
+  bool processingIncoming() const { return processingIncomingFlag; }
+  Halfedge firstHalfedge() const { return firstHe; }
+  Halfedge currentHalfedge() const { return currHe; }
+
+private:
+  bool useImplicitTwinFlag;
   Halfedge currHe = Halfedge();
 
   // if useImplicitTwin == false, this is populated
-  bool processingIncoming = false;
+  bool processingIncomingFlag = false;
   Halfedge firstHe = Halfedge();
-
-  void advance();
-  bool isHalfedgeCanonical() const; // this currently pointing at the one canonical halfedge along an edge
-  bool operator==(const VertexNeighborIteratorState& rhs) const;
 };
 
 // Adjacent vertices

include/geometrycentral/surface/halfedge_element_types.ipp

@@ -134,26 +134,26 @@ inline bool VertexRangeF::elementOkay(const SurfaceMesh& mesh, size_t ind) {
 // Our data structure offers iteration around the outgoing halfedges from each vertex, which takes care of outoing halfedges. Furthermore one thing we know is that for every face in which the vertex appears, there is at least one incoming and one outgoing halfedge. We can use this to find the faces and outgoing halfedges.  For edges and vertices, 
 // In both the mannifold and nonmanifold case, if a vertex appears in a face multiple times, (aka its a Delta-complex), then these iterators will return elements multiple times.
 
-inline VertexNeighborIteratorState::VertexNeighborIteratorState(Halfedge currHe_, bool useImplicitTwin_) : useImplicitTwin(useImplicitTwin_), currHe(currHe_), firstHe(currHe_) {}
+inline VertexNeighborIteratorState::VertexNeighborIteratorState(Halfedge currHe_, bool useImplicitTwin_) : useImplicitTwinFlag(useImplicitTwin_), currHe(currHe_), firstHe(currHe_) {}
 
 // clang-format on
 inline void VertexNeighborIteratorState::advance() {
-  if (useImplicitTwin) {
+  if (useImplicitTwinFlag) {
     currHe = currHe.nextOutgoingNeighbor(); // twin().next()
   } else {
-    if (!processingIncoming) {
+    if (!processingIncomingFlag) {
       // this happens first
       currHe = currHe.nextOutgoingNeighbor();
       if (currHe == firstHe) { // switch to processing incoming if needed
-        processingIncoming = true;
+        processingIncomingFlag = true;
         currHe = firstHe.prevOrbitFace();
         firstHe = currHe;
       }
     } else {
       // this happens second
       currHe = currHe.nextIncomingNeighbor();
       if (currHe == firstHe) { // switch back to processing outgoing if needed (returning to initial state)
-        processingIncoming = false;
+        processingIncomingFlag = false;
         currHe = firstHe.next();
         firstHe = currHe;
       }
@@ -164,25 +164,25 @@ inline void VertexNeighborIteratorState::advance() {
 inline bool VertexNeighborIteratorState::isHalfedgeCanonical() const {
   // TODO I _think_ that this leads to different Delta-complex behavior on implicit twin vs. without wrt yielding
   // elements multiple times when there is a self-edge...
-  if (useImplicitTwin) {
+  if (useImplicitTwinFlag) {
     return true;
   } else {
     return currHe == currHe.edge().halfedge();
   }
 }
 
 inline bool VertexNeighborIteratorState::operator==(const VertexNeighborIteratorState& rhs) const {
-  return currHe == rhs.currHe && processingIncoming == rhs.processingIncoming;
+  return currHe == rhs.currHe && processingIncomingFlag == rhs.processingIncomingFlag;
 }
 // clang-format off
 
 inline void VertexAdjacentVertexNavigator::advance() { currE.advance(); }
 inline bool VertexAdjacentVertexNavigator::isValid() const { return currE.isHalfedgeCanonical(); }
 inline Vertex VertexAdjacentVertexNavigator::getCurrent() const {
-  if(currE.useImplicitTwin || !currE.processingIncoming) {
-    return currE.currHe.next().vertex();
+  if(currE.useImplicitTwin() || !currE.processingIncoming()) {
+    return currE.currentHalfedge().next().vertex();
   } else {
-    return currE.currHe.vertex();
+    return currE.currentHalfedge().vertex();
   }
 }
 
@@ -200,7 +200,7 @@ inline Corner VertexAdjacentCornerNavigator::getCurrent() const { return currE.c
 
 inline void VertexAdjacentEdgeNavigator::advance() { currE.advance(); }
 inline bool VertexAdjacentEdgeNavigator::isValid() const { return currE.isHalfedgeCanonical(); }
-inline Edge VertexAdjacentEdgeNavigator::getCurrent() const { return currE.currHe.edge(); }
+inline Edge VertexAdjacentEdgeNavigator::getCurrent() const { return currE.currentHalfedge().edge(); }
 
 inline void VertexAdjacentFaceNavigator::advance() { currE = currE.nextOutgoingNeighbor(); }
 inline bool VertexAdjacentFaceNavigator::isValid() const { return currE.isInterior(); }

include/geometrycentral/utilities/element_iterators.h

@@ -31,11 +31,19 @@ template <typename F>
 class RangeIteratorBase {
 
 public:
+  using difference_type = std::ptrdiff_t;
+  using value_type = typename F::Etype;
+  using pointer = value_type*;
+  using reference = const value_type&;
+  using iterator_category = std::forward_iterator_tag;
+
+  RangeIteratorBase() = default;
   RangeIteratorBase(typename F::ParentMeshT* mesh_, size_t iStart_, size_t iEnd_);
-  const RangeIteratorBase& operator++();
+  RangeIteratorBase& operator++();
+  RangeIteratorBase operator++(int);
   bool operator==(const RangeIteratorBase& other) const;
   bool operator!=(const RangeIteratorBase& other) const;
-  typename F::Etype operator*() const;
+  reference operator*() const;
 
 private:
   typename F::ParentMeshT* mesh;
@@ -81,11 +89,20 @@ template <typename N>
 class NavigationIteratorBase {
 
 public:
+  using difference_type = std::ptrdiff_t;
+  using value_type = typename N::Rtype;
+  using pointer = value_type*;
+  using reference = value_type;
+  using iterator_category = std::forward_iterator_tag;
+
+  NavigationIteratorBase() = default;
+
   NavigationIteratorBase(typename N::Etype firstE_, bool justStarted_);
-  const NavigationIteratorBase& operator++();
+  NavigationIteratorBase& operator++();
+  NavigationIteratorBase operator++(int);
   bool operator==(const NavigationIteratorBase& other) const;
   bool operator!=(const NavigationIteratorBase& other) const;
-  typename N::Rtype operator*() const;
+  reference operator*() const;
 
 private:
   N state;

include/geometrycentral/utilities/element_iterators.ipp

@@ -13,14 +13,21 @@ inline RangeIteratorBase<F>::RangeIteratorBase(typename F::ParentMeshT* mesh_, s
 }
 
 template <typename F>
-inline const RangeIteratorBase<F>& RangeIteratorBase<F>::operator++() {
+inline RangeIteratorBase<F>& RangeIteratorBase<F>::operator++() {
   iCurr++;
   while (iCurr != iEnd && !F::elementOkay(*mesh, iCurr)) {
     iCurr++;
   }
   return *this;
 }
 
+template <typename F>
+inline RangeIteratorBase<F> RangeIteratorBase<F>::operator++(int) {
+  auto ret = *this;
+  ++(*this);
+  return ret;
+}
+
 template <typename F>
 inline bool RangeIteratorBase<F>::operator==(const RangeIteratorBase<F>& other) const {
   return iCurr == other.iCurr;
@@ -32,7 +39,7 @@ inline bool RangeIteratorBase<F>::operator!=(const RangeIteratorBase<F>& other)
 }
 
 template <typename F>
-inline typename F::Etype RangeIteratorBase<F>::operator*() const {
+inline typename RangeIteratorBase<F>::reference RangeIteratorBase<F>::operator*() const {
   return typename F::Etype(mesh, iCurr);
 }
 
@@ -75,7 +82,7 @@ inline NavigationIteratorBase<N>::NavigationIteratorBase(typename N::Etype e, bo
 }
 
 template <typename N>
-inline const NavigationIteratorBase<N>& NavigationIteratorBase<N>::operator++() {
+inline NavigationIteratorBase<N>& NavigationIteratorBase<N>::operator++() {
   state.advance();
   while (!state.isValid()) {
     state.advance();
@@ -84,6 +91,13 @@ inline const NavigationIteratorBase<N>& NavigationIteratorBase<N>::operator++()
   return *this;
 }
 
+template <typename N>
+inline NavigationIteratorBase<N> NavigationIteratorBase<N>::operator++(int) {
+  auto ret = *this;
+  ++(*this);
+  return ret;
+}
+
 template <typename N>
 inline bool NavigationIteratorBase<N>::operator==(const NavigationIteratorBase<N>& other) const {
   return justStarted == other.justStarted && state.currE == other.state.currE;
@@ -95,7 +109,7 @@ inline bool NavigationIteratorBase<N>::operator!=(const NavigationIteratorBase<N
 }
 
 template <typename N>
-inline typename N::Rtype NavigationIteratorBase<N>::operator*() const {
+inline typename NavigationIteratorBase<N>::reference NavigationIteratorBase<N>::operator*() const {
   return state.getCurrent();
 }
 

test/CMakeLists.txt

@@ -93,6 +93,7 @@ set(TEST_SRCS
   src/main_test.cpp
   src/load_test_meshes.cpp
   src/eigen_interop_helpers_test.cpp
+  src/halfedge_iterator_categories.cpp
   src/halfedge_mesh_test.cpp
   src/halfedge_mutation_test.cpp
   src/halfedge_geometry_test.cpp

test/src/halfedge_iterator_categories.cpp

@@ -0,0 +1,111 @@
+#include "geometrycentral/surface/halfedge_element_types.h"
+
+#include "gtest/gtest.h"
+
+#include <type_traits>
+
+using namespace geometrycentral;
+using namespace geometrycentral::surface;
+
+// ============================================================
+// =============== Helper Type Traits
+// ============================================================
+
+// std::void_t is not in the standard library until c++17
+template <typename... Ts>
+using void_t = void;
+
+// std::is_swappable is not in the standard library until c++17
+template <typename T, typename = void>
+struct is_swappable : std::false_type {};
+template <typename T>
+struct is_swappable<T, void_t<decltype(std::swap(std::declval<T>(), std::declval<T>()))>> : std::true_type {};
+
+template <typename T, typename = void>
+struct is_dereferencable : std::false_type {};
+template <typename T>
+struct is_dereferencable<T, void_t<decltype(*std::declval<T&>())>> : std::true_type {};
+
+template <typename T, typename = void>
+struct is_pre_incrementable : std::false_type {};
+template <typename T>
+struct is_pre_incrementable<T, void_t<decltype(++std::declval<T&>())>> : std::true_type {};
+
+template <typename T, typename = void>
+struct is_post_incrementable : std::false_type {};
+template <typename T>
+struct is_post_incrementable<T, void_t<decltype(std::declval<T&>()++)>> : std::true_type {};
+
+// ============================================================
+// =============== Templated Test Setup
+// ============================================================
+
+template <typename T>
+class StlIteratorCategories : public ::testing::Test {};
+
+using GCRangeTypes = ::testing::Types<
+    HalfedgeSet, HalfedgeInteriorSet, HalfedgeExteriorSet, CornerSet, VertexSet, EdgeSet, FaceSet, BoundaryLoopSet,
+    NavigationSetBase<VertexAdjacentVertexNavigator>, NavigationSetBase<VertexIncomingHalfedgeNavigator>,
+    NavigationSetBase<VertexOutgoingHalfedgeNavigator>, NavigationSetBase<VertexAdjacentCornerNavigator>,
+    NavigationSetBase<VertexAdjacentEdgeNavigator>, NavigationSetBase<VertexAdjacentFaceNavigator>,
+    NavigationSetBase<EdgeAdjacentHalfedgeNavigator>, NavigationSetBase<EdgeAdjacentInteriorHalfedgeNavigator>,
+    NavigationSetBase<EdgeAdjacentFaceNavigator>, NavigationSetBase<FaceAdjacentVertexNavigator>,
+    NavigationSetBase<FaceAdjacentHalfedgeNavigator>, NavigationSetBase<FaceAdjacentCornerNavigator>,
+    NavigationSetBase<FaceAdjacentEdgeNavigator>, NavigationSetBase<FaceAdjacentFaceNavigator>,
+    NavigationSetBase<BoundaryLoopAdjacentVertexNavigator>, NavigationSetBase<BoundaryLoopAdjacentHalfedgeNavigator>,
+    NavigationSetBase<BoundaryLoopAdjacentEdgeNavigator>>;
+TYPED_TEST_SUITE(StlIteratorCategories, GCRangeTypes);
+
+
+// ============================================================
+// =============== Templated Test Cases
+// ============================================================
+
+TYPED_TEST(StlIteratorCategories, InputIteratorConcept) {
+  using iterator_t = decltype(std::declval<TypeParam>().begin());
+
+  // LegacyIterator requirements
+  EXPECT_TRUE(std::is_copy_constructible<iterator_t>::value);
+  EXPECT_TRUE(std::is_copy_assignable<iterator_t>::value);
+  EXPECT_TRUE(std::is_destructible<iterator_t>::value);
+  EXPECT_TRUE(is_swappable<iterator_t>::value);
+  // Check for iterator_traits typedefs
+  using value_type = typename std::iterator_traits<iterator_t>::value_type;
+  using difference_type = typename std::iterator_traits<iterator_t>::difference_type;
+  using pointer = typename std::iterator_traits<iterator_t>::pointer;
+  using reference = typename std::iterator_traits<iterator_t>::reference;
+  using iterator_category = typename std::iterator_traits<iterator_t>::iterator_category;
+  EXPECT_TRUE(is_dereferencable<iterator_t>::value);
+  EXPECT_TRUE(is_pre_incrementable<iterator_t>::value);
+
+  // LegacyInputIterator
+  // - operator-> is not checked for, the RangeIteratorBase and NavigationIteratorBase create their values on
+  //   dereference, so returning a pointer to them is asking for a dangling pointer. operator-> is not required to meet
+  //   the c++20 std::input_iterator concept so in practice not having this shouldnt affect the ability to use std
+  //   algorithms.
+  EXPECT_TRUE((std::is_convertible<decltype(std::declval<iterator_t>() != std::declval<iterator_t>()), bool>::value));
+  EXPECT_TRUE((std::is_convertible<decltype(*std::declval<iterator_t>()), value_type>::value));
+  EXPECT_TRUE(is_post_incrementable<iterator_t>::value);
+  EXPECT_TRUE((std::is_convertible<decltype(*std::declval<iterator_t>()++), value_type>::value));
+  EXPECT_TRUE((std::is_base_of<std::input_iterator_tag, iterator_category>::value));
+}
+
+TYPED_TEST(StlIteratorCategories, ForwardIteratorConcept) {
+  using iterator_t = decltype(std::declval<TypeParam>().begin());
+
+  using value_type = std::iterator_traits<iterator_t>::value_type;
+  using reference = std::iterator_traits<iterator_t>::reference;
+  using iterator_category = std::iterator_traits<iterator_t>::iterator_category;
+  // LegacyForwardIterator requirements
+  // Note: There are a number of LegacyForwardIterator requirements that we do not enforce here:
+  // - RangeIteratorBase<F>::reference is not a reference type as the values are generated on dereference, returning a
+  //   reference does not make sense
+  // - Because of the above, the multipass guarantee is not strictly met. The value returned from dereferencing two
+  //   iterators that compare equal will compare equal, but they will not be references to the same object. In practice
+  //   this doesnt matter as far as using std algorithms
+  EXPECT_TRUE(std::is_default_constructible<iterator_t>::value);
+  EXPECT_TRUE((std::is_same<decltype(std::declval<iterator_t>()++), iterator_t>::value));
+  EXPECT_TRUE((std::is_same<decltype(*std::declval<iterator_t>()++), reference>::value));
+  EXPECT_TRUE((std::is_same<std::decay<reference>::type, value_type>::value));
+  EXPECT_TRUE((std::is_base_of<std::forward_iterator_tag, iterator_category>::value));
+}