Exposes the "Connected" mode from the weld modifier in the "Merge by
Distance" geometry node.
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
Event Timeline
| source/blender/nodes/geometry/nodes/node_geo_merge_by_distance.cc | ||
|---|---|---|
| 6 | Do you need to add this include? | |
| 21–23 | I'm not sure if this makes more sense as a boolean option or a mode enum in the node options. Currently I'm learning towards an enum option rather than a boolean input. What do you think? | |
| 22 | False is already the default default value, no need to specify it. | |
| 60–90 | Better to move the two cases a bit higher, using two different field evaluations-- one with the boolean array as a result, another using get_evaluated_as_mask. | |
| 94 | There's no need, since we can make a selection of loose edges with geometry nodes. | |
| 104–107 | Try combining two curve primitives with the "Geometry to Instance" node, and you'll see this error message appear twice. | |
| source/blender/nodes/geometry/nodes/node_geo_merge_by_distance.cc | ||
|---|---|---|
| 21–23 | I think that makes sense, yes. That would match how it's currently presented in the weld modifier. I'll take a look at this tomorrow. I've addressed your other notes now. | |
| source/blender/nodes/geometry/nodes/node_geo_merge_by_distance.cc | ||
|---|---|---|
| 84–89 | You can take this a bit further by using evaluator.add_with_destination, which will allow avoiding virtual arrays entirely. | |
| source/blender/nodes/geometry/nodes/node_geo_merge_by_distance.cc | ||
|---|---|---|
| 84–89 | Where do we store the results instead? | |
| source/blender/nodes/geometry/nodes/node_geo_merge_by_distance.cc | ||
|---|---|---|
| 84–89 | In this case you can create a boolean array like before, but give that as an argument to add_with_destination. | |
Comment Actions
Accepting because the changes I requested are trivial and there's no need for another round of review. Thanks!
| source/blender/blenloader/intern/versioning_300.c | ||
|---|---|---|
| 2420 | Unless you're bumping the subversion in this commit (which you don't need to do), you can just add this right under /* Keep this block, even when empty. */, without the top if statement | |
| source/blender/makesrna/intern/rna_nodetree.c | ||
| 526 | Suggested description:
That performance claim should be verified I suppose. | |
| source/blender/nodes/geometry/nodes/node_geo_merge_by_distance.cc | ||
| 122 | BLI_assert_unreachable(); | |
| source/blender/makesrna/intern/rna_nodetree.c | ||
|---|---|---|
| 526 | I find your suggested description for the "All" mode confusing. It seems to say that only the selection is considered for the merge, but then it goes on to say that the selection doesn't matter? Perhaps it means that selected points can be merged with non-selected points if they are close enough? I haven't looked at the actual algorithm. As for the "Connected" mode, I had opted to re-use the description from the Weld modifier. Anyway, I don't think it's productive to bike-shed over these descriptions, feel free to change them! | |
| source/blender/makesrna/intern/rna_nodetree.c | ||
|---|---|---|
| 526 | Eek! My description for the "all" method is indeed wrong. The last word should be "connected", not "selected" oops! And yes, mentioning "closeness" too might be helpful. | |
| source/blender/makesrna/intern/rna_nodetree.c | ||
|---|---|---|
| 525 | I added the descriptions I mentioned and moved this declaration inside of def_geo_merge_by_distance. | |
| source/blender/nodes/geometry/nodes/node_geo_merge_by_distance.cc | ||
| 68–70 | Checking selection.is_empty() for an already allocated Array doesn't make sense, I removed that when committing. | |