This node has 2 modes: the first mode computes a circle from 3 locations and a resolution. The second mode takes radius and resolution.
Details
Details
- Reviewers
Hans Goudey (HooglyBoogly) - Maniphest Tasks
- T89220: Curve Primitive Nodes
- Commits
- rBc1fc18086118: Geometry Nodes: Curve Primitive Circle
Diff Detail
Diff Detail
Event Timeline
Comment Actions
Can this have two outputs? One for the cirlcle but another for the arc from point 1, through 2, ending at 3?
I think people needing it for an arc will be more common than actually creating the whole circle
Comment Actions
I'd have to experiment. I was happy just to get the math to work for the whole circle! :D
Comment Actions
- This is great! It's really satisfying when you plug in three empties to it and it works well!
- It would be nice to give the node an error message in its failure cases (the points are collinear, etc).
- The inline comments are just an initial review, I'd like to come back and understand this better in another pass.
- Better input names would probably be "Start", "Rim" / "Middle", and "End"
- I'm not sure about combining "Arc" and "Circle" in a single node, here's an alternate proposal below, I'm curious what you think about that.
- Another benefit of the mockup below is that we wouldn't need the "Polygon" node.
| source/blender/nodes/geometry/nodes/node_geo_curve_primitive_points_circle.cc | ||
|---|---|---|
| 43–44 | Where possible, it's preferred to use float3 methods instead. Also, don't forget the f at the end of 0.5 here. | |
| 53 | I think calling a static method on an instance of the class is a bit confusing. You can use float3::cross instead. | |
| 57–58 | You can declare these const like const float v4 = float3::cross(v3, v1).normalized(); | |
| 68 | Same here, float3::distance | |
| 90 | This can be declared inside each loop, lower scope is nicer than reusing variables. | |
Comment Actions
Awesome, thanks! I just remembered that for the arc in radius mode it would need to have a start and end angle, oops.
Comment Actions
Makes sense. I need to wire up a toggle button for the circle node and it will be ready, but at the moment with an int to choose the mode, it works perfectly. Then I can start on the arc node from that code.
| source/blender/nodes/geometry/nodes/node_geo_curve_primitive_circle.cc | ||
|---|---|---|
| 28 ↗ | (On Diff #38672) | Soft max of 0 for the radius maybe? |
| 41–43 ↗ | (On Diff #38672) | Unused variable |
| 45–47 ↗ | (On Diff #38672) | These three lines don't do anything. |
| 110 ↗ | (On Diff #38672) | /home/hans/Blender-Git/blender/source/blender/nodes/geometry/nodes/node_geo_curve_primitive_circle.cc:110:29: warning: ignoring return value of ‘bool isect_plane_plane_plane_v3(const float*, const float*, const float*, float*)’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
110 | isect_plane_plane_plane_v3(plane_1, plane_2, plane_3, center);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~You could return early if this fails? Maybe allow creating an empty geometry set. |
| 121 ↗ | (On Diff #38672) | You could pull out theta_step here like below. |
| 122–125 ↗ | (On Diff #38672) | In C++ we can overload the + and * operators, making this much simpler: |
| 128 ↗ | (On Diff #38672) | This is done by spline->resize |
Comment Actions
Requested Revisions - Sorry I was copying what I thought was boilerplate code for the UI button and apparently had copied more than I needed. Also, I forget that float3's are so heavily operator overloaded.
Comment Actions
Looks good. I changed a few things when committing (small cleanup):
- Declare a few more variables const
- Change whitespace slightly (remove extra newlines, add period at end of comment, etc.
- Used unique_ptr implicit boolean conversion instead of == nullptr
- Switch order of if (curve != nullptr) { test to remove double negative
Comment Actions
@Johnny Matthews (guitargeek)
Here we have confusing situation, when there are two identical options in the node search. One for mesh circle and one for curve circle.
Can you fix naming?
