Page MenuHome

Curves: Further implementation of new curves data structure
ClosedPublic

Authored by Hans Goudey (HooglyBoogly) on Feb 9 2022, 1:40 AM.

Details

Summary

The general idea here is to wrap the CurvesGeometry DNA struct with
a C++ class that can do most of the heavy lifting for the curve geometry
in a better way. Using a C++ class allows easier ways to group methods,
easier const correctness, and code that's more readable and faster to write.

This way, it works much more like a version of CurveEval that uses more
efficient attribute storage. I think that's a good thing, because other than
that, I still agree with most of the design decisions we made when building
that and the Spline classes.

This commit adds the structure of some yet-to-be-implemented code,
the largest thing being mutexes and vectors meant to hold lazily
calculated evaluated positions, tangents, and normals. That part might
change slightly, but it's helpful to be able to see the direction this patch
is aiming in. In particular, the inherently single-threaded accumulated
lengths and Bezier evaluated point offsets might be cached.

The next steps after this patch:

  • Implement conversion to and from CurveEval
  • Implement catmull-rom evaluation for CurvesGeometry and CurveEval
  • Implement Bezier, NURBS, and Poly evaluation for CurvesGeometry

Diff Detail

Repository
rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Feb 9 2022, 1:40 AM
Hans Goudey (HooglyBoogly) created this revision.
Hans Goudey (HooglyBoogly) planned changes to this revision.Feb 10 2022, 1:08 AM
Hans Goudey (HooglyBoogly) updated this revision to Diff 48161.
Hans Goudey (HooglyBoogly) retitled this revision from Curves: Further implementation of new curves data structure (WIP) to Curves: Further implementation of new curves data structure.
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)

I still want to look through this again and add comments to the class.

source/blender/blenkernel/intern/curves.cc
202

Not introduced by this commit, but this should use curve_size.

source/blender/blenkernel/intern/curves.cc
202

Yikes, good catch. Thanks.

Hans Goudey (HooglyBoogly) marked an inline comment as done.
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)
Hans Goudey (HooglyBoogly) set the repository for this revision to rB Blender.
  • Cleanup
  • Comments

Correct diff (don't include BLI_bounds.hh change here)

Jacques Lucke (JacquesLucke) requested changes to this revision.Feb 11 2022, 2:59 PM
Jacques Lucke (JacquesLucke) added inline comments.
source/blender/blenkernel/BKE_curves.h
94

Public members generally do not have a trailing underscore.

150

This should probably just return a MutableSpan

185

Maybe start being more explicit here already, similar to what I did in BKE_node_tree_update.h. Internally it might still just mark the entire cache invalid.

  • tag_positions_changed
  • tag_topology_changed
201

Think it would be better to use a more conventional api for this for now, similar to how new meshes are created.

source/blender/blenkernel/intern/curves_geometry.cc
85

This implementation weird.

  • It makes a shallow copy.
  • The dna_struct isn't const.

I'd just get rid of this a rely on type casting instead.

This revision now requires changes to proceed.Feb 11 2022, 2:59 PM
Hans Goudey (HooglyBoogly) marked 5 inline comments as done.
  • Add wrap functions, move assign operator
  • Use more standard naming for function
  • Split up cache invalidation method
  • Return mutable span curve types attribute
  • Don't use underscores for public members
  • Merge branch 'bli-bounds' into curves-cpp-implementation
source/blender/blenkernel/BKE_curves.h
185

Good idea, I did that now.

source/blender/blenkernel/intern/curves_geometry.cc
85

Good point. I replaced this with a wrap method that works a lot better.

Jacques Lucke (JacquesLucke) requested changes to this revision.Feb 14 2022, 10:36 AM
Jacques Lucke (JacquesLucke) added inline comments.
source/blender/blenkernel/BKE_curves.h
93

What's that?

99

Remove //, same below.

167

Mention what the return value means.

185

Did you plan to use that somewhere very soon? If not, it seems like something that can be added when it becomes necessary.

191

A comment should mention what happens to existing attributes.

source/blender/blenkernel/intern/curves.cc
94

One shouldn't move-assign non-trivial types to uninitialized memory.
Instead consider using the placement-new operator to construct curves->geometry.
new (&curves->geometry) blender::bke::CurvesGeometry();

109

typo (beause)

331

Does bounds_min_max take the original value of min and max into account?
If yes, that should be mentioned in a comment on bounds_min_max.
If no, then the parameters should be renamed to`r_min` and r_max.

source/blender/blenkernel/intern/curves_geometry.cc
33

Remove this.

51

Maybe don't hardcode the names of built-in attributes, but store their names in static variable at the top of the file instead.

90

Do we need move assignment for this? There also isn't a move constructor which seems fine.

Also, the implementation is quite problematic. It should leave other in a moved-from state, not in a destructed state.

This revision now requires changes to proceed.Feb 14 2022, 10:36 AM
Hans Goudey (HooglyBoogly) marked 11 inline comments as done.Feb 14 2022, 6:36 PM
Hans Goudey (HooglyBoogly) added inline comments.
source/blender/blenkernel/BKE_curves.h
93

Leftovers from debugging, sorry about that.

185

It's useful to detect whether an attribute should exist (which I expect will be necessarily pretty soon), I can remove it for now though.

191

In the comment I mentioned that new values should be initialized afterwards, since I don't want to rely on the recently changed behavior of CustomData_realloc here

source/blender/blenkernel/intern/curves.cc
94

Aha, I forgot about that, thanks, that's helpful.

source/blender/blenkernel/intern/curves_geometry.cc
51

I'm not really convinced that's an improvement, but I'm also fine with it. We've been hard-coding the name of built-in attributes everywhere though. Should we change that more generally?

90

I read a bit more about this, and this makes sense. I guess the fix would be to just remove other.~CurvesGeometry();.

Generally I like having the move assign/move construct functions, since it can avoid overhead and sometimes it's nice semantically. In the spirit of keeping this patch simple though, I've removed it for now.

Hans Goudey (HooglyBoogly) marked 6 inline comments as done.
  • Remove move assign operator for now
  • Use static variables for attribute names
  • Fix bad section end comment
  • Further improve bounds_min_max comment
  • Fix typo, improve comment
  • Add comment for resize
  • Merge branch 'blender-v3.1-release'
  • Fix T95778, the macOS minimum versions have been increased for Metal.
  • Remove has_curve_with_type for now
  • Cleanup, improve comment for bounds function
  • Merge branch 'master' into curves-cpp-implementation
  • Cleaunp: Modify comment
source/blender/blenkernel/intern/curves_geometry.cc
51

Think it's fine to just change it here for now. Similar to POINTCLOUD_ATTR_POSITION.

Jacques Lucke (JacquesLucke) added inline comments.
source/blender/blenkernel/intern/curves_geometry.cc
73

Maybe leave a comment mentioning that copy_curves_geometry is responsible for clearing the old state of dst as well.

This revision is now accepted and ready to land.Feb 14 2022, 7:22 PM

This seems generally fine. I think @Jacques Lucke (JacquesLucke) reviewing this in detail it's enough.

source/blender/blenkernel/BKE_curves.h
68

Maybe it makes sense to move this to a BKE_curves.hh that includes BKE_curves.h?