Details

Reviewers

Jacques Lucke (JacquesLucke)
Simon Thommes (simonthommes)
Dalai Felinto (dfelinto)

Maniphest Tasks

T87429: Curve evaluation / to points for instancing

Commits

rBfcbb20286a31: Geometry Nodes: Curve to Points Node for Evaluated Data

Summary

This node implements the second option of T87429, creating points
along the input splines with the necessary evaluated information
for instancing: tangent, normal, and rotation attributes.
All generic curve point and spline attributes are copied to the
result points as well.

I'm actually quite happy with the implementation right now. It's
readable enough and there isn't too much boilerplate code. The thing
I expect could use improvement is that there is a lot of temporary
memory allocation. Sharing a buffer for each thread would be a nice
improvement in the future.

The patch currently includes some refactoring of the curve resample
node with some newly abstracted functions. I may commit that separately.
The "Evaluated" mode for the resample node will be a separate commit.

Diff Detail

Repository

rB Blender

Branch

geometry-nodes-curve-to-points-node (branched from master)

Build Status

Buildable 15043
Build 15043: arc lint + arc unit

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Jun 9 2021, 12:32 AM

Hans Goudey (HooglyBoogly) created this revision.

Harbormaster completed remote builds in B15042: Diff 38021.Jun 9 2021, 12:32 AM

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)Jun 9 2021, 12:34 AM

Hans Goudey (HooglyBoogly) added a task: T87429: Curve evaluation / to points for instancing.

Hans Goudey (HooglyBoogly) added a reviewer: Jacques Lucke (JacquesLucke).

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)

Hans Goudey (HooglyBoogly) added a project: Geometry Nodes.

Fix RNA struct type

Harbormaster completed remote builds in B15043: Diff 38022.Jun 9 2021, 12:52 AM

HEYPictures (HEYPictures) awarded a token.Jun 9 2021, 9:29 AM

Alex Smith (asmitty) added a subscriber: Alex Smith (asmitty).Jun 9 2021, 10:25 AM

Alex Smith (asmitty) removed a subscriber: Alex Smith (asmitty).

Jacques Lucke (JacquesLucke) requested changes to this revision.Jun 9 2021, 1:37 PM

Jacques Lucke (JacquesLucke) added inline comments.

source/blender/nodes/geometry/nodes/node_geo_curve_to_points.cc
75	Add a comment that says that this is just filling the cache.
137	This function can make use of the function below to avoid duplication.
191	What is a non-poly case here?
227	even -> uniform?
237	In my test `size` was sometimes zero which leads to crash in `sample_uniform_index_factors`. My guess is that `sample_uniform_index_factors` should just support the case when `samples_size == 0`.
239	I wonder whether position, radius and tilt could be handled as part of the more generic loop below. It could just use the attribute api.

This revision now requires changes to proceed.Jun 9 2021, 1:37 PM

Merge branch 'master' into geometry-nodes-curve-to-points-node
Add comment
Check for size==0, reduce duplication

Harbormaster completed remote builds in B15063: Diff 38067.Jun 9 2021, 3:54 PM

Hans Goudey (HooglyBoogly) added inline comments.Jun 9 2021, 3:54 PM

source/blender/nodes/geometry/nodes/node_geo_curve_to_points.cc
191	I just meant the other two spline types, I'll make that more clear.
239	Right, I thought that at first too, but since we're using the evaluated values here the situation is different. We need a single span for each spline, and the only way to get that from the attribute API would be to pick about the `VArray` that it returns, which I don't think is a good thing to do. I thought a bit about moving the spline attribute API from the custom data storage to the spline itself, so that it could have special cases for the builtin attributes. I'm not sure that's better though, it has some other downsides.

Seems to work as expected now. It's just a bit annoying that the radius of the points is so large by default..

Simon and/or Dalai should still have a look at it.

This revision is now accepted and ready to land.Jun 9 2021, 4:38 PM

I think the default "mode" should be Count (or at least Length)
I think the radius should now be interpolated from the curve, but instead set to the same we use in the Point Distribute (0.05)
Why to call it Length instead of Distance?

This revision now requires changes to proceed.Jun 9 2021, 6:05 PM

Charlie Jolly (charlie) mentioned this in T86749: Geometry Node Community Additions.Jun 10 2021, 1:08 PM

I think the radius should now be interpolated from the curve, but instead set to the same we use in the Point Distribute (0.05)

Interpolating the radius seems like the right thing to do. I don't see why radius should be a special case. Maybe we should set a smaller default radius in some nodes.

Change default to "Count", divide radius by 10

I'm curious what people think about this solution for the radius.
I think it's a good compromise.

Harbormaster completed remote builds in B15107: Diff 38155.Jun 10 2021, 4:43 PM

Alex Smith (asmitty) awarded a token.Jun 11 2021, 3:37 AM

Lopo Isaac (lopoIsaac) awarded a token.Jun 12 2021, 12:38 PM

Suggested limit of 2 for count minimum. Other than that good to go.

I would still love to hear from artists on this. Specially in regarding to the expectations regarding the tangents. But nothing blocking the patch.

This revision is now accepted and ready to land.Jun 14 2021, 3:36 PM

Specially in regarding to the expectations regarding the tangents.

What do you mean by this?

Closed by commit rBfcbb20286a31: Geometry Nodes: Curve to Points Node for Evaluated Data (authored by Hans Goudey (HooglyBoogly)). · Explain WhyJun 14 2021, 7:53 PM

This revision was automatically updated to reflect the committed changes.

Hans Goudey (HooglyBoogly) added a commit: rBfcbb20286a31: Geometry Nodes: Curve to Points Node for Evaluated Data.

Duarte Farrajota Ramos (duarteframos) awarded a token.Jun 15 2021, 12:47 AM

Hans Goudey (HooglyBoogly) mentioned this in T89215: Curve Endpoints Node.Jun 16 2021, 9:30 PM

Geometry Nodes: Curve to Points Node
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 38022

release/scripts/startup/nodeitems_builtins.py

source/blender/blenkernel/BKE_node.h

source/blender/blenkernel/BKE_spline.hh

source/blender/blenkernel/intern/node.cc

source/blender/blenkernel/intern/spline_base.cc

source/blender/makesdna/DNA_node_types.h

source/blender/makesrna/intern/rna_nodetree.c

source/blender/nodes/CMakeLists.txt

source/blender/nodes/NOD_geometry.h

source/blender/nodes/NOD_static_types.h

source/blender/nodes/geometry/nodes/node_geo_curve_resample.cc

source/blender/nodes/geometry/nodes/node_geo_curve_to_points.cc

Geometry Nodes: Curve to Points NodeClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 38022

release/scripts/startup/nodeitems_builtins.py

source/blender/blenkernel/BKE_node.h

source/blender/blenkernel/BKE_spline.hh

source/blender/blenkernel/intern/node.cc

source/blender/blenkernel/intern/spline_base.cc

source/blender/makesdna/DNA_node_types.h

source/blender/makesrna/intern/rna_nodetree.c

source/blender/nodes/CMakeLists.txt

source/blender/nodes/NOD_geometry.h

source/blender/nodes/NOD_static_types.h

source/blender/nodes/geometry/nodes/node_geo_curve_resample.cc

source/blender/nodes/geometry/nodes/node_geo_curve_to_points.cc

Geometry Nodes: Curve to Points Node
ClosedPublic
Actions

Revision Contents
Changeset List