Page MenuHome

Geometry Nodes: Add curve index and custom value to sample curve node
ClosedPublic

Authored by Hans Goudey (HooglyBoogly) on Oct 5 2022, 5:33 AM.

Details

Summary

As described in T92474 and T91650, this patch adds two features to the
sample curve node. First is an index input, to allow choosing the curve to
sample for each point. Second is a custom field input, which is evaluated
on the control points of the curve and then sampled like the other outputs.
There is an "All Curves" option for the old behavior which takes the length
of all curves into account.

For invalid curve indices, the node outputs zeros (default values).
Invalid lengths and factors are clamped.

Internally there is some room for improvement (making the code a bit
more generic, etc.), but I tried to keep the change as small as possible.
There have been various discussions about splitting the node up more,
but I think this is an intuitive combination of options and will work well
enough for current use cases. The node can be generalized more in the
future.

Keep in mind that the source field is evaluated on curve control points,
not the evaluated points used for sampling. This is necessary so that
fields like "Index" work as expected.

Diff Detail

Repository
rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Oct 5 2022, 5:33 AM
Hans Goudey (HooglyBoogly) created this revision.

Versioning for "All Curves" option

Simon Thommes (simonthommes) requested changes to this revision.Oct 5 2022, 10:51 PM

I tested the patch, here a couple of points:

  • The positioning of the Value input is not consistent with other node (e.g. raycast, sample uv surface, ) Putting the Value input directly after the curve geometry input also makes more sense as this is the only input field that is actually evaluated on that geometry input.
  • Factor should be the default imo. I know that this is not the case on the existing node either but in my experience this is a much more generally useful setting and changing this doesn't need any versioning anyways
  • The geometry input should probably be called Curves (plural)
  • I already wrote a whole bunch of arguments why we should split up the node for the generic and the properly interpolated curve attributes, but now I'm deleting them. I'm okay with how it is, I think. We can still decide to split it up later and ideally we would generalize this more anyways.

I didn't have time to test the actual functionality yet, I'll try to get to it tomorrow.

This revision now requires changes to proceed.Oct 5 2022, 10:51 PM
Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)
  • Make location of custom socket consistent
  • Set default to factor
  • Rename socket to curves
  • Fix invalid curve index behavior

Thanks for looking into this. I agree this could be split up more, and I think it would be really valuable to spend some time designing how that would work. But I also think it's okay to add this first.

Simon Thommes (simonthommes) requested changes to this revision.Oct 16 2022, 3:09 PM

Thanks for the changes, looks good to me now!

When testing I got a crash opening a file using the node that I didn't get with master.
Here is that file:


(CC-BY Blender Studio https://studio.blender.org/films/heist/3b0f29b4825fa2/?asset=6132)

Could you check on that crash? Otherwise, I think, this is good to go into master.

This revision now requires changes to proceed.Oct 16 2022, 3:09 PM

Thanks for the test file. I think I fixed the issue a couple days ago but forgot to add it to the patch, it was a simple problem.

I don't have time to test, but if you can confirm that the file I sent works fine now that's good for me

This revision is now accepted and ready to land.Oct 20 2022, 12:25 PM