Geometry Nodes: Add quad_method and ngon_method enums to triangulate node
ClosedPublic
Actions

Authored by Léo Depoix (PiloeGAO) on Nov 7 2020, 7:38 PM.

Details

Reviewers

Hans Goudey (HooglyBoogly)
Dalai Felinto (dfelinto)

Summary

This diff is part of "Cleanup 1st Sprint Nodes" (https://developer.blender.org/T82370).

This add two drop-downs for quad and ngon methods based the triangulate modifier.
I couldn't add "Keep Normals" as input because geometry-nodes doesn't support custom datas for now.

I have two problems in this diff:

I set a default property in RNA but UI didn't update, did you know why? (Sorry if it's a beginner question) [Edit: Done]
I found nothing on adding a title to enums, like that :

Is this possible in nodes?

Example:

Diff Detail

Repository: rB Blender

Event Timeline

Léo Depoix (PiloeGAO) requested review of this revision.Nov 7 2020, 7:38 PM

Léo Depoix (PiloeGAO) created this revision.

Léo Depoix (PiloeGAO) mentioned this in T82370: Cleanup 1st Sprint Nodes.

I set a default property in RNA but UI didn't update, did you know why? (Sorry if it's a beginner question)

I think you have to set this in the node's "init" method. One of the geometry nodes already does this, I forget which one right now though, sorry. It's pretty simple though.

This revision add node init void to set default enums values in UI (shortedge for quad_method and beauty for ngon_method).

Thanks for working on this! Just one comment, but I'm accepting this because otherwise it looks good.

source/blender/nodes/geometry/nodes/node_geo_triangulate.cc
43–44	You should be able to use the explicit values `GEO_NODE_TRIANGULATE` etc. here. Maybe you'd have to include `DNA_node_types.h`, but that's fine.

This revision is now accepted and ready to land.Nov 9 2020, 4:16 AM

Added very minor comments.

source/blender/makesdna/DNA_node_types.h
1457	Newline at the end. Editors can do that.
source/blender/makesrna/intern/rna_nodetree.c
8248	The last period should be removed.
8256	Same here for the period.
source/blender/nodes/geometry/nodes/node_geo_triangulate.cc
58–59	They can be marked `const`. Also prefer `static_cast`. https://wiki.blender.org/wiki/Style_Guide
61	0 1 3 look like magic numbers here.

I made all changes requested by @Hans Goudey (HooglyBoogly) and @Ankit Meel (ankitm) .

Thank you for your feedback.

Ankit Meel (ankitm) added inline comments.Nov 9 2020, 1:53 PM

source/blender/nodes/geometry/nodes/node_geo_triangulate.cc
65	https://godbolt.org/z/Y99vT9 `sizeof(enum)` shows the size of underlying type of the enum, not the total elements in the enum. I wonder if this check is really needed? Does the triangulate modifier check for out-of-enum values?

Ankit Meel (ankitm) awarded a token.Nov 9 2020, 1:54 PM

I removed the out-of-enum check (not present in the triangulate modifier) as @Ankit Meel (ankitm) suggested.

Léo Depoix (PiloeGAO) marked an inline comment as done.Nov 9 2020, 5:06 PM

Using defines instead of hardcoded 1, 2, 3, ... is a hard requirement for me. @Léo Depoix (PiloeGAO) could you please address the issues mentioned here. It should be all easy.