Details

Reviewers

Maniphest Tasks

Commits

rB21ebee258027: Geometry Nodes: Curve Primitive Spiral

Summary

This patch adds a Curve Primitives menu in Geometry nodes with an initial entry of Curve Spiral.

This node creates a curve geometry object and gives control for the number of rotations, the number of points per rotation, start and end radius, height, and direction.

Diff Detail

Repository: rB Blender

Event Timeline

Johnny Matthews (guitargeek) requested review of this revision.Jun 15 2021, 6:14 AM

Johnny Matthews (guitargeek) created this revision.

Arian (14AUDDIN) awarded a token.Jun 15 2021, 5:27 PM

Add Socket Subtypes

Charlie Jolly (charlie) mentioned this in T86749: Geometry Node Community Additions.Jun 15 2021, 6:24 PM

Aneesh Nazar (aneeshnazar) added a subscriber: Aneesh Nazar (aneeshnazar).Jun 16 2021, 8:43 AM

Johnny Matthews (guitargeek) added a project: Geometry Nodes.Jun 16 2021, 4:45 PM

Aaron Carlisle (Blendify) added a subscriber: Aaron Carlisle (Blendify).Jun 16 2021, 4:57 PM

Aaron Carlisle (Blendify) added inline comments.

source/blender/nodes/geometry/nodes/node_geo_curve_primitive_spiral.cc
22	"Subdivisions" may be a better name

HEYPictures (HEYPictures) awarded a token.Jun 16 2021, 6:12 PM

Tomas (Rusculleda) awarded a token.Jun 16 2021, 6:47 PM

Hans Goudey (HooglyBoogly) added a task: T89220: Curve Primitive Nodes.Jun 16 2021, 11:40 PM

Deen (Bit) awarded a token.Jun 17 2021, 6:55 AM

Lloyd (digim0nk) awarded a token.Jun 17 2021, 11:43 PM

Paul Larson (GeorgiaPacific) awarded a token.Jun 18 2021, 1:03 AM

Cleanup

This looks quite nice! I like the inputs you've exposed and the order you chose for them.

I think "Rotations" could be a float input so that you could use it to make an incomplete turn.
In the image below you can see that with "1" rotation specified, the circle isn't completed. I think it would make sense to build the full 360 degree rotation when the rotations input is an integer.

These defaults might be a bit simpler? Aesthetically I like the idea of the spiral getting smaller at the bottom, maybe that's just me though.

source/blender/nodes/geometry/nodes/node_geo_curve_primitive_spiral.cc
22	I think "Resolution" is probably fine, it's clear enough that it's not the entire curve because of the default anyway. "Subdivisions" IMO isn't a very clear name, it makes me think too much of the other subdivision nodes.
40–41	I think `start_radius` and `end_radius` are fine, no need for the contractions. This is subjective, it's just more consistent with the way other code is written in this area.
61–62	To simplify this, you could start `accumulate_radius` out at `start_radius`, and then just add `delta_radius` to it. Also, given the clearer names for `srad` and `erad`, IMO it's probably fine to drop the `accumulate_` prefix.
84–91	You can avoid choosing names and retyping the inputs like this: It's probably subjective, but IMO this is a bit more straightforward. std::unique_ptr<CurveEval> curve = create_spiral_curve( std::max(params.extract_input<int>("Rotations"), 1), std::max(params.extract_input<int>("Points Per Rotation"), 1), params.extract_input<float>("Start Radius"), params.extract_input<float>("End Radius"), params.extract_input<float>("Height"), params.extract_input<bool>("Reverse"));