This replaces the minimum vert count with a selection field input. The Face Neighbors -> Vertex Count input node can be used to replicate the minimum size as well as make it able to triangulate based on other conditions such as face size, position, or other datapoint.
Details
Details
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- triangle (branched from master)
- Build Status
Buildable 20099 Build 20099: arc lint + arc unit
Event Timeline
Comment Actions
Thanks for looking into this.
The function in MOD_triangulate can be made static now.
And the annoying part: versioning, this will require adding those two nodes attached to every triangulate node. There is some precedent in the versioning code though.
| source/blender/nodes/geometry/nodes/node_geo_triangulate.cc | ||
|---|---|---|
| 19–20 | Add the BMesh includes in a separate block | |
| 56 | I think this function could take an IndexMask selection input if it used something like BM_mesh_elem_table_ensure(bm, BM_FACE); and BM_face_at_index | |
| 66–68 | I think all the handling of custom split normals can be removed, that's not something we've really supported in GN yet (and we have different plans with T93551). | |
| 88–89 | With the changes mentioned above, the input mesh can be for_read now I think. | |
| 88–89 | has_mesh was already checked above. | |
| 96–99 | I guess we could verify that this is still true, since that function isn't used here anymore. | |
Comment Actions
- Cleanup from @Hans Goudey (HooglyBoogly)
- Remove C style coding from copied function
TODO: Versioning Code
Comment Actions
- Merge branch 'master' into triangle
- Small cleanup from @Hans Goudey (HooglyBoogly)
- First pass at versioning code.
Comment Actions
This looks good to me, just a few smaller comments.
| source/blender/blenloader/intern/versioning_300.c | ||
|---|---|---|
| 630 ↗ | (On Diff #47013) | This has got me in trouble before, I think using nodeFindSocket would be better here too. |
| 2597 ↗ | (On Diff #47013) | Improve this comment with some reasoning: I forget the reason there, I think you mentioned something in chat? |
| source/blender/nodes/geometry/nodes/node_geo_triangulate.cc | ||
| 52 | static | |
| 77–79 | Unnecessary change here. | |
| source/blender/blenloader/intern/versioning_300.c | ||
|---|---|---|
| 2619 ↗ | (On Diff #47172) | Looks like this loop is now inside the loop you added, probably an error with merging master. |
Comment Actions
The versioning seems to be incomplete unfortunately. A few things that could be improved:
- Only add the additional nodes when Minimum Vertices is larger than 4 and it is not linked.
- If the Minimum Vertices input was linked, the second input of the new compare node has to be linked as well.
I'm currently thinking about how bad it would be to keep the Minimum Vertices input for now. That way we could separate the decision to add a Selection from the decision to remove Minimum Vertices... That should make this patch simpler for sure.
Comment Actions
We talked about this in the meeting today, and while personally I wasn't excited about it, we agreed that having both inputs is fine for now.
| source/blender/nodes/geometry/nodes/node_geo_triangulate.cc | ||
|---|---|---|
| 52 | Pass mesh by reference instead of by value here | |