Details

Reviewers

Jacques Lucke (JacquesLucke)
Hans Goudey (HooglyBoogly)

Commits

rBfd0370acc212: Geometry Nodes: Add explicit Float to Int conversion node

Summary

This patch adds a very simple node that explicitly converts a float to an int. While this may seem redundant, it would offer 2 benefits to the current requirement to use implicit float conversions:

It makes the node tree's intent more clear and self-documenting (especially if changes in the future require integer inputs for some nodes).
It eliminates undefined behavior in current/future nodes from float inputs by guaranteeing that the input is an integer.

As per Jacques Lucke, the node now offers a variety of rounding techniques to make it more flexible. Documentation update is attached as a child diff.

Diff Detail

Repository: rB Blender

Event Timeline

Nikhil Shringarpurey (Nikhil.Net) requested review of this revision.Jun 25 2021, 1:45 AM

Nikhil Shringarpurey (Nikhil.Net) created this revision.

Nikhil Shringarpurey (Nikhil.Net) edited the summary of this revision. (Show Details)Jun 25 2021, 1:48 AM

Nikhil Shringarpurey (Nikhil.Net) added reviewers: Jacques Lucke (JacquesLucke), Hans Goudey (HooglyBoogly).

I think in such a node it makes sense to have different modes: round, floor, ceil, truncate. What do you think?

HEYPictures (HEYPictures) awarded a token.Jun 25 2021, 1:53 PM

In D11700#301222, @Jacques Lucke (JacquesLucke) wrote:

I think in such a node it makes sense to have different modes: round, floor, ceil, truncate. What do you think?

Good point. Best to allow specific options. I'll add the appropriate RNA/DNA and code.

I have added various conversion types and added the supporting RNA and DNA elements.

I could use a touch of help though, as the layout function is throwing an error in the system console ("uiItemR: property not found: GeometryNodeFloatToInt.roundingmode"). It looks like the properties are all hooked up correctly, but I am obviously missing something...

I like having a node with these options. Generally I think this makes more sense to implement as a function node though, so it will work in the attribute processor and potentially other places.

Also, don't forget to use clang format on the patch.

source/blender/makesdna/DNA_node_types.h
1393	`roundingmode` -> `rounding_mode`
source/blender/makesrna/intern/rna_nodetree.c
10011	`ceil` -> `ceiling`
10019	`roundingmode` -> `rounding_mode`
10020	The call to `RNA_def_struct_sdna_from` tells makesrna to look in the storage struct for the subsequently defined properties. Because there is no "custom1" in `NodeGeometryFloatToInt` I expect that this fails. So replacing `custom1` with `rounding_mode` should make this work. That's my guess anyway, I haven't tested it.

This revision now requires changes to proceed.Jun 25 2021, 9:54 PM

Also, the node "class" AKA category, should be NODE_CLASS_CONVERTOR probably.

release/scripts/startup/nodeitems_builtins.py
569	And the category should be "Utilities" here.

Got all of your suggestions accounted for and I fixed the error - just figuring out the clang formatting and I'll drop a new patch. The storage issue turned out to be more complex but solvable by using an existing DNA field that is already there (custom1), which is what the Triangulate node also does.

Appreciate the suggestions/guidance!

Diff updated to clang-format.

Verified functional on Windows 64-bit. I have revisions to the manual standing by for submission as well.

Nikhil Shringarpurey (Nikhil.Net) mentioned this in D11711: Documentation for new Float To Int Node.Jun 26 2021, 12:30 AM

Nikhil Shringarpurey (Nikhil.Net) added a child revision: D11711: Documentation for new Float To Int Node.Jun 26 2021, 12:32 AM

Final edits to cleanup, as per Hans Goudey's suggestions. Also switching image in initial diff to match new look of node.

Fixed the ridiculous rounding errors in the first line of the table.

Fixed last commit - is there any way to delete Diff 38809 and 38810? I did a dumb thing and fixed it, but deleting both those diffs would clean up the history and show the completed tasks better.

Sorry, I should have clarified earlier. What I meant by the "function node" comment was implementing it like one of the node_fn_* functions, node_fn_float_compare.cc is probably a good example.

Currently there is no visible difference between the two for geometry nodes, but the function nodes are more generic (easier to make work in contexts different than geometry nodes), and would be more helpful for the attribute processor or whatever other "parallel" concepts we use next.

source/blender/nodes/geometry/nodes/node_geo_float_to_int.cc
45–46 ↗	(On Diff #38810)	Comments should start with a `/*`, and end with a period.
56 ↗	(On Diff #38810)	`roundingmode` -> `rounding_mode`

This revision now requires changes to proceed.Jun 29 2021, 12:05 AM

Hans, I get what you mean now. So if I just move the code around to the right folders and do some light editing, it will still be usable in Geometry Nodes? I did not realize all function nodes automatically become available there.

Right, exactly. If a node has a function node implementation then the geometry nodes evaluator can use it as well.

Pratik Borhade (PratikPB2123) added a subscriber: Pratik Borhade (PratikPB2123).Jun 29 2021, 6:04 AM

Updated all code to make node into a Function node. Removed unneeded DNA storage type.

Code is generally patterned after the Boolean Math node. Tested and functional including file save/load on Windows64.

Clang format is not applied to the patch. Other than that and the small comments inline, this looks good.

source/blender/nodes/function/nodes/node_fn_float_to_int.cc
70	Sorry to badger, but `roundingmode` is still here, seems better to be consistent with variable naming.
85	`BLI_assert_unreachable()` is a more obvious way to do this.
102	Best to just remove this if you don't need it.

This revision now requires changes to proceed.Jun 29 2021, 11:36 PM

Made changes suggested by Hans. Not sure about your reference to clang-formatting the diff, though, as I made sure to run "make format" before generating the previous diff as well as this one, so the code should already be clang-format compliant.

EDIT: I did a little experimentation and found that clang-format on Windows does not seem to enforce leading tab restrictions. I made several changes to files both increasing and decreasing their indentations and "make format" went thru all files and changed nothing. Possibly a bug report for another topic regarding clang-format on Win.

Manually formatted leading space, since make format will not do any changes to these files.

@Hans Goudey (HooglyBoogly) I only have Windows available to me at the moment - is there any other way to have someone on Linux run a "make format" against this diff? Otherwise, I think I'm out of options until I can setup a Linux box.

As far as I know make format should work on Windows. I can just do it when committing though

This revision is now accepted and ready to land.Jul 5 2021, 5:41 PM

Jacques Lucke (JacquesLucke) accepted this revision.Jul 5 2021, 5:46 PM

In D11700#304039, @Hans Goudey (HooglyBoogly) wrote:

As far as I know make format should work on Windows. I can just do it when committing though

Make format will work out of the box on windows, nothing extra needed, just run it in the main source folder

Closed by commit rBfd0370acc212: Geometry Nodes: Add explicit Float to Int conversion node (authored by Nikhil Shringarpurey (Nikhil.Net), committed by Hans Goudey (HooglyBoogly)). · Explain WhyJul 5 2021, 6:54 PM

This revision was automatically updated to reflect the committed changes.

Hans Goudey (HooglyBoogly) added a commit: rBfd0370acc212: Geometry Nodes: Add explicit Float to Int conversion node.

In D11700#304044, @Ray Molenkamp (LazyDodo) wrote:

In D11700#304039, @Hans Goudey (HooglyBoogly) wrote:

As far as I know make format should work on Windows. I can just do it when committing though

Make format will work out of the box on windows, nothing extra needed, just run it in the main source folder

I know this is closed, @Ray Molenkamp (LazyDodo), but I definitely confirmed an issue. I'm going to post it as a bug, but as a test, I changed various leading spaces in the "source/blender/nodes/function/nodes/node_fn_float_to_int.cc" file and saved it, and then ran make format after each change, and each time it did not do anything to the file. I did change larger things like curly brace placement and it would change those, but it was stubbornly refusing to do anything with the indenting spaces. Trust me, I ran it 10-15 times back to back just to see if I was crazy.

Geometry Nodes: Add explicit Float to Int conversion node
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 39168

release/scripts/startup/nodeitems_builtins.py

source/blender/blenkernel/BKE_node.h

source/blender/blenkernel/intern/node.cc

source/blender/makesdna/DNA_node_types.h

source/blender/makesrna/RNA_enum_types.h

source/blender/makesrna/intern/rna_nodetree.c

source/blender/nodes/CMakeLists.txt

source/blender/nodes/NOD_function.h

source/blender/nodes/NOD_static_types.h

source/blender/nodes/function/nodes/node_fn_float_to_int.cc

Geometry Nodes: Add explicit Float to Int conversion nodeClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 39168

release/scripts/startup/nodeitems_builtins.py

source/blender/blenkernel/BKE_node.h

source/blender/blenkernel/intern/node.cc

source/blender/makesdna/DNA_node_types.h

source/blender/makesrna/RNA_enum_types.h

source/blender/makesrna/intern/rna_nodetree.c

source/blender/nodes/CMakeLists.txt

source/blender/nodes/NOD_function.h

source/blender/nodes/NOD_static_types.h

source/blender/nodes/function/nodes/node_fn_float_to_int.cc

Geometry Nodes: Add explicit Float to Int conversion node
ClosedPublic
Actions

Revision Contents
Changeset List