Curves: Initial geometry nodes support for curves object.
ClosedPublic
Actions

Authored by Jacques Lucke (JacquesLucke) on Mar 8 2022, 1:25 PM.

Details

Reviewers

Hans Goudey (HooglyBoogly)

Maniphest Tasks

T96215: Support Curves object in Object Info node.

Commits

rB2839fe9a4d30: Curves: initial geometry nodes support for curves objects

Summary

Diff Detail

Repository

rB Blender

Branch

curves-geometry-nodes (branched from master)

Build Status

Buildable 20906
Build 20906: arc lint + arc unit

Event Timeline

Jacques Lucke (JacquesLucke) requested review of this revision.Mar 8 2022, 1:25 PM

Jacques Lucke (JacquesLucke) created this revision.

Harbormaster completed remote builds in B20906: Diff 49028.Mar 8 2022, 1:25 PM

Jacques Lucke (JacquesLucke) edited the summary of this revision. (Show Details)Mar 8 2022, 1:26 PM

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

Jacques Lucke (JacquesLucke) edited the summary of this revision. (Show Details)Mar 8 2022, 1:29 PM

Jacques Lucke (JacquesLucke) edited the summary of this revision. (Show Details)

RedMser (RedMser) added a subscriber: RedMser (RedMser).Mar 8 2022, 6:43 PM

RedMser (RedMser) added inline comments.

release/scripts/startup/bl_operators/geometry_nodes.py
29	Isn't the `'CURVES'` type used for hair objects? If so, the comment is outdated.

I really like how simple the modifier stack for the curves object is. The way it should be!

source/blender/blenkernel/intern/curve_eval.cc
402–403	I think it would probably be better to do this as part of the curves modifier stack. It's not just nodes that use `CurveEval` that don't support Catmull-Rom curves. And I wouldn't expect that updating a node to use `Curves` would also mean that it supports them immediately either.

Jacques Lucke (JacquesLucke) added inline comments.Mar 8 2022, 7:58 PM

source/blender/blenkernel/intern/curve_eval.cc
402–403	I don't quite see why this should be part of the modifier stack? I didn't intend to commit it with this todo btw. I think we generally shouldn't assert when there is a catmull rom curve. Just ignore it until we add support for it.

Hans Goudey (HooglyBoogly) added inline comments.Mar 8 2022, 8:13 PM

source/blender/blenkernel/intern/curve_eval.cc
402–403	Ah, if the idea wasn't to commit it like that, makes sense. We can remove the asserts I added, but we should also make sure that ignoring some curves doesn't break the nodes.

Jacques Lucke (JacquesLucke) added a task: T96215: Support Curves object in Object Info node..Mar 9 2022, 1:58 PM

Philipp Oeser (lichtwerk) added a subscriber: Philipp Oeser (lichtwerk).Mar 25 2022, 11:02 AM

Recently I've been thinking we should do this by using the new curves type for drawing CurveComponent in the viewport, so basically removing any case that we currently have to convert to CurveEval.
Based on T68981, we're getting very close to that point, and doing it that way would avoid any wasted work supporting Catmull Rom curves with CurveEval or something.

Seems reasonable as well, I don't know how complex it would be to update the drawing code tbh.
Regardless, it should also be possible to use CurveEval for drawing, even if it does not support Catmull Rom curves. We'd just need a function to convert them to bezier curves. Not saying we should do that, unless updating drawing code takes too long.

Hans Goudey (HooglyBoogly) mentioned this in T96874: New haircurves brush adds them with zero radius, radius not taken into account by Eevee (when rendered as strips).Mar 30 2022, 5:17 AM

Hans Goudey (HooglyBoogly) mentioned this in D11592: Alembic/USD: use geometry sets to import data.Apr 1 2022, 1:06 AM

Hans Goudey (HooglyBoogly) added inline comments.Apr 8 2022, 3:06 PM

source/blender/blenkernel/intern/curve_eval.cc
402–403	At this point I think it's probably fine to say "Some nodes don't support catmull rom curves and convert them to poly curves"
source/blender/blenkernel/intern/object_dupli.cc
854	I think having `OB_CURVES` here will cause the rendering to use the new curves object, which we probably don't want until the GPU drawing code for `Curves` is finished. So I think I'd leave this part unchanged for now. We can easily toggle between the two by changing `get_curve_for_render` to `get_for_read`

Merge branch 'master' into curves-geometry-nodes
cleanup
fix

source/blender/blenkernel/intern/object_dupli.cc
854	Hm, without this change, the curves are drawn twice currently. Needs to be investigated. It may be fine to just use the curves drawing for now, since we have been doing that before this patch as well.

Harbormaster completed remote builds in B21528: Diff 50246.Apr 8 2022, 3:45 PM

Jacques Lucke (JacquesLucke) retitled this revision from Curves: Initial geometry nodes support for curves object (WIP). to Curves: Initial geometry nodes support for curves object..Apr 8 2022, 3:45 PM

Jacques Lucke (JacquesLucke) edited the summary of this revision. (Show Details)

Jacques Lucke (JacquesLucke) added inline comments.Apr 14 2022, 12:23 PM

source/blender/blenkernel/intern/object_dupli.cc
854	@Hans Goudey (HooglyBoogly) can you say if it's ok for you if we don't change how curves objects are drawn as part of this patch?

Hans Goudey (HooglyBoogly) accepted this revision.Apr 14 2022, 5:54 PM

Hans Goudey (HooglyBoogly) added inline comments.

source/blender/blenkernel/intern/curves.cc
328–331	Did you find that creating an empty curves data-block was necessary, or is this more preemptive? If it's not necessary, I think we might as well avoid it now.
source/blender/blenkernel/intern/object_dupli.cc
854	Okay, I think this is fine as is, since it only affects the new curves object, and it doesn't affect instances. That means you can use the old drawing with the Geometry to Instance node, which is actually great for testing!