Page MenuHome

Curves: Port curve to mesh node to the new data-block
ClosedPublic

Authored by Hans Goudey (HooglyBoogly) on Apr 14 2022, 12:40 AM.

Details

Summary

This commit changes the Curve to Mesh node to work with Curves
instead of CurveEval. The change ends up basically completely
rewriting the node, since the different attribute storage means that
the decisions made previously don't make much sense anymore.

The main loops are now "for each attribute: for each curve combination"
rather than the other way around, with the goal of taking advantage
of the locality of curve attributes. This improvement is quite noticeable
with many small curves; I measured a 4-5x improvement (around 4-5s
to <1s) when converting millions of curves to tens of millions of faces.
I didn't obverse any change in performance compared to 3.1 with fewer
curves though.

The changes also solve an algorithmic flaw where any interpolated
attributes would be evaluated for every curve combination instead
of just once per curve. This can be a large improvement when there
are many profile curves.

The code relies heavily on a function called foreach_curve_combination
which calculates some basic information about each combination and
calls a templated function. I made some assumptions there about
unnecessary information gathering being removed with compiler optimizations.
For further performance improvements in the future that might be
an area to investigate. Another might be using a "for a group of curves:
for each attribute: for each curve" pattern to increase the locality of
memory access.

Diff Detail

Repository
rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Apr 14 2022, 12:40 AM
Hans Goudey (HooglyBoogly) created this revision.
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)
Jacques Lucke (JacquesLucke) requested changes to this revision.Apr 14 2022, 12:16 PM

The flower shop scene seems broken now. It crashes because it runs out of memory for some reason.

source/blender/blenkernel/intern/curve_to_mesh_convert.cc
41–57

edge -> segment?

187–194

Isn't this essentially single threaded when only one curve with one profile is used? For testing I added a couple of threading::parallel_for and it resulted in a fairly significant speedup.

332

Why is this POINT and not the domain of the builtin attribute on the destination?

383

Why is this an int instead of bool?

694

This looks like a potential use-after-free, because radii may point to memory owned by orig_radii, which is freed the next line.

This revision now requires changes to proceed.Apr 14 2022, 12:16 PM
Hans Goudey (HooglyBoogly) marked 4 inline comments as done.Apr 14 2022, 7:48 PM

I'm getting a heap-buffer-overflow with ASAN in the flower shop scene in master, I'll look into that first.

source/blender/blenkernel/intern/curve_to_mesh_convert.cc
332

In this case the mesh attribute is *not* built in, so we just use the point domain as a default.
It's the last case (the final return statement) when the mesh attribute is built in and has to be transferred to a specific domain.

However I think returning "auto" when the attribute meta data can't be retrieved from the mesh is wrong, I changed that.

Hans Goudey (HooglyBoogly) marked an inline comment as done.
  • Use bool instead of int
  • Rename edge to segment
  • Fix potential use after free
  • Move some code out of a template
  • Fix getting result attribute domain from mesh

The issue with the flower shop scene is fixed with D14655.

For single curves it's still single threaded, right?

This revision is now accepted and ready to land.Apr 15 2022, 8:56 AM

For single curves it's still single threaded, right?

Yes, I'd like to do that in a separate commit.