Address feedback

Signed-off-by: Dan Bailey <[email protected]>
AcademySoftwareFoundation · Oct 27, 2024 · 485ff98 · 485ff98
1 parent f9a34cb
commit 485ff98
Show file tree

Hide file tree

Showing 3 changed files with 38 additions and 108 deletions.
diff --git a/openvdb/openvdb/tree/InternalNode.h b/openvdb/openvdb/tree/InternalNode.h
@@ -620,41 +620,43 @@ class InternalNode
     /// If no such node exists, return nullptr.
     template<typename NodeType> NodeType* probeNode(const Coord& xyz);
     template<typename NodeType> const NodeType* probeConstNode(const Coord& xyz) const;
-    template<typename NodeType> const NodeType* probeNode(const Coord& xyz) const;
+    template<typename NodeType> const NodeType* probeNode(const Coord& xyz) const { return this->probeConstNode<NodeType>(xyz); }
     //@}
 
     //@{
     /// @brief Return a pointer to the child node that contains voxel (x, y, z).
     /// If no such node exists, return nullptr.
     ChildNodeType* probeChild(const Coord& xyz);
     const ChildNodeType* probeConstChild(const Coord& xyz) const;
-    const ChildNodeType* probeChild(const Coord& xyz) const;
+    const ChildNodeType* probeChild(const Coord& xyz) const { return this->probeConstChild(xyz); }
     //@}
 
     //@{
     /// @brief Return a pointer to the child node that contains voxel (x, y, z).
     /// If no such node exists, return nullptr.
     ChildNodeType* probeChild(const Coord& xyz, ValueType& value, bool& active);
     const ChildNodeType* probeConstChild(const Coord& xyz, ValueType& value, bool& active) const;
-    const ChildNodeType* probeChild(const Coord& xyz, ValueType& value, bool& active) const;
+    const ChildNodeType* probeChild(const Coord& xyz, ValueType& value, bool& active) const  { return this->probeConstChild(xyz, value, active); }
     //@}
 
     //@{
     /// @brief Return a pointer to the child node for a specific offset.
     /// If no such node exists, return nullptr.
+    /// @warning This method should only be used by experts seeking low-level optimizations.
     /// @note Out-of-bounds memory access attempts will wrap around using modulo indexing.
-    ChildNodeType* probeChild(Index offset);
-    const ChildNodeType* probeConstChild(Index offset) const;
-    const ChildNodeType* probeChild(Index offset) const;
+    ChildNodeType* probeChildUnsafe(Index offset);
+    const ChildNodeType* probeConstChildUnsafe(Index offset) const;
+    const ChildNodeType* probeChildUnsafe(Index offset) const { return this->probeConstChildUnsafe(offset); }
     //@}
 
     //@{
     /// @brief Return a pointer to the child node for a specific offset.
     /// If no such node exists, return nullptr.
+    /// @warning This method should only be used by experts seeking low-level optimizations.
     /// @note Out-of-bounds memory access attempts will wrap around using modulo indexing.
-    ChildNodeType* probeChild(Index offset, ValueType& value, bool& active);
-    const ChildNodeType* probeConstChild(Index offset, ValueType& value, bool& active) const;
-    const ChildNodeType* probeChild(Index offset, ValueType& value, bool& active) const;
+    ChildNodeType* probeChildUnsafe(Index offset, ValueType& value, bool& active);
+    const ChildNodeType* probeConstChildUnsafe(Index offset, ValueType& value, bool& active) const;
+    const ChildNodeType* probeChildUnsafe(Index offset, ValueType& value, bool& active) const { return this->probeConstChildUnsafe(offset, value, active); }
     //@}
 
     //@{
@@ -1261,15 +1263,6 @@ InternalNode<ChildT, Log2Dim>::probeConstNode(const Coord& xyz) const
 }
 
 
-template<typename ChildT, Index Log2Dim>
-template<typename NodeT>
-inline const NodeT*
-InternalNode<ChildT, Log2Dim>::probeNode(const Coord& xyz) const
-{
-    return this->probeConstNode<NodeT>(xyz);
-}
-
-
 template<typename ChildT, Index Log2Dim>
 template<typename NodeT, typename AccessorT>
 inline const NodeT*
@@ -1297,80 +1290,56 @@ inline ChildT*
 InternalNode<ChildT, Log2Dim>::probeChild(const Coord& xyz)
 {
     const Index n = this->coordToOffset(xyz);
-    return this->probeChild(n);
+    return this->probeChildUnsafe(n);
 }
 
 template<typename ChildT, Index Log2Dim>
 inline const ChildT*
 InternalNode<ChildT, Log2Dim>::probeConstChild(const Coord& xyz) const
 {
     const Index n = this->coordToOffset(xyz);
-    return this->probeConstChild(n);
-}
-
-template<typename ChildT, Index Log2Dim>
-inline const ChildT*
-InternalNode<ChildT, Log2Dim>::probeChild(const Coord& xyz) const
-{
-    return this->probeConstChild(xyz);
+    return this->probeConstChildUnsafe(n);
 }
 
 template<typename ChildT, Index Log2Dim>
 inline ChildT*
 InternalNode<ChildT, Log2Dim>::probeChild(const Coord& xyz, ValueType& value, bool& active)
 {
     const Index n = this->coordToOffset(xyz);
-    return this->probeChild(n, value, active);
+    return this->probeChildUnsafe(n, value, active);
 }
 
 template<typename ChildT, Index Log2Dim>
 inline const ChildT*
 InternalNode<ChildT, Log2Dim>::probeConstChild(const Coord& xyz, ValueType& value, bool& active) const
 {
     const Index n = this->coordToOffset(xyz);
-    return this->probeConstChild(n, value, active);
-}
-
-template<typename ChildT, Index Log2Dim>
-inline const ChildT*
-InternalNode<ChildT, Log2Dim>::probeChild(const Coord& xyz, ValueType& value, bool& active) const
-{
-    return this->probeConstChild(xyz, value, active);
+    return this->probeConstChildUnsafe(n, value, active);
 }
 
 template<typename ChildT, Index Log2Dim>
 inline ChildT*
-InternalNode<ChildT, Log2Dim>::probeChild(Index offset)
+InternalNode<ChildT, Log2Dim>::probeChildUnsafe(Index offset)
 {
     OPENVDB_ASSERT(offset < NUM_VALUES);
-    offset &= (NUM_VALUES - 1); // prevent use of an out-of-bounds index
     if (mChildMask.isOn(offset))    return mNodes[offset].getChild();
     return nullptr;
 }
 
 template<typename ChildT, Index Log2Dim>
 inline const ChildT*
-InternalNode<ChildT, Log2Dim>::probeConstChild(Index offset) const
+InternalNode<ChildT, Log2Dim>::probeConstChildUnsafe(Index offset) const
 {
     OPENVDB_ASSERT(offset < NUM_VALUES);
-    offset &= (NUM_VALUES - 1); // prevent use of an out-of-bounds index
     if (mChildMask.isOn(offset))    return mNodes[offset].getChild();
     return nullptr;
 }
 
-template<typename ChildT, Index Log2Dim>
-inline const ChildT*
-InternalNode<ChildT, Log2Dim>::probeChild(Index offset) const
-{
-    return this->probeConstChild(offset);
-}
-
 template<typename ChildT, Index Log2Dim>
 inline ChildT*
-InternalNode<ChildT, Log2Dim>::probeChild(Index offset, ValueType& value, bool& active)
+InternalNode<ChildT, Log2Dim>::probeChildUnsafe(Index offset, ValueType& value, bool& active)
 {
     OPENVDB_ASSERT(offset < NUM_VALUES);
-    offset &= (NUM_VALUES - 1); // prevent use of an out-of-bounds index
     if (mChildMask.isOn(offset))    return mNodes[offset].getChild();
     value = mNodes[offset].getValue();
     active = mValueMask.isOn(offset);
@@ -1379,22 +1348,15 @@ InternalNode<ChildT, Log2Dim>::probeChild(Index offset, ValueType& value, bool&
 
 template<typename ChildT, Index Log2Dim>
 inline const ChildT*
-InternalNode<ChildT, Log2Dim>::probeConstChild(Index offset, ValueType& value, bool& active) const
+InternalNode<ChildT, Log2Dim>::probeConstChildUnsafe(Index offset, ValueType& value, bool& active) const
 {
     OPENVDB_ASSERT(offset < NUM_VALUES);
-    offset &= (NUM_VALUES - 1); // prevent use of an out-of-bounds index
     if (mChildMask.isOn(offset))    return mNodes[offset].getChild();
     value = mNodes[offset].getValue();
     active = mValueMask.isOn(offset);
     return nullptr;
 }
 
-template<typename ChildT, Index Log2Dim>
-inline const ChildT*
-InternalNode<ChildT, Log2Dim>::probeChild(Index offset, ValueType& value, bool& active) const
-{
-    return this->probeConstChild(offset, value, active);
-}
 
 ////////////////////////////////////////
 

diff --git a/openvdb/openvdb/tree/RootNode.h b/openvdb/openvdb/tree/RootNode.h
@@ -735,7 +735,7 @@ class RootNode
     /// return @c nullptr.
     bool probe(const Coord& xyz, ChildNodeType*& child, ValueType& value, bool& active);
     bool probeConst(const Coord& xyz, const ChildNodeType*& child, ValueType& value, bool& active) const;
-    bool probe(const Coord& xyz, const ChildNodeType*& child, ValueType& value, bool& active) const;
+    bool probe(const Coord& xyz, const ChildNodeType*& child, ValueType& value, bool& active) const { return this->probeConst(xyz, child, value, active); }
     //}
 
     //@{
@@ -752,15 +752,15 @@ class RootNode
     /// If no such node exists, return @c nullptr.
     ChildNodeType* probeChild(const Coord& xyz);
     const ChildNodeType* probeConstChild(const Coord& xyz) const;
-    const ChildNodeType* probeChild(const Coord& xyz) const;
+    const ChildNodeType* probeChild(const Coord& xyz) const { return this->probeConstChild(xyz); }
     //@}
 
     //@{
     /// @brief Return a pointer to the leaf node that contains voxel (x, y, z).
     /// If no such node exists, return @c nullptr.
     LeafNodeType* probeLeaf(const Coord& xyz);
     const LeafNodeType* probeConstLeaf(const Coord& xyz) const;
-    const LeafNodeType* probeLeaf(const Coord& xyz) const;
+    const LeafNodeType* probeLeaf(const Coord& xyz) const { return this->probeConstLeaf(xyz); }
     //@}
 
     //@{
@@ -2872,14 +2872,6 @@ RootNode<ChildT>::probeConst(const Coord& xyz, const ChildNodeType*& child, Valu
 }
 
 
-template<typename ChildT>
-inline bool
-RootNode<ChildT>::probe(const Coord& xyz, const ChildNodeType*& child, ValueType& value, bool& active) const
-{
-    return this->probeConst(xyz, child, value, active);
-}
-
-
 template<typename ChildT>
 inline ChildT*
 RootNode<ChildT>::probeChild(const Coord& xyz)
@@ -2888,14 +2880,6 @@ RootNode<ChildT>::probeChild(const Coord& xyz)
 }
 
 
-template<typename ChildT>
-inline const ChildT*
-RootNode<ChildT>::probeChild(const Coord& xyz) const
-{
-    return this->template probeConstNode<ChildT>(xyz);
-}
-
-
 template<typename ChildT>
 inline const ChildT*
 RootNode<ChildT>::probeConstChild(const Coord& xyz) const

diff --git a/openvdb/openvdb/unittest/TestInternalNode.cc b/openvdb/openvdb/unittest/TestInternalNode.cc
@@ -151,7 +151,7 @@ TEST_F(TestInternalNode, testProbe)
         EXPECT_FALSE(bool(node6));
     }
 
-    { // probeChild, probeConstChild - coord access
+    { // probeChild, probeConstChild
         auto* node1 = internalNode.probeChild(Coord(0, 256, 4096));
         EXPECT_TRUE(bool(node1));
         auto* node2 = internalNode.probeChild(Coord(0, 128, 4096));
@@ -167,32 +167,26 @@ TEST_F(TestInternalNode, testProbe)
         EXPECT_FALSE(bool(node6));
     }
 
-    { // probeChild, probeConstChild - index access
-        auto* node1 = internalNode.probeChild(64);
+    { // probeChildUnsafe, probeConstChildUnsafe
+        auto* node1 = internalNode.probeChildUnsafe(64);
         EXPECT_TRUE(bool(node1));
-        auto* node2 = internalNode.probeChild(33);
+        auto* node2 = internalNode.probeChildUnsafe(33);
         EXPECT_FALSE(bool(node2));
         const InternalNode& constInternalNode = internalNode;
-        auto* node3 = constInternalNode.probeChild(64);
+        auto* node3 = constInternalNode.probeChildUnsafe(64);
         EXPECT_TRUE(bool(node3));
-        auto* node4 = constInternalNode.probeChild(33);
+        auto* node4 = constInternalNode.probeChildUnsafe(33);
         EXPECT_FALSE(bool(node4));
-        auto* node5 = internalNode.probeConstChild(64);
+        auto* node5 = internalNode.probeConstChildUnsafe(64);
         EXPECT_TRUE(bool(node5));
-        auto* node6 = internalNode.probeConstChild(33);
+        auto* node6 = internalNode.probeConstChildUnsafe(33);
         EXPECT_FALSE(bool(node6));
-
-        // wrap-around modulo indexing
-        auto* node7 = internalNode.probeConstChild(64 + 32*32*32);
-        EXPECT_TRUE(bool(node7));
-        auto* node8 = internalNode.probeConstChild(33 + 32*32*32);
-        EXPECT_FALSE(bool(node8));
     }
 
     float value = -1.0f;
     bool active = false;
 
-    { // probeChild, probeConstChild - coord access with value and active status
+    { // probeChild, probeConstChild with value and active status
         auto* node1 = internalNode.probeChild(Coord(0, 256, 4096), value, active);
         EXPECT_TRUE(bool(node1));
         EXPECT_EQ(value, -1.0f);
@@ -220,41 +214,31 @@ TEST_F(TestInternalNode, testProbe)
         EXPECT_TRUE(active); active = false;
     }
 
-    { // probeChild, probeConstChild - index access with value and active status
-        auto* node1 = internalNode.probeChild(64, value, active);
+    { // probeChildUnsafe, probeConstChildUnsafe with value and active status
+        auto* node1 = internalNode.probeChildUnsafe(64, value, active);
         EXPECT_TRUE(bool(node1));
         EXPECT_EQ(value, -1.0f);
         EXPECT_FALSE(active);
-        auto* node2 = internalNode.probeChild(33, value, active);
+        auto* node2 = internalNode.probeChildUnsafe(33, value, active);
         EXPECT_FALSE(bool(node2));
         EXPECT_EQ(value, 4.0f); value = -1.0f;
         EXPECT_TRUE(active); active = false;
         const InternalNode& constInternalNode = internalNode;
-        auto* node3 = constInternalNode.probeChild(64, value, active);
+        auto* node3 = constInternalNode.probeChildUnsafe(64, value, active);
         EXPECT_TRUE(bool(node3));
         EXPECT_EQ(value, -1.0f);
         EXPECT_FALSE(active);
-        auto* node4 = constInternalNode.probeChild(33, value, active);
+        auto* node4 = constInternalNode.probeChildUnsafe(33, value, active);
         EXPECT_FALSE(bool(node4));
         EXPECT_EQ(value, 4.0f); value = -1.0f;
         EXPECT_TRUE(active); active = false;
-        auto* node5 = internalNode.probeConstChild(64, value, active);
+        auto* node5 = internalNode.probeConstChildUnsafe(64, value, active);
         EXPECT_TRUE(bool(node5));
         EXPECT_EQ(value, -1.0f);
         EXPECT_FALSE(active);
-        auto* node6 = internalNode.probeConstChild(33, value, active);
+        auto* node6 = internalNode.probeConstChildUnsafe(33, value, active);
         EXPECT_FALSE(bool(node6));
         EXPECT_EQ(value, 4.0f); value = -1.0f;
         EXPECT_TRUE(active); active = false;
-
-        // wrap-around modulo indexing
-        auto* node7 = internalNode.probeConstChild(64 + 32*32*32, value, active);
-        EXPECT_TRUE(bool(node7));
-        EXPECT_EQ(value, -1.0f);
-        EXPECT_FALSE(active);
-        auto* node8 = internalNode.probeConstChild(33 + 32*32*32, value, active);
-        EXPECT_FALSE(bool(node8));
-        EXPECT_EQ(value, 4.0f); value = -1.0f;
-        EXPECT_TRUE(active); active = false;
     }
 }