Page MenuHome

Geometry Nodes: Curve Connect Splines Node
Needs RevisionPublic

Authored by Johnny Matthews (guitargeek) on Aug 10 2021, 3:22 AM.

Details

Summary

This is the first pass at a node to connect separate splines by either order or a distance threshold.

Diff Detail

Repository
rB Blender

Event Timeline

Johnny Matthews (guitargeek) requested review of this revision.Aug 10 2021, 3:22 AM
Johnny Matthews (guitargeek) created this revision.

When 2 curves have the same named attribute, do not duplicate the names.

Hans Goudey (HooglyBoogly) requested changes to this revision.Aug 10 2021, 6:14 AM

Generally I like the approach. Like you mentioned, a fair amount of cleanup left though. Comments inline.

source/blender/nodes/geometry/nodes/node_geo_curve_connect.cc
115

Be careful passing Vector and Array types by value. Since they own their memory, they have to copy all of it in these cases, which might be quite expensive.

That's what Span and Mutable span are for-- to give a view into memory that isn't owned locally.

132

Unused variable

133

Calling this splines is a bit confusing, since that's often the name for something like Span<SplinePtr> splines = curve.splines();

Also, I don't think you to manually initialize it here.

186

You can use .last() instead of end_index

275

Vector<GeometrySet> & -> MutableSpan<GeometrySet>

280–322

Couldn't this part be shared between the two methods?

400

I doubt this copy is necessary. At least ideally it wouldn't be. const is also somewhat misleading there, since you adjust the data there below. Sometimes which symbol const applies to can be confusing.

Some fussing, this might be a solution:

template<typename T> static void reverse_data(Span<T> src, MutableSpan<T> dst)
{
  for (const int i : dst.index_range()) {
    dst[size - 1 - i] = src[i];
  }
}

    const Spline &src_spline = *src_curve->splines()[merge_map[i].source_spline];
    const int src_size = src_spline.size();

    if (merge_map[i].reverse) {
      reverse_data(src_spline.positions(), dst->positions().slice(dst_point, src_size));
      reverse_data(src_spline.radii(), dst->radii().slice(dst_point, src_size));
      reverse_data(src_spline.tilts(), dst->tilts().slice(dst_point, src_size));
    }
    else {
      dst->positions().slice(dst_point, src_size).copy_from(src_spline.positions());
      dst->radii().slice(dst_point, src_size).copy_from(src_spline.radii());
      dst->tilts().slice(dst_point, src_size).copy_from(src_spline.tilts());
    }

    etc...
448

Unfortunately this attribute copying code will need to deal with different attribute types and domains on the same names between the input splines, which is a bit of a pain. I'd recommend dealing with that last.

501–502

Looks like these aren't used.

This revision now requires changes to proceed.Aug 10 2021, 6:14 AM
Johnny Matthews (guitargeek) marked 7 inline comments as done.
Johnny Matthews (guitargeek) retitled this revision from Geometry Nodes: Curve Connect Node to Geometry Nodes: Curve Connect Splines Node.

Refactoring and updates based on @Hans Goudey (HooglyBoogly) comments

Move MutableSpan reversing functions to the MutableSpan class
cleanup the reverse node for new reverse functions
cleanup connect node for reverse functions.

Hans Goudey (HooglyBoogly) requested changes to this revision.Aug 12 2021, 7:34 PM

Nice, I like the split out reverse functions. I didn't look into the attribute stuff yet, since there are enough comments here already, and I think it makes sense to have everything else totally solid before starting that.

release/scripts/startup/nodeitems_builtins.py
210–256

Unrelated formatting changes

source/blender/blenlib/BLI_span.hh
724 ↗(On Diff #40574)

No need for the newline between the comment and the definition.

737 ↗(On Diff #40574)

The template is already part of the class, no need to have it on member functions.

source/blender/nodes/geometry/nodes/node_geo_curve_connect_splines.cc
28 ↗(On Diff #40574)

In C++ the typedef is unnecessary, so it can look like this:

struct MergeMapEntry {
  int target_spline;
  int source_set;
  int source_spline;
  int length;
  int attr_start;
  bool reverse;
};

I also think these should be defined after the inputs, layout, init update functions.

37 ↗(On Diff #40574)

A more C++ style way to do this: enum class MapStatus { Okay, None, Mixed };

Then you can refer to them like MapStatus::Mixed

169 ↗(On Diff #40574)

It's simpler to call spline.size() I think.

171 ↗(On Diff #40574)

spline_list.append()

176–199 ↗(On Diff #40574)

Something I've learned from Jacques and others is that if a chunk of code seems like it needs a header comment block like /* Build the KD Tree. */, it's a good sign it could just be split into a separate function with a descriptive name like KDTree_3d *build_kdtree_from_spline_endpoints(...)

Then the code documents itself, and it's easier to read since it's more clear which parameters the tree building needs, and you can more easily get a high-level view of the whole algorithm.

186 ↗(On Diff #40574)

Generally one should be careful about calling insert on a Vector, since if the index isn't the last, it will have to move every element after the index, which can be slow.
In this case, append should work well.

202 ↗(On Diff #40574)

output_spline_number -> output_spline_index

218–255 ↗(On Diff #40574)

I'd recommend splitting this to a separate function too.

226–231 ↗(On Diff #40574)

Is this necessary? If the size of a span is 1, first() and last() should be equal.

232 ↗(On Diff #40574)

I think this has to be freed unfortunately.

I think this could be implemented with BLI_kdtree_3d_range_search_cb so that a results array wouldn't have to be allocated for every end point encountered in this loop.
The point distribute node has an example.

237 ↗(On Diff #40574)

spline_index_from_endpoint makes me a bit skeptical. Maybe it's necessary, but traversing the whole list for every result is O(n^2) behavior that we should aim not to have.

329–343 ↗(On Diff #40574)

I think this can make use of the fact that it doesn't run for mixed spline types and call spline->copy_only_settings();

420 ↗(On Diff #40574)
for (SplinePtr &spline : curve_out->splines()) {
  spline->attributes.reallocate(spline->size());
}
465 ↗(On Diff #40574)

The value of point_count is assigned but not used, it could just be removed.

This revision now requires changes to proceed.Aug 12 2021, 7:34 PM
Johnny Matthews (guitargeek) marked 16 inline comments as done.

Made requested changes (hopefully)

A couple more comments from me.

I'm not exactly sure, but I think when I make the threshold mode very large, I would expect the the connections it chooses to be sorted by the distance between the endpoints.


Sorry, I had trouble drawing the annotations at the right depth!

/home/hans/Blender-Git/blender/source/blender/nodes/geometry/nodes/node_geo_curve_connect_splines.cc: In function ‘blender::nodes::MapStatus blender::nodes::output_type(blender::Span<GeometrySet>&, Spline::Type&)’:
/home/hans/Blender-Git/blender/source/blender/nodes/geometry/nodes/node_geo_curve_connect_splines.cc:124:9: warning: unused variable ‘attr_index’ [-Wunused-variable]
  124 |     int attr_index = 0;
      |         ^~~~~~~~~~
/home/hans/Blender-Git/blender/source/blender/nodes/geometry/nodes/node_geo_curve_connect_splines.cc: In function ‘blender::nodes::MapStatus blender::nodes::merge_map_order(blender::Span<GeometrySet>, blender::Vector<blender::nodes::MergeMapEntry>&, Spline::Type&)’:
/home/hans/Blender-Git/blender/source/blender/nodes/geometry/nodes/node_geo_curve_connect_splines.cc:327:27: warning: unused variable ‘component’ [-Wunused-variable]
  327 |     const CurveComponent *component = set.get_component_for_read<CurveComponent>();
      |                           ^~~~~~~~~
/home/hans/Blender-Git/blender/source/blender/nodes/geometry/nodes/node_geo_curve_connect_splines.cc: In lambda function:
/home/hans/Blender-Git/blender/source/blender/nodes/geometry/nodes/node_geo_curve_connect_splines.cc:270:11: error: control reaches end of non-void function [-Werror=return-type]
  270 |           },
      |           ^
/home/hans/Blender-Git/blender/source/blender/nodes/geometry/nodes/node_geo_curve_connect_splines.cc: At global scope:
/home/hans/Blender-Git/blender/source/blender/nodes/geometry/nodes/node_geo_curve_connect_splines.cc:102:12: warning: ‘int blender::nodes::spline_index_from_endpoint(blender::Span<blender::nodes::SplineEntry>, blender::nodes::EndPointEntry)’ defined but not used [-Wunused-function]
  102 | static int spline_index_from_endpoint(Span<SplineEntry> list, EndPointEntry end)
      |            ^~~~~~~~~~~~~~~~~~~~~~~~~~
source/blender/nodes/geometry/nodes/node_geo_curve_connect_splines.cc
287 ↗(On Diff #40650)

Is there a reason to use std::map over blender::Map? If not, best to just use Blender's map, it actually has better performance.

339 ↗(On Diff #40650)

This passes the vector by value, which will copy its entire contents. best to use a Span in this case.

371 ↗(On Diff #40650)

Looks like you only use i to index into merge_map. In that case you could use for (const MergeMapEntry &merge : merge_map) {

329–343 ↗(On Diff #40574)

This can still be done, let me know if I can make it clearer.

Johnny Matthews (guitargeek) marked 3 inline comments as done.

Try this version and see what you think.

This seems to be a better solve. You also needed it to be cyclic to connect the start/end of the newly generated curve.

source/blender/nodes/geometry/nodes/node_geo_curve_connect_splines.cc
287 ↗(On Diff #40650)

I've gotten rid of the map altogether.

329–343 ↗(On Diff #40574)

Yes, but which spline do they copy? There could be several source splines to each new spline. This was just to set defaults. Unless you mean something else.

Hans Goudey (HooglyBoogly) requested changes to this revision.Sep 4 2021, 12:19 AM

Yes, the results are great, nice! It becomes very helpful for drawing, since you can start a new spline every time but still draw a continuous curve.

I noticed some inline comments marked as "Done" but not addressed. With those solved I think this would be pretty much ready.

source/blender/blenlib/BLI_span.hh
737 ↗(On Diff #40574)

This is still an issue:

/home/hans/Blender-Git/blender/source/blender/blenlib/BLI_span.hh:737:12: error: declaration of template parameter ‘T’ shadows template parameter
  737 |   template<typename T> constexpr void copy_from_reversed(Span<T> src)
      |            ^~~~~~~~
/home/hans/Blender-Git/blender/source/blender/blenlib/BLI_span.hh:470:10: note: template parameter ‘T’ declared here
  470 | template<typename T> class MutableSpan {
      |          ^~~~~~~~
/home/hans/Blender-Git/blender/source/blender/blenlib/BLI_span.hh:745:12: error: declaration of template parameter ‘T’ shadows template parameter
  745 |   template<typename T> constexpr void copy_from_reversed(MutableSpan<T> src)
      |            ^~~~~~~~
/home/hans/Blender-Git/blender/source/blender/blenlib/BLI_span.hh:470:10: note: template parameter ‘T’ declared here
  470 | template<typename T> class MutableSpan {
      |          ^~~~~~~~
source/blender/nodes/geometry/nodes/node_geo_curve_connect_splines.cc
56 ↗(On Diff #41246)

This can use PROP_DISTANCE

176–199 ↗(On Diff #40574)

Another comment here that hasn't been addressed.

232 ↗(On Diff #40574)

This is marked "Done" but I don't think it was addressed. I would suggest the BLI_kdtree_3d_range_search_cb approach since that might allow an early exit in addition to the lack of allocations.

==124164==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 13009920 byte(s) in 9240 object(s) allocated from:
    #0 0x7f6ab4f6793f in __interceptor_malloc (/lib64/libasan.so.6+0xae93f)
    #1 0x866591f in MEM_lockfree_mallocN /home/hans/Blender-Git/blender/intern/guardedalloc/intern/mallocn_lockfree_impl.c:277
    #2 0x86652c3 in MEM_lockfree_reallocN_id /home/hans/Blender-Git/blender/intern/guardedalloc/intern/mallocn_lockfree_impl.c:183
    #3 0x828cf4d in nearest_add_in_range /home/hans/Blender-Git/blender/source/blender/blenlib/intern/kdtree_impl.h:628
    #4 0x828e1fa in BLI_kdtree_3d_range_search_with_len_squared_cb /home/hans/Blender-Git/blender/source/blender/blenlib/intern/kdtree_impl.h:696
    #5 0x828e7d6 in BLI_kdtree_3d_range_search /home/hans/Blender-Git/blender/source/blender/blenlib/intern/kdtree_impl.h:731
    #6 0x3927e33 in merge_map_threshold /home/hans/Blender-Git/blender/source/blender/nodes/geometry/nodes/node_geo_curve_connect_splines.cc:233
    #7 0x3932cb7 in geo_node_curve_connect_splines_exec /home/hans/Blender-Git/blender/source/blender/nodes/geometry/nodes/node_geo_curve_connect_splines.cc:471
237 ↗(On Diff #40574)

Did you have thoughts about this? I think the code is still the same. O(n^2) algorithms can be pretty dangerous when the problem starts to scale a bit.

This revision now requires changes to proceed.Sep 4 2021, 12:19 AM
Johnny Matthews (guitargeek) marked 2 inline comments as done.
  • Still a major WIP, but made some good progress
Hans Goudey (HooglyBoogly) requested changes to this revision.Oct 12 2022, 7:50 PM

Just requesting changes so this doesn't show up as "Needs Review" in my list.

This revision now requires changes to proceed.Oct 12 2022, 7:50 PM

This node is really useful, but the concept needs to be rethought