Page MenuHome

Geometry Nodes: Field version of mesh to curve node
ClosedPublic

Authored by Hans Goudey (HooglyBoogly) on Sep 21 2021, 12:36 AM.

Details

Summary

This commit adds a fields version of the mesh to curve node, with a
field for the input selection. In order to reduce code duplication, it
adds the mesh to curve conversion to the new geometry module
and calls that implementation from both places.

More details on the geometry module can be found here: T86869

Diff Detail

Repository
rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Sep 21 2021, 12:36 AM
Hans Goudey (HooglyBoogly) created this revision.
  • Merge branch 'master' into mesh-to-curve-geometry-module
  • Cleanup, simplify slightly
  • Add newline at end of file
Jacques Lucke (JacquesLucke) requested changes to this revision.Oct 13 2021, 4:02 PM
Jacques Lucke (JacquesLucke) added inline comments.
source/blender/geometry/CMakeLists.txt
34

The indentation is wrong, same below.

source/blender/geometry/intern/mesh_to_curve_convert.cc
78

One has to be a bit careful with removing elements from the set you are iterating over. blender::Set should support that by design, but it's not part of the "specification" yet.
For blender::Map I added void remove(const BaseIterator &iterator) for this use case. It's even a bit more efficient because it avoids looking up the key again.
The same could be done for blender::Set. Maybe leave a comment about that or add the functionality to blender::Set.

You also don't want to copy over anonymous attributes that don't have strong references anymore.

This revision now requires changes to proceed.Oct 13 2021, 4:02 PM
  • Merge branch 'master' into mesh-to-curve-geometry-module
  • Add check for no strong references, fix remove in loop
  • Fix indentation in cmakelists
Hans Goudey (HooglyBoogly) marked 2 inline comments as done.Oct 14 2021, 1:02 AM
Hans Goudey (HooglyBoogly) added inline comments.
source/blender/geometry/intern/mesh_to_curve_convert.cc
78

Removing was pretty simple to add, but I wonder, it looks like a const cast is required (It isn't in the map remove function because the iterator is not internally const-correct.
I guess that's probably fine, but a separate iterator would probably need to be implemented for const sets vs mutable sets.

For now there was no real reason to use a separate loop, so I just combined them.

I also added a check for unused anonymous attributes. I'll refactor the way attributes are gathered for propagation to make this unnecessary though.

diff --git a/source/blender/blenlib/BLI_set.hh b/source/blender/blenlib/BLI_set.hh
index a8ccf957f6c..1ac6bdbbd09 100644
--- a/source/blender/blenlib/BLI_set.hh
+++ b/source/blender/blenlib/BLI_set.hh
@@ -423,6 +423,8 @@ class Set {
     int64_t total_slots_;
     int64_t current_slot_;
 
+    friend Set;
+
    public:
     Iterator(const Slot *slots, int64_t total_slots, int64_t current_slot)
         : slots_(slots), total_slots_(total_slots), current_slot_(current_slot)
@@ -467,6 +469,12 @@ class Set {
     {
       return !(a != b);
     }
+
+   protected:
+    Slot &current_slot() const
+    {
+      return slots_[current_slot_];
+    }
   };
 
   Iterator begin() const
@@ -484,6 +492,19 @@ class Set {
     return Iterator(slots_.data(), slots_.size(), slots_.size());
   }
 
+  /**
+   * Remove the key-value-pair that the iterator is currently pointing at.
+   * It is valid to call this method while iterating over the map. However, after this method has
+   * been called, the removed element must not be accessed anymore.
+   */
+  void remove(const Iterator &iterator)
+  {
+    Slot &slot = iterator.current_slot();
+    BLI_assert(slot.is_occupied());
+    slot.remove();
+    removed_slots_++;
+  }
+
   /**
    * Print common statistics like size and collision count. This is useful for debugging purposes.
    */
diff --git a/source/blender/blenlib/tests/BLI_set_test.cc b/source/blender/blenlib/tests/BLI_set_test.cc
index 3a4733a218f..b2e1f94d22c 100644
--- a/source/blender/blenlib/tests/BLI_set_test.cc
+++ b/source/blender/blenlib/tests/BLI_set_test.cc
@@ -544,6 +544,33 @@ TEST(set, GenericAlgorithms)
   EXPECT_EQ(std::count(set.begin(), set.end(), 20), 1);
 }
 
+TEST(set, RemoveDuringIteration)
+{
+  Set<int> set;
+  set.add(2);
+  set.add(5);
+  set.add(1);
+  set.add(2);
+  set.add(3);
+
+  EXPECT_EQ(set.size(), 5);
+
+  using Iter = Set<int>::Iterator;
+  Iter begin = set.begin();
+  Iter end = set.end();
+  for (Iter iter = begin; iter != end; ++iter) {
+    int item = *iter;
+    if (item == 2) {
+      set.remove(iter);
+    }
+  }
+
+  EXPECT_EQ(set.size(), 3);
+  EXPECT_TRUE(set.contains(2));
+  EXPECT_TRUE(set.contains(6));
+  EXPECT_TRUE(set.contains(3));
+}
+
 /**
  * Set this to 1 to activate the benchmark. It is disabled by default, because it prints a lot.
  */
Jacques Lucke (JacquesLucke) added inline comments.
source/blender/geometry/intern/mesh_to_curve_convert.cc
78

The comment in this patch still says map instead of set. Other than that, it looks fine.
Adding the const cast in the remove method is reasonable (it's valid because the remove method itself is not const).

Did you actually run the unit test? This part looks wrong already because you add the same value twice:

Set<int> set;
set.add(2);
set.add(5);
set.add(1);
set.add(2);
set.add(3);
EXPECT_EQ(set.size(), 5);
This revision is now accepted and ready to land.Oct 14 2021, 1:14 PM
This revision was automatically updated to reflect the committed changes.
Hans Goudey (HooglyBoogly) marked an inline comment as done.