This node takes a count, vector field, and float field and creates n points at the positions indicated in the vector field at the radii specified in the float field
It is placed in the "Point" menu
Details
Details
- Reviewers
Hans Goudey (HooglyBoogly) Jacques Lucke (JacquesLucke) - Maniphest Tasks
- T93044: Single Point Node
- Commits
- rB9a0a4b0c0d45: Geometry Nodes: Add Points Node
Diff Detail
Diff Detail
- Repository
- rB Blender
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
The code looks good, I'd like to discuss this one more time though, since last time I think we weren't completely sure we wanted to add it.
Design-wise, I could see this as part of a "Point Primitives" menu with another 2D/3D grid node.
| source/blender/makesrna/intern/rna_nodetree.c | ||
|---|---|---|
| 11532 ↗ | (On Diff #47456) | Point Cloud should be two words |
| source/blender/nodes/geometry/nodes/node_geo_single_point.cc | ||
| 53 ↗ | (On Diff #47456) | const |
| 59 ↗ | (On Diff #47456) | Since mesh is just a pointer, there's no need to move it. |
Comment Actions
My thoughts are that a single point is a useful primitive (I find myself often making a Curve Line and then deleting one endpoint), but that this could alternatively be implemented by generalizing the Cube primitive. Currently the Cube primitive accepts vertex counts >= 2 on each axis, but the degenerate cases are also useful. If we consider Vx, Vy, and Vz to be the vertex count for each axis, then:
- If one of Vx, Vy, or Vz = 1, and the others are >= 2, the result is a planar grid perpendicular to the axis where V = 1.
- If two of Vx, Vy, or Vz =1 and the remaining is >= 2, the result is a subdivided line aligned with the axis where V >= 2.
- If all three of Vx, Vy, and Vz = 1, the result is a single point as proposed in this patch.
I looked at the existing code for the Cube primitive, and there are enough intrinsic assumptions of V >= 2 that this would need a little refactoring and/or conditional logic. Considering UX design and decluttering of menus, however, I think it might be worth the effort to implement it that way. The Cube node would then provide mesh primitives for the current subdivided cube as well as planes and lines in any of the three axes, plus the goal of @Johnny Matthews (guitargeek) (which I definitely support) to conveniently generate a single point.
An alternative would be to add a point count input parameter to the existing Curve Line primitive to make it function similarly to the Mesh primitives that accept vertex counts, and then to ensure that V = 1 is accepted as valid input (the second vector input, in that case, should remain available to allow for runtime parameter changes on the vertex count, but it would be ignored when V = 1).
That said, the idea of offering Mesh, Curve, or Point Cloud outputs saves the graph space of a conversion node. Using the Cube as a basis, the output could trivially be presented as a Point Cloud. For the Curve option, the existing Mesh to Curve algorithm is reasonably efficient although it ignores the colinearity of points on the cube faces.
Comment Actions
- Merge branch 'master' into singlepoint
- Change to "Points"
- Parallelize Position and Radii copy
- Formatting
| source/blender/nodes/geometry/nodes/node_geo_points.cc | ||
|---|---|---|
| 47–48 | I'd suggest using add_with_destination and retrieving spans for that with the attribute API. I think this would be even simpler. | |
Comment Actions
Looking closer!
| source/blender/nodes/geometry/nodes/node_geo_points.cc | ||
|---|---|---|
| 34 | This can call params.set_default_remaining_outputs(), which is just a bit simpler. There might as well be some warning like other primitive nodes as well. | |
| 46–56 | The OutputAttributes have to be kept around in order to call save() after modifying them. This probably has some debug prints because it doesn't do that. | |
| source/blender/nodes/geometry/nodes/node_geo_points.cc | ||
|---|---|---|
| 39 | A couple empty lines to create sections would make this block easier to read IMO. Also, sometimes I call PointCloudComponent variables just points so retrieving the component fits on a single line. | |
Comment Actions
Generally looks good. However, the field evaluation has to be changed a little bit to avoid unexpected results. Either you can use a separate FieldContext (as in the Volume Cube node), or you evaluate the field into new arrays first and then copy the data to the pointcloud afterwards. The first approach is probably preferred because it involves less overhead.
We'll also talk about the name again next week, just to be sure that Points is fine with everyone.
| source/blender/nodes/geometry/nodes/node_geo_points.cc | ||
|---|---|---|
| 57–63 | This can reuse the implementation in IndexFieldInput::get_index_varray | |
| 58 | It doesn't really matter since this won't be used in the end, but a grain size of 1 is quite low for this. The grain size is the smallest task size TBB will create when looking for work. Generally it's chosen to lower overhead. If the work was very expensive, then a small grain size makes sense, but for simple work it usually makes sense to keep a larger grain size because the overhead from multi-threading becomes relatively more significant otherwise. | |
| 87 | Can remove this commented line | |
| 90–96 | Use std::move for the field inputs to the evaluator and in params.set_output | |
| source/blender/nodes/geometry/nodes/node_geo_points.cc | ||
|---|---|---|
| 57–63 | I meant actually calling IndexFieldInput::get_index_varray(mask); :) | |