Geometry Nodes: Port set handle nodes to new data-block
ClosedPublic
Actions

Authored by Hans Goudey (HooglyBoogly) on Mar 26 2022, 12:44 AM.

Details

Reviewers

Jacques Lucke (JacquesLucke)

Commits

rB00ba51d37bf5: Geometry Nodes: Port set handle nodes to new data-block

Summary

The nodes get simpler and likely much faster too, though they're
not usually a bottleneck. Most of the code comes from porting
the internal handle calculation and adjustment code from BezierSpline.

I still haven't decided whether to use lazy calculation of vector and
auto Bezier handles or not. I could see how deferring calculation until
it's actually needed is nice. But maybe it's simpler to calculate all of
it whenever changing handles or handle types.

Diff Detail

Repository: rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Mar 26 2022, 12:44 AM

Hans Goudey (HooglyBoogly) created this revision.

Hans Goudey (HooglyBoogly) mentioned this in T95443: Refactor curve nodes to use new data structure.

I still haven't decided whether to use lazy calculation of vector and auto Bezier handles or not.

If we would compute the handles lazily, that should probably be done as part of calling CurvesGeometry::handle_positions_left and CurvesGeometry::handle_positions_right. Or at least those methods should assert that the handles are up-to-date.

Having said that, there might be a strong argument for eager computation of handle positions: Is there any use case where curves are modified, but we don't have to update handle positions eventually? I can't think of a use case. This is different from e.g. lazily computing normals, because normals are not always required.
If they are always needed anyway, we can also just update them immediately and benefit from the fact that the data is already in cache and that we know exactly which handles have to be updated.

source/blender/blenkernel/intern/curve_bezier.cc
61	This doesn't work when there are aligned handles, does it? Not sure how a mix of aligned and auto handles works.
146	Add a comment that code should first set handle types before setting handle positions to avoid unexpected results.
source/blender/nodes/geometry/nodes/node_geo_curve_set_handle_type.cc
69	This is necessary here, because currently auto-handles are lazily evaluated?
115	Not sure how this message would help the user. Also note that this would show even if the attributes exists on some instance but not on others.

Jacques Lucke (JacquesLucke) requested changes to this revision.Mar 27 2022, 1:39 PM

This revision now requires changes to proceed.Mar 27 2022, 1:39 PM

If we would compute the handles lazily, that should probably be done as part of calling CurvesGeometry::handle_positions_left and CurvesGeometry::handle_positions_right. Or at least those methods should assert that the handles are up-to-date.

Yeah, if we went that way I would probably basically copy the logic in BezierSpline.

Is there any use case where curves are modified, but we don't have to update handle positions eventually?

Many of the possible use cases could be countered with "just use a poly curve instead", but I think one valid one is using multiple set position nodes before finally evaluating the curve.
The set position node modifies the handles, but the auto and vector handles would still have to be recalculated every time if this happened eagerly.

we can also just update them immediately and benefit from the fact that the data is already in cache

I think the cache might be used well in both cases, if we calculate the auto handles right before evaluating the curve.

What do you think, based on that use case? Maybe eagerly is still fine, for now anyway? It's probably relatively easy to switch to lazy calculation later if it's helpful.

source/blender/blenkernel/intern/curve_bezier.cc
61	Yeah, I don't think it's meant to deal with that, since the "Set Handle Positions" node (and edit mode transform) are meant to handle that case.
source/blender/nodes/geometry/nodes/node_geo_curve_set_handle_type.cc
69	Yeah, if we go with eager calculation I'll remove this (I'll add a comment in case we go with lazy calculation).
115	Fair enough. I wanted to avoid actually checking the curve types for a Bezier curve, but I didn't want to lie and say that that means there were no Bezier curves at all. I adjusted it a bit to still be clear but hopefully more helpful.

Fixes, cleanup

The set position node modifies the handles, but the auto and vector handles would still have to be recalculated every time if this happened eagerly.

True, but on the other hand, the Set Position node could more easily only update the handles that need to be updated (when using a selection). It would be very costly to recompute all handles if just a single point is moved.
The best solution would be hybrid solution of course, where we sometimes do eager and sometimes lazy computation, based on some heuristic. To me that complexity is not worth it right now and eager computation is good enough.

source/blender/blenkernel/intern/curve_bezier.cc
61	What's the fundamental difference between vector/auto and aligned handles? It seems like the difference is that vector and auto handles change depending on neighboring control points, while aligned handles only depend on the handle on the same control point. However, there seems to be one flaw in that argument: vector handles have to be updated before aligned handles. Consider the case below. Here it is impossible to update aligned handles before vector handles are updated when the point at the bottom is moved.

This revision now requires changes to proceed.Mar 30 2022, 11:50 AM

Okay, we can use eager calculation for now, I'm fine with that. It's not trivial to update only affected handles; the initial selection on the curve points has to be localized to each curve and expanded by one. So I think I'll save that for the future.

In this revision I've improved on the handling of handle types in the set handle position node, and extended the auto handle calculation to support aligned handles.

Hans Goudey (HooglyBoogly) updated this revision to Diff 49935.Mar 31 2022, 12:40 AM

Don't have to look at it again.

source/blender/blenkernel/BKE_curves.hh
402	This comment probably has to be updated now, because aligned handles are handled as well.
source/blender/blenkernel/intern/curve_bezier.cc
61	The `adjust` in the name seems misleading, because this is a pure function (without side effects).
source/blender/nodes/geometry/nodes/node_geo_curve_set_handle_type.cc
69	Guess this can be removed now then? Or moved to the end of the function.
source/blender/nodes/geometry/nodes/node_geo_set_curve_handles.cc
124	For eager computation, should this also be moved down?
source/blender/nodes/geometry/nodes/node_geo_set_position.cc
96	Is `automatical` a word? Better do this after calling `save`.

This revision is now accepted and ready to land.Mar 31 2022, 11:26 AM

Hans Goudey (HooglyBoogly) marked 7 inline comments as done.Apr 1 2022, 6:43 AM

Hans Goudey (HooglyBoogly) added inline comments.

source/blender/blenkernel/intern/curve_bezier.cc
61	Good point. Naming is a bit odd when it depends on its initial value. I went with `calculate_aligned_handle`
source/blender/nodes/geometry/nodes/node_geo_curve_set_handle_type.cc
69	Right, thanks. I think it's needed if the new handle type is auto, vector, or aligned.