Page MenuHome
Differential D14284

Curves: Initial evaluation for curves data-block
ClosedPublic
Actions

Authored by Hans Goudey (HooglyBoogly) on Mar 8 2022, 11:24 PM.

Details

Reviewers
Jacques Lucke (JacquesLucke)
Maniphest Tasks
T95942: Add evaluation to new curves data structure
Commits
rB8538c6992166: Curves: Initial evaluation for curves data-block
Summary

This patch adds evaluation for NURBS, Bezier, and Catmull Rom
curves for the new Curves data-block. The main difference from
the code in BKE_spline.hh is that the functionality is not
encapsulated in classes. Instead, each function has arguments
for all of the information it needs. This makes the code more
reusable and removes a bunch of unnecessary complications
for keeping track of state.

NURBS and Bezier evaluation work the same way as existing code.
The Catmull Rom implementation is new with the basis function,
based on Cycles code. I added some basic tests for it.

For NURBS and Catmull Romcurves, evaluating positions is the same
as any generic attribute, so it's implemented by the generic interpolation
to evaluated points. Bezier curves are a bit special, because the "handle"
control points are stored in a separate attribute. This patch doesn't
include generic interpolation to evaluated points for Bezier curves.

Ref T95942

Diff Detail

Repository
rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Mar 8 2022, 11:24 PM
Hans Goudey (HooglyBoogly) created this revision.
Hans Goudey (HooglyBoogly) updated this revision to Diff 49087.Mar 9 2022, 6:06 AM
Hans Goudey (HooglyBoogly) added a task: T95942: Add evaluation to new curves data structure.

A bit of progress and another option for the CurveRef idea

Hans Goudey (HooglyBoogly) updated this revision to Diff 49160.Mar 11 2022, 6:39 AM

More progress

Hans Goudey (HooglyBoogly) updated this revision to Diff 49233.Mar 14 2022, 5:16 AM
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)
Hans Goudey (HooglyBoogly) set the repository for this revision to rB Blender.
  • Implement initial NURBS evaluation. Evaluated positions is implemented now, not tested yet though
Hans Goudey (HooglyBoogly) updated this revision to Diff 49272.Mar 15 2022, 4:53 AM
Hans Goudey (HooglyBoogly) retitled this revision from Curves: Initial evaluation for curves data-block (WIP) to Curves: Initial evaluation for curves data-block.
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)
Hans Goudey (HooglyBoogly) added a reviewer: Jacques Lucke (JacquesLucke).
  • Finish changes, cleanup, add comments
Hans Goudey (HooglyBoogly) updated this revision to Diff 49273.Mar 15 2022, 5:11 AM

Cleanup

source/blender/blenkernel/BKE_curves.hh
29

Ideally this would be in some more general file. We can also use it more elsewhere.

I experimented with an IndexOffsets class that inherits from Span and has this as a member function. It didn't seem that great though, honestly.

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)Mar 15 2022, 5:12 AM
Jacques Lucke (JacquesLucke) requested changes to this revision.Mar 15 2022, 12:26 PM

Overall, I'm quite happy with where this is going! All the "blocking" comments should be easy to fix. The rest can be worked in separately afterwards.

source/blender/blenkernel/BKE_curves.hh
50

So that's an int per evaluated point?

64

typo (evaluated)

72

Should be fine for now, but we might want to flatten this vector of vectors out as well in the future.

154

Document which of these attributes are on the curve vs point domain.

209

unrelated changes

295

Maybe use num_points or something like that instead of size? I find that more clear.

306

index -> segment_index
Given the function below, I assume this function does not support cyclic splines?

323

I wonder if these lower level functions should support a resolution per segment, even if we don't support it at the Curves level. Passing handle types to these lower level functions feels a bit wrong. The fact that vector segments are not influenced by the resolution seems to be a user level decision, that shouldn't necessarily leak into these low level functions.

Note, that this does not have to be done now, but might be worth doing in a separate refactor.

If we decide to support per segment resolution in these low level functions, it would probably be good to have two overloads for calculate_evaluated_offsets. One that takes a single resolution and one that takes a span. That is, instead of taking a VArray (maybe there could be a third overload for that). The main reason is, that we can reasonably optimize this function for both cases.

343

missing word (first index IS the)

365

Again, personally I find size not descriptive enough.
Might be a language thing, but size in this context always makes me think of radius or length.

369

typo (evenly)

371

I think passing in the resolution separately is reasonable instead of trying to recover it from the span sizes. That also allows adding an assert to check if the spans have the expected size.

404

typo (an -> a)

source/blender/blenkernel/intern/curve_catmull_rom.cc
22

This and a couple other functions should be static.

68

What if there is a cyclic curve with only two control points? (currently this branch always evaluates three segments, even when there are only two)

source/blender/blenkernel/intern/curve_nurbs.cc
62

I'm not entirely sure, but I wonder if this function would be simpler if it just had a top-level switch statement, even if that might lead to a little bit of code duplication.

109

General question, does it make sense for knots to be floats, or should they be ints?

201

Doesn't have to change now, but I wonder if the modulo is only used for cyclic curves. If yes, it might be reasonable to provide a fast path that doesn't need the modulo.
Or the loop could be duplicated, one with and one for the remaining elements that need the modulo. In this case, is it possible to replace the modulo with - src.size()?

248

Guess you meant to call interpolate_to_evaluated_rational?

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

Better start task isolation directly after locking the mutex, in case one of the other methods uses multiple threads.

This revision now requires changes to proceed.Mar 15 2022, 12:26 PM
Hans Goudey (HooglyBoogly) updated this revision to Diff 49307.Mar 15 2022, 9:49 PM
Hans Goudey (HooglyBoogly) marked 14 inline comments as done.
  • Simplify arguments to Bezier position evaluation
  • Add a resolution argument to catmull rom evaluation
  • Fix unused variable
  • Move task isolation
  • Make functions static
  • Fix stack buffer overflow in NURBS evaluation with weights
  • Fix two point cyclic catmull rom curve add test
  • Improve function name, fix comment typos
  • Remove unrelated changes
  • Add comments to new functions in CurvesGeometry
  • Improve comment for BasisCache.weights
source/blender/blenkernel/BKE_curves.hh
72

We might want to. I'm not sure though. Before last week, NURBSpline had a vector per evaluated point, and that wasn't even much of a bottleneck.

One really nice benefit to storing separate vectors is that the cache is the same for every NURBS curve with the same knots mode, resolution, size, and cyclic value.
For use cases where many similarly-sized curves are needed, that should be a very nice performance improvement. I'd basically store this in a map, and only build a new
cache when necessary.

295

I've been using size pretty consistently to refer to array/curve sizes. I don't have a strong opinion, but I like being consistent.
I think I remember some discussion about that being the guideline for variable names in that case. I asked about that in chat.
If I'm remembering wrong I'll switch the naming more consistently in this patch.

306

The other function is just for the last segment in cyclic curves. I'll make the naming more clear.

323

I think I'll keep it is for now, since it would be relatively easy to implement when we want to expose that functionality.
I like your idea of overloading this function in that case.

For Bezier curves, in this patch it became much more clear to me that the entire concept of handles and handle types is just a UI level thing, it's not a concept of the curves at a lower level at all.
I think Blender is doing something relatively non-standard by storing the handle positions in separate arrays (and not storing attribute values for handle points.
I'm not sure Blender's choice is the right one honestly, but multiplying the number of control points in every Bezier curve by three seems like too big of a change.

404

The comment also said "Bezier" instead of "NURBS", oops.

source/blender/blenkernel/intern/curve_catmull_rom.cc
68

A cyclic curve with two points is basically just not cyclic, so it should end up in the segment_size == 1 case. I'll add a test to verify that.

source/blender/blenkernel/intern/curve_nurbs.cc
62

Yes, I think it's better that way too. It ended up like this because for Curve/Nurb, the knots mode is a bitflag. So as we remove that, it would make sense to refactor this.
Not now though.

109

The knots can be thought of as a custom parametarization of the curve. They influence where the control point influence for a particular evaluated point starts, but they also influence the actual values of the resulting basis weights. So it's important that it's floats.

Even after all this time, I'm still working on getting an intuitive understanding for it.

201

I like the idea. Once we get this implementation hooked up to some tests, I'll look into that.

248

Oops!

Hans Goudey (HooglyBoogly) updated this revision to Diff 49308.Mar 15 2022, 9:54 PM

Remove accidental formatting change

Hans Goudey (HooglyBoogly) set the repository for this revision to rB Blender.Mar 15 2022, 9:54 PM
Jacques Lucke (JacquesLucke) accepted this revision.Mar 16 2022, 12:08 PM

Might be nice to get some more test coverage, but beside that, this patch looks good to me.

source/blender/blenkernel/BKE_curves.hh
72

I see. Yes when the cache could be reused for multiple curves, then that seems reasonable.

323

Hm, I haven't seen other software that treats the handle positions the same as the control point positions. The software I've used always treats handles as being attached to the control points, so it doesn't feel non-standard. Whether or not that concept also exist at a lower level for curves is mostly not relevant. We just have to write our abstractions so that the ui level concepts don't leak into the low level functions if it can be avoided.

source/blender/blenkernel/intern/curve_catmull_rom.cc
52

These unexpected special cases feel very expected and non-special ;D

This revision is now accepted and ready to land.Mar 16 2022, 12:08 PM
Hans Goudey (HooglyBoogly) added inline comments.Mar 16 2022, 2:55 PM
source/blender/blenkernel/BKE_curves.hh
323

Yeah, I was talking mostly about the internal representation. But right, the idea of aligning the internals to the UI is also a good point.

source/blender/blenkernel/intern/curve_catmull_rom.cc
52

Good point, I'll rephrase that :P

Closed by commit rB8538c6992166: Curves: Initial evaluation for curves data-block (authored by Hans Goudey (HooglyBoogly)). · Explain WhyMar 16 2022, 9:47 PM
This revision was automatically updated to reflect the committed changes.
Hans Goudey (HooglyBoogly) added a commit: rB8538c6992166: Curves: Initial evaluation for curves data-block.

Revision Contents
Changeset List

PathSize
source/
blender/
blenkernel/
297 lines
3 lines
intern/
138 lines
114 lines
252 lines
367 lines
320 lines

Diff 49351

View Options

source/blender/blenkernel/BKE_curves.hh

Loading...
View Options

source/blender/blenkernel/CMakeLists.txt

Loading...
View Options

source/blender/blenkernel/intern/curve_bezier.cc

Loading...
View Options

source/blender/blenkernel/intern/curve_catmull_rom.cc

Loading...
View Options

source/blender/blenkernel/intern/curve_nurbs.cc

Loading...
View Options

source/blender/blenkernel/intern/curves_geometry.cc

Loading...
View Options

source/blender/blenkernel/intern/curves_geometry_test.cc

Loading...