Nodes: use auto registration for nodes #110686

Jacques Lucke · 2023-08-01T14:29:59+02:00

Jacques Lucke commented

2023-08-01 14:29:59 +02:00

The goal here is to reduce the number of files that need to be edited when adding a new node. To register a node, one currently has to add a line to node_geometry_register.cc and node_geometry_register.hh (for geometry nodes). Those files can be generated automatically.

There is a new NOD_REGISTER_NODE macro that nodes can use to register themselves. The macro is then discovered by discover_nodes.py that generates code that calls all the registration functions. The script also works when the register functions are in abitrary namespaces. This allows simplifying the node code as well.

In the past I tried a few times to get auto-registration working without resorting to code generation, but that never ended up working. The general idea for that would be to non-trivial initialization for static variables. The issue always ends up being that the linker just discards those variables, because they are unused and it doesn't care if there are side effects in the initialization. For more context see this and its follow-up post.

Related discussion regarding using Python for code generation: https://devtalk.blender.org/t/code-generation-with-python/30558

The goal here is to reduce the number of files that need to be edited when adding a new node. To register a node, one currently has to add a line to `node_geometry_register.cc` and `node_geometry_register.hh` (for geometry nodes). Those files can be generated automatically. There is a new `NOD_REGISTER_NODE` macro that nodes can use to register themselves. The macro is then discovered by `discover_nodes.py` that generates code that calls all the registration functions. The script also works when the register functions are in abitrary namespaces. This allows simplifying the node code as well. In the past I tried a few times to get auto-registration working without resorting to code generation, but that never ended up working. The general idea for that would be to non-trivial initialization for static variables. The issue always ends up being that the linker just discards those variables, because they are unused and it doesn't care if there are side effects in the initialization. For more context see [this](https://www.cppstories.com/2018/02/factory-selfregister/) and its [follow-up post](https://www.cppstories.com/2018/02/static-vars-static-lib/). Related discussion regarding using Python for code generation: https://devtalk.blender.org/t/code-generation-with-python/30558

❤️ 1 🎉 2

Jacques Lucke added 1 commit 2023-08-01 14:30:08 +02:00

buildbot/vexp-code-patch-coordinator Build done. Details

39e9713d26 initial test

Jacques Lucke commented

2023-08-01 14:44:00 +02:00

@blender-bot build

Iliya Katushenock commented

2023-08-01 14:47:43 +02:00

A very useful thing! It also seems to make sense to consider using user-defined attributes ([[AutoLinking(node_geometry_register)]]) as a way to label this kind of functionality. Then it would be similarly generalized to other areas of the blender.

A very useful thing! It also seems to make sense to consider using user-defined attributes (`[[AutoLinking(node_geometry_register)]]`) as a way to label this kind of functionality. Then it would be similarly generalized to other areas of the blender.

Jacques Lucke added 4 commits 2023-08-02 21:09:35 +02:00

3c9139fd71 Merge branch 'main' into auto-node-register

dc3ff710e9 fixes

6f11f1056e add register

7b915336e0 replace header

Jacques Lucke added 1 commit 2023-08-02 21:12:06 +02:00

buildbot/vexp-code-patch-coordinator Build done. Details

85d6865f17 cleanup

Jacques Lucke commented

2023-08-02 21:12:27 +02:00

@blender-bot build

Jacques Lucke added 3 commits 2023-08-02 21:47:26 +02:00

4279179f33 allow NOD_REGISTER_NODE to be in namespace

e3af2397f7 cleanup

buildbot/vexp-code-patch-coordinator Build done. Details

48fd5c206a fix

Jacques Lucke commented

2023-08-02 21:50:01 +02:00

@blender-bot build

Ray molenkamp commented

2023-08-02 22:55:39 +02:00

The code generation is done with a Python script, but that likely has to become a C++ file to be able to build without Python.

May sound strange but personally i'd have a strong lean towards python here, rather than C++. Beyond it being a more comfortable tool for the job, it also resists the urge of "ohh i just need this one function from blenkernel, I'll just add it as a quick dep" end you end up making both a dependency mess and a rather ....girthy... codegen tool (ie bf_gpu's shader builder) python sidesteps all of this nicely and it's better at text processing than C++ would be. We'd have to check with core, see how they feel about this. But i'm not ruling out python use here, would actively encourage it.

Some small notes, somewhat tying into "keeping the build times down"

Don't refresh the file if it hasn't changed, for other codegen tools like makesrna we write to a temp file, compare the files and only replace the file if it is different. This will prevent unneeded rebuilds.
If the files being generated could be in their own cmake target that be ideal, when cmake determines what needs to be minimally build when you build bf_something, it walks the dep tree and flags all targets with a codegen stage [1] and builds the whole dep, rather than just doing the codegen. So in this case, because register_geometry_nodes.cc lives inside bf_nodes_geometry all of bf_nodes_geometry will now be build if it shows up in the dependency tree somewhere.

More concretely when you currently have a clean build folder and on main run (-n to not actually build, since we just want to know if things are scheduled to build)

ninja -j1 -n bf_realtime_compositor

you'll notice bf_nodes_geometry will not be build

after applying this PR and running the same command, you'll notice this popping up in the build log (and all other unity compile units)

[1841/1893] Building CXX object source\blender\nodes\geometry\CMakeFiles\bf_nodes_geometry.dir\Unity\unity_14_cxx.cxx.obj

the problem there is....bf_nodes_geometry is a bit... heavy.... and it really doesn't need to build

making a small tweak like

diff --git a/source/blender/nodes/geometry/CMakeLists.txt b/source/blender/nodes/geometry/CMakeLists.txt
index 1a171fcb801..b0dda7d190e 100644
--- a/source/blender/nodes/geometry/CMakeLists.txt
+++ b/source/blender/nodes/geometry/CMakeLists.txt
@@ -268,13 +268,11 @@ add_custom_command(
     ${CMAKE_CURRENT_SOURCE_DIR}/detect_register_functions.py
 )

-list(APPEND SRC
-  ${CMAKE_CURRENT_BINARY_DIR}/register_geometry_nodes.cc
-)
+add_library(bf_nodes_geometry_generated ${CMAKE_CURRENT_BINARY_DIR}/register_geometry_nodes.cc)

-set_source_files_properties(
-  ${CMAKE_CURRENT_BINARY_DIR}/register_geometry_nodes.cc
-  PROPERTIES SKIP_UNITY_BUILD_INCLUSION ON)
+list(APPEND LIB
+  bf_nodes_geometry_generated
+)

 blender_add_lib(bf_nodes_geometry "${SRC}" "${INC}" "${INC_SYS}" "${LIB}")

and moving register_geometry_nodes.cc into its own project sidesteps this wonky behavior and only

[49/1876] Building CXX object source\blender\nodes\geometry\CMakeFiles\bf_nodes_geometry_generated.dir\register_geometry_nodes.cc.obj

will show up. Shaving a few minutes off a common "hey I have a compile error/main broke on platform X, can you take a look?" scenario.

There's other libs in blender with a codegen stage where this could be applied and get some more savings, but I have lacked the time sofar to do it.

[1] https://cmake.org/cmake/help/latest/prop_tgt/OPTIMIZE_DEPENDENCIES.html

> The code generation is done with a Python script, but that likely has to become a C++ file to be able to build without Python. May sound strange but personally i'd have a strong lean _towards_ python here, rather than C++. Beyond it being a more comfortable tool for the job, it also resists the urge of "ohh i just need this one function from blenkernel, I'll just add it as a quick dep" end you end up making both a dependency mess and a rather ....girthy... codegen tool (ie bf_gpu's shader builder) python sidesteps all of this nicely and it's better at text processing than C++ would be. We'd have to check with core, see how they feel about this. But i'm not ruling out python use here, would actively encourage it. Some small notes, somewhat tying into "keeping the build times down" 1) Don't refresh the file if it hasn't changed, for other codegen tools like makesrna we write to a temp file, compare the files and only replace the file if it is different. This will prevent unneeded rebuilds. 2) If the files being generated could be in their own cmake target that be ideal, when cmake determines what needs to be minimally build when you build `bf_something`, it walks the dep tree and flags all targets with a codegen stage [1] and builds the whole dep, rather than just doing the codegen. So in this case, because `register_geometry_nodes.cc` lives inside `bf_nodes_geometry` _all_ of `bf_nodes_geometry` will now be build if it shows up in the dependency tree somewhere. More concretely when you currently have a clean build folder and on `main` run (-n to not actually build, since we just want to know if things are scheduled to build) `ninja -j1 -n bf_realtime_compositor` you'll notice `bf_nodes_geometry` will not be build after applying this PR and running the same command, you'll notice this popping up in the build log (and all other unity compile units) `[1841/1893] Building CXX object source\blender\nodes\geometry\CMakeFiles\bf_nodes_geometry.dir\Unity\unity_14_cxx.cxx.obj` the problem there is....`bf_nodes_geometry` is a bit... heavy.... and it really doesn't need to build making a small tweak like ```diff diff --git a/source/blender/nodes/geometry/CMakeLists.txt b/source/blender/nodes/geometry/CMakeLists.txt index 1a171fcb801..b0dda7d190e 100644 --- a/source/blender/nodes/geometry/CMakeLists.txt +++ b/source/blender/nodes/geometry/CMakeLists.txt @@ -268,13 +268,11 @@ add_custom_command( ${CMAKE_CURRENT_SOURCE_DIR}/detect_register_functions.py ) -list(APPEND SRC - ${CMAKE_CURRENT_BINARY_DIR}/register_geometry_nodes.cc -) +add_library(bf_nodes_geometry_generated ${CMAKE_CURRENT_BINARY_DIR}/register_geometry_nodes.cc) -set_source_files_properties( - ${CMAKE_CURRENT_BINARY_DIR}/register_geometry_nodes.cc - PROPERTIES SKIP_UNITY_BUILD_INCLUSION ON) +list(APPEND LIB + bf_nodes_geometry_generated +) blender_add_lib(bf_nodes_geometry "${SRC}" "${INC}" "${INC_SYS}" "${LIB}") ``` and moving `register_geometry_nodes.cc` into its own project sidesteps this wonky behavior and only `[49/1876] Building CXX object source\blender\nodes\geometry\CMakeFiles\bf_nodes_geometry_generated.dir\register_geometry_nodes.cc.obj` will show up. Shaving a few minutes off a common "hey I have a compile error/main broke on platform X, can you take a look?" scenario. There's other libs in blender with a codegen stage where this could be applied and get some more savings, but I have lacked the time sofar to do it. [1] https://cmake.org/cmake/help/latest/prop_tgt/OPTIMIZE_DEPENDENCIES.html

Jacques Lucke commented

2023-08-02 23:01:48 +02:00

Thanks for the feedback, I'll definitely do what you suggested here then regarding avoiding rebuilds.

Would be nice if we could use Python here, but yeah we'd have to check with other devs how Blender is expected to behave when building without Python.

Thanks for the feedback, I'll definitely do what you suggested here then regarding avoiding rebuilds. Would be nice if we could use Python here, but yeah we'd have to check with other devs how Blender is expected to behave when building without Python.

Jacques Lucke added 7 commits 2023-08-03 00:11:01 +02:00

439e41d538 update all geometry nodes

dc60e762b1 generalize script a bit

bfde0a6b47 more generalization

e0e6e4891f cleanup code

a5b0252760 improve documentation

f88ed830b2 try making discovery mechanism more reusable

buildbot/vexp-code-patch-coordinator Build done. Details

c1eb1e7604 add cmake todo comment

Jacques Lucke commented

2023-08-03 00:12:08 +02:00

@blender-bot build

Jacques Lucke added 7 commits 2023-08-03 13:27:46 +02:00

144cd55fc4 Merge branch 'main' into auto-node-register

fc1f18d7f6 use source file list from cmake instead of discovering them in Python

9f61f049d9 use auto discovery for function nodes

f4da8e3c81 simplify function nodes registration

deb5dec629 add comment

d06323f230 cleanup

buildbot/vexp-code-patch-coordinator Build done. Details

ec88d9ac17 Merge branch 'main' into auto-node-register

Jacques Lucke commented

2023-08-03 13:28:02 +02:00

@blender-bot build

Jacques Lucke added 1 commit 2023-08-03 13:55:26 +02:00

1e26907eb2 don't overwrite generated file when the output has not changed

Jacques Lucke commented

2023-08-03 13:58:26 +02:00

@LazyDodo I wonder if this build failure is caused by moving the register functions into a different library: https://builder.blender.org/admin/#/builders/132/builds/1839
The library doesn't find the symbols because it does not depend on them (would result in a circular dependency I think).

@LazyDodo I wonder if this build failure is caused by moving the register functions into a different library: https://builder.blender.org/admin/#/builders/132/builds/1839 The library doesn't find the symbols because it does not depend on them (would result in a circular dependency I think).

Ray molenkamp commented

2023-08-04 23:12:54 +02:00

It's a link order issue, the generated code relies on functions in a different library, hence must declare it needs it to link

diff --git a/source/blender/nodes/function/CMakeLists.txt b/source/blender/nodes/function/CMakeLists.txt
index 1b110ec226b..7cf47758867 100644
--- a/source/blender/nodes/function/CMakeLists.txt
+++ b/source/blender/nodes/function/CMakeLists.txt
@@ -62,7 +62,7 @@ list(APPEND LIB


 blender_add_lib(bf_nodes_function "${SRC}" "${INC}" "${INC_SYS}" "${LIB}")
-
+target_link_libraries(bf_nodes_functions_generated bf_nodes_function)
 if(WITH_UNITY_BUILD)
   set_target_properties(bf_nodes_function PROPERTIES UNITY_BUILD ON)
   set_target_properties(bf_nodes_function PROPERTIES UNITY_BUILD_BATCH_SIZE 10)
diff --git a/source/blender/nodes/geometry/CMakeLists.txt b/source/blender/nodes/geometry/CMakeLists.txt
index a1e1f53daa1..9adfd70083f 100644
--- a/source/blender/nodes/geometry/CMakeLists.txt
+++ b/source/blender/nodes/geometry/CMakeLists.txt
@@ -270,6 +270,7 @@ list(APPEND LIB
 )

 blender_add_lib(bf_nodes_geometry "${SRC}" "${INC}" "${INC_SYS}" "${LIB}")
+target_link_libraries(bf_nodes_geometry_generated bf_nodes_geometry)

 if(WITH_UNITY_BUILD)
   set_target_properties(bf_nodes_geometry PROPERTIES UNITY_BUILD ON)

will do it, but it may be neater to pass the depended lib as an extra parameter to add_node_discovery so you don't have to scatter target_link_libraries all over the place

It's a link order issue, the generated code relies on functions in a different library, hence must declare it needs it to link ```diff diff --git a/source/blender/nodes/function/CMakeLists.txt b/source/blender/nodes/function/CMakeLists.txt index 1b110ec226b..7cf47758867 100644 --- a/source/blender/nodes/function/CMakeLists.txt +++ b/source/blender/nodes/function/CMakeLists.txt @@ -62,7 +62,7 @@ list(APPEND LIB blender_add_lib(bf_nodes_function "${SRC}" "${INC}" "${INC_SYS}" "${LIB}") - +target_link_libraries(bf_nodes_functions_generated bf_nodes_function) if(WITH_UNITY_BUILD) set_target_properties(bf_nodes_function PROPERTIES UNITY_BUILD ON) set_target_properties(bf_nodes_function PROPERTIES UNITY_BUILD_BATCH_SIZE 10) diff --git a/source/blender/nodes/geometry/CMakeLists.txt b/source/blender/nodes/geometry/CMakeLists.txt index a1e1f53daa1..9adfd70083f 100644 --- a/source/blender/nodes/geometry/CMakeLists.txt +++ b/source/blender/nodes/geometry/CMakeLists.txt @@ -270,6 +270,7 @@ list(APPEND LIB ) blender_add_lib(bf_nodes_geometry "${SRC}" "${INC}" "${INC_SYS}" "${LIB}") +target_link_libraries(bf_nodes_geometry_generated bf_nodes_geometry) if(WITH_UNITY_BUILD) set_target_properties(bf_nodes_geometry PROPERTIES UNITY_BUILD ON) ``` will do it, but it may be neater to pass the depended lib as an extra parameter to `add_node_discovery` so you don't have to scatter `target_link_libraries` all over the place

Jacques Lucke added 3 commits 2023-08-05 08:10:58 +02:00

ba0f83aae8 Merge branch 'main' into auto-node-register

559294b747 fix after merge

buildbot/vexp-code-patch-coordinator Build done. Details

bb61463915 use fix from LazyDodo

Jacques Lucke commented

2023-08-05 08:11:25 +02:00

@blender-bot build

Jacques Lucke added 1 commit 2023-08-08 15:31:10 +02:00

3f85cd05a2 Merge branch 'main' into auto-node-register

Jacques Lucke added 1 commit 2023-08-09 13:07:30 +02:00

buildbot/vexp-code-patch-coordinator Build done. Details

98f7ab48a0 Merge branch 'main' into auto-node-register

Jacques Lucke changed title from ~~WIP: Nodes: experiment with auto registration of nodes~~ to Nodes: use auto registration for nodes

2023-08-09 13:15:06 +02:00

Jacques Lucke commented

2023-08-09 13:16:11 +02:00

@blender-bot build

Jacques Lucke requested review from Hans Goudey 2023-08-09 13:22:35 +02:00

Jacques Lucke requested review from Lukas Tönne 2023-08-09 13:26:39 +02:00

Lukas Tönne approved these changes 2023-08-09 13:54:01 +02:00

Lukas Tönne left a comment

Looks ok to me. Not thrilled about the } // namespace way of finding namespace closing brackets, but can't think of a better way to do it, short of actually parsing C/C++ code.

Looks ok to me. Not thrilled about the `} // namespace` way of finding namespace closing brackets, but can't think of a better way to do it, short of actually parsing C/C++ code.

source/blender/nodes/intern/discover_nodes.py

						
				@ -0,0 +40,4 @@

				# Use a single regular expression to search for opening namespaces, closing namespaces

				# and macro invocations. This makes it easy to iterate over the matches in order.

				re_namespace_begin = r"^namespace ([\w:]+) \{"

Lukas Tönne commented

2023-08-09 13:44:10 +02:00

Technically i think this should be ([\w:]*) to also match namespace{. Our formatting makes sure there is always whitespace but i don't think we want to rely on that.

Then again this script also relies on the // namespace X part and its special formatting to find closing brackets, so i guess it's fine.

Technically i think this should be `([\w:]*)` to also match `namespace{`. Our formatting makes sure there is always whitespace but i don't think we want to rely on that. Then again this script also relies on the `// namespace X` part and its special formatting to find closing brackets, so i guess it's fine.

Lukas Tönne commented

2023-08-09 14:06:42 +02:00

C++ type_info contains the namespaces, but the name would need to be unmangled somehow:

namespace dummy {
namespace for_testing::yay {
struct WhatsMyName {
};
}  // namespace for_testing::yay
}  // namespace dummy

const std::type_info &info = typeid(dummy::for_testing::yay::WhatsMyName);
std::cout << info.name() << std::endl;

outputs: N5dummy11for_testing3yay11WhatsMyNameE

C++ type_info contains the namespaces, but the name would need to be unmangled somehow: ```cpp namespace dummy { namespace for_testing::yay { struct WhatsMyName { }; } // namespace for_testing::yay } // namespace dummy const std::type_info &info = typeid(dummy::for_testing::yay::WhatsMyName); std::cout << info.name() << std::endl; ``` outputs: `N5dummy11for_testing3yay11WhatsMyNameE`

Ray molenkamp commented

2023-08-09 17:39:07 +02:00

Mangling rules are undefined in the standard, every vendor is free to do whatever they want here, I'd recommend not going down this particular road for that reason.

llvm/clang is a soft dependency for us currently, if that were to change, we could easily build tools upon a real C++ parser, i've done so in the past to automatically add padding to dna where required.

https://archive.blender.org/developer/D9465

for now, i'd stick with that regex though, as this is a much larger conversation to have, and this pull isn't really the right venue for it

Mangling rules are undefined in the standard, every vendor is free to do whatever they want here, I'd recommend not going down this particular road for that reason. llvm/clang is a soft dependency for us currently, if that were to change, we could easily build tools upon a real C++ parser, i've done so in the past to automatically add padding to dna where required. https://archive.blender.org/developer/D9465 for now, i'd stick with that regex though, as this is a much larger conversation to have, and this pull isn't really the right venue for it

Jacques Lucke commented

2023-08-09 21:54:01 +02:00

I think it's fine if we rely on certain formatting, because that part is automated with clang format anyway.
Relying on std::type_info is indeed not really an option here unfortunately.

I think it's fine if we rely on certain formatting, because that part is automated with clang format anyway. Relying on `std::type_info` is indeed not really an option here unfortunately.

Hans Goudey approved these changes 2023-08-09 17:08:21 +02:00

Hans Goudey left a comment

This looks fairly straightforward actually! Definitely nicer this way.

Jacques Lucke merged commit 19912457c6 into main

2023-08-09 22:01:10 +02:00

Jacques Lucke deleted branch auto-node-register

2023-08-09 22:01:12 +02:00

Jacques Lucke referenced this issue from a commit

2023-08-09 22:01:12 +02:00

Nodes: use auto registration for nodes

Almaz-Shinbay referenced this issue from a commit

2023-08-10 18:10:38 +02:00

Nodes: use auto registration for nodes

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Nodes: use auto registration for nodes #110686