Details

Reviewers

Hans Goudey (HooglyBoogly)
Jacques Lucke (JacquesLucke)

Commits

rB17578408434f: Geometry Nodes: Generalized Compare Node

Summary

Replace the float compare node with a generalized compare node. The node allows for the comparison of float, int, string, color, and vector.

The datatypes support the following operators:
Float, Int: <, >, <=, >=, ==, !=
String: ==, !=
Color: ==, !=, lighter, darker (using rgb_to_grayscale value as the brightness value)

Vector Supports 5 comparison modes for: ==, !=, <, >, <=, >=

Average: The average of the components of the vectors are compared.
Dot Product: The dot product of the vectors are compared.
Direction: The angle between the vectors is compared to an angle
Element-wise: The individual components of the vectors are compared.
Length: The lengths of the vectors are compared.

T92729

Diff Detail

Repository

rB Blender

Branch

general_compare (branched from master)

Build Status

Buildable 18671
Build 18671: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Finish adding functions for string and color
formatting
Menus now update correctly

Harbormaster completed remote builds in B18675: Diff 44784.Nov 16 2021, 5:02 AM

Johnny Matthews (guitargeek) edited the summary of this revision. (Show Details)Nov 16 2021, 5:10 AM

Johnny Matthews (guitargeek) edited the summary of this revision. (Show Details)Nov 16 2021, 5:16 AM

Johnny Matthews (guitargeek) added a project: Geometry Nodes.

Merge branch 'master' into general_compare

Harbormaster completed remote builds in B18676: Diff 44785.Nov 16 2021, 5:18 AM

Versioning WIP - I will need help.

Harbormaster completed remote builds in B18677: Diff 44786.Nov 16 2021, 6:06 AM

Fix Versioning

Harbormaster completed remote builds in B18708: Diff 44826.Nov 16 2021, 5:34 PM

Johnny Matthews (guitargeek) retitled this revision from Geometry Nodes: Generalized Compare Node - WIP to Geometry Nodes: Generalized Compare Node.Nov 16 2021, 5:35 PM

Johnny Matthews (guitargeek) edited the summary of this revision. (Show Details)

cleanup
Re-add label function

Harbormaster completed remote builds in B18710: Diff 44828.Nov 16 2021, 5:56 PM

Generally this is working well for me!
I think epsilon should always come last, even in the dot product mode.
I'm getting some unexpected behavior in the dot product equal mode:

Compare Dot Product Test.blend881 KBDownload

Switching from vector to color can give an invalid "operation" enum. The enum's update method would have to automatically change the enum when switching to color.

As for code prettiness, I just investigated templating the type and operation, at least for the basic comparisons, but I didn't get anywhere yet.
IMO templating only really feels worth it if both the type and the operation could be templated.

source/blender/nodes/function/nodes/node_fn_compare.cc
164	For directions, I think `angle_v3v3` is simpler here, since the dot product doesn't give you the angle directly.
297–302	I think `rgb_to_grayscale` would be right here
405	/home/hans/Blender-Git/blender/source/blender/nodes/function/nodes/node_fn_compare.cc:379:11: warning: this statement may fall through [-Wimplicit-fallthrough=] 379 \| switch (data->mode) { \| ^~~~~~ I think there should be a `break` at the end of the subcases.

This revision now requires changes to proceed.Nov 16 2021, 9:19 PM

Several Fixes:

Fix order of extra inputs vs function signatures
Missing breaks
Better evals for color and vector direction
Fix operation to valid type when switching to color

Harbormaster completed remote builds in B18715: Diff 44839.Nov 16 2021, 9:54 PM

This looks pretty close now! Though there is a merge conflict with master.

Initially I liked the idea of a dot product mode, but now I'm finding it a bit less intuitive than I expected. Maybe I just need to get used to it, not sure. What do you think about it? Generally I like the idea of the vector comparison nodes, I think they will be useful.

When I played around with the code a bit, I didn't find a great way to template the operations and the types. Could be that it's a difficult problem, or it could just be that I didn't understand it well enough. Maybe @Jacques Lucke (JacquesLucke) has better ideas there.

source/blender/makesrna/intern/rna_nodetree.c
504	This one can be defined inside of `def_compare`, since it isn't used elsewhere.
source/blender/nodes/function/nodes/node_fn_compare.cc
81–83	Suggest `snake_case` naming, maybe even just `comp`, `angle`, and `epsilon`

Initially I liked the idea of a dot product mode, but now I'm finding it a bit less intuitive than I expected. Maybe I just need to get used to it, not sure. What do you think about it? Generally I like the idea of the vector comparison nodes, I think they will be useful.

I don't think it hurts to leave it in. I guess angle comparison does the same thing but on a scale of 0 to 180 rather than -1 to 1. Your call.

minor cleanup
Merge branch 'master' into general_compare
Fix nodeSetSocketAvailability

Harbormaster completed remote builds in B18773: Diff 44940.Nov 18 2021, 10:52 PM

Merge branch 'master' into general_compare

Harbormaster completed remote builds in B18860: Diff 45080.Nov 22 2021, 4:23 PM

Jacques Lucke (JacquesLucke) added inline comments.Nov 24 2021, 6:44 PM

source/blender/nodes/function/nodes/node_fn_compare.cc
78	This should be done in an update function in rna.
130–131	Would it make sense to move the function definitions into the switch statement below? Then every function would be in its own little scope and you could just name it `fn`. Furthermore, you wouldn't need comments like `/* Float Functions */` because this structure would be provided by the switch statement.
274	add some utility method to compute the average, this is repeated a lot here
292	Comment style.

Lopo Isaac (lopoIsaac) awarded a token.Nov 25 2021, 8:51 AM

Hans Goudey (HooglyBoogly) added inline comments.Nov 27 2021, 10:24 PM

source/blender/nodes/function/nodes/node_fn_compare.cc
130–131	I like this idea too, seems the code will self-document a little more this way.

Merge branch 'master' into general_compare
Add file namespace
Move functions further down

Harbormaster completed remote builds in B19017: Diff 45373.Nov 29 2021, 2:01 AM

Johnny Matthews (guitargeek) added inline comments.Nov 29 2021, 2:01 AM

source/blender/nodes/function/nodes/node_fn_compare.cc
78	Not sure how I should go about that.

Hans Goudey (HooglyBoogly) added inline comments.Nov 29 2021, 2:06 AM

source/blender/nodes/function/nodes/node_fn_compare.cc
78	For example, `rna_GeometryNodeAttributeRandomize_data_type_update` does something similar.

Merge branch 'master' into general_compare

Harbormaster completed remote builds in B19048: Diff 45416.Nov 29 2021, 4:32 PM

Move type change default to rna

Harbormaster completed remote builds in B19053: Diff 45424.Nov 29 2021, 5:15 PM

Johnny Matthews (guitargeek) added inline comments.Nov 29 2021, 5:15 PM

source/blender/nodes/function/nodes/node_fn_compare.cc
78	Awesome, thanks!

The code looks okay to me now. I think I won't have any comments after this.

source/blender/makesdna/DNA_node_types.h
1611–1614	Add comments for the types of the other enums.
1903	Maybe this comment should be removed?
source/blender/nodes/function/nodes/node_fn_compare.cc
52	Maybe a default of `0.9f` makes more sense? Since usually you are comparing to see if the dot products are equal-ish. Also, maybe the name "Threshold" would make more sense, since the dot product has to be higher than that value.
53	I think a default of 5 degrees makes a bit more sense.
86	Hmm, I don't think casting from `CustomDataType` to `eNodeSocketDatatype` is correct. Ah, it seems that the comment in `NodeFunctionCompare` is incorrect, it should say `eNodeSocketDatatype`
143	Unnecessary newline. There are some below too.

This revision now requires changes to proceed.Nov 29 2021, 5:19 PM

Final Cleanup

Harbormaster completed remote builds in B19056: Diff 45427.Nov 29 2021, 5:57 PM

Looks mostly good now, I'm just not sure about the string comparison. There are so many ways to order strings, that it seems wrong to just use the default behavior of comparing std::string without further consideration. I'd be fine with keeping the equal and not-equal check for strings, but the other once I'd remove for now. Maybe we need more options for those.

/home/jacques/blender-git/blender/source/blender/makesrna/intern/rna_nodetree.c: In function ‘compare_other_operation_supported’:
/home/jacques/blender-git/blender/source/blender/makesrna/intern/rna_nodetree.c:2122:71: warning: unused parameter ‘item’ [-Wunused-parameter]
 2122 | static bool compare_other_operation_supported(const EnumPropertyItem *item)
      |                                               ~~~~~~~~~~~~~~~~~~~~~~~~^~~~

source/blender/makesdna/DNA_node_types.h
411–412	Various unrelated changes.

This revision now requires changes to proceed.Nov 30 2021, 1:36 PM

formatting
cleanup
Merge branch 'master' into general_compare
Remove extra functions for string

Harbormaster completed remote builds in B19105: Diff 45498.Nov 30 2021, 5:06 PM

LGTM.

Hans Goudey (HooglyBoogly) accepted this revision.Dec 1 2021, 3:41 PM

Hans Goudey (HooglyBoogly) added inline comments.

source/blender/blenloader/intern/versioning_300.c
2175	Add a comment above this block: `/* Convert float compare into a more general compare node. */`
source/blender/nodes/function/nodes/node_fn_compare.cc
52

This revision is now accepted and ready to land.Dec 1 2021, 3:41 PM

Johnny Matthews (guitargeek) edited the summary of this revision. (Show Details)Dec 1 2021, 4:29 PM

Closed by commit rB17578408434f: Geometry Nodes: Generalized Compare Node (authored by Johnny Matthews (guitargeek)). · Explain WhyDec 1 2021, 4:39 PM

This revision was automatically updated to reflect the committed changes.

Johnny Matthews (guitargeek) marked 2 inline comments as done.

Johnny Matthews (guitargeek) added a commit: rB17578408434f: Geometry Nodes: Generalized Compare Node.

Johnny Matthews (guitargeek) edited the summary of this revision. (Show Details)Dec 1 2021, 4:39 PM

Geometry Nodes: Generalized Compare Node
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 44779

release/scripts/startup/nodeitems_builtins.py

source/blender/blenkernel/BKE_node.h

source/blender/blenkernel/intern/node.cc

source/blender/blenloader/intern/versioning_300.c

source/blender/makesdna/DNA_node_types.h

source/blender/makesdna/intern/dna_rename_defs.h

source/blender/makesrna/RNA_enum_items.h

source/blender/makesrna/intern/rna_nodetree.c

source/blender/nodes/CMakeLists.txt

source/blender/nodes/NOD_function.h

source/blender/nodes/NOD_math_functions.hh

source/blender/nodes/NOD_static_types.h

source/blender/nodes/function/nodes/node_fn_compare.cc

source/blender/nodes/function/nodes/node_fn_float_compare.cc

source/blender/nodes/geometry/nodes/legacy/node_geo_attribute_compare.cc

source/blender/nodes/intern/math_functions.cc

Geometry Nodes: Generalized Compare NodeClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 44779

release/scripts/startup/nodeitems_builtins.py

source/blender/blenkernel/BKE_node.h

source/blender/blenkernel/intern/node.cc

source/blender/blenloader/intern/versioning_300.c

source/blender/makesdna/DNA_node_types.h

source/blender/makesdna/intern/dna_rename_defs.h

source/blender/makesrna/RNA_enum_items.h

source/blender/makesrna/intern/rna_nodetree.c

source/blender/nodes/CMakeLists.txt

source/blender/nodes/NOD_function.h

source/blender/nodes/NOD_math_functions.hh

source/blender/nodes/NOD_static_types.h

source/blender/nodes/function/nodes/node_fn_compare.cc

source/blender/nodes/function/nodes/node_fn_float_compare.cc

source/blender/nodes/geometry/nodes/legacy/node_geo_attribute_compare.cc

source/blender/nodes/intern/math_functions.cc

Geometry Nodes: Generalized Compare Node
ClosedPublic
Actions

Revision Contents
Changeset List