Geometry Nodes: deduplicate code to deal with dynamic socket amounts #113114

Jacques Lucke · 2023-10-01T14:47:24+02:00

Jacques Lucke commented

2023-10-01 14:47:24 +02:00

The goal of this refactor is to reduce the amount of boilerplate code that is necessary to have a dynamic number of sockets on nodes. This is achieved by making the code more reusable. Currently, only the simulation and repeat zone nodes make use of this. However, even with just those two, the amount of code is reduced already. The benefit of this refactor will become even more significant as more nodes support a dynamic number of sockets. For example, the bake node and for-each zone will also benefit from this.

We could probably make some of the utility functions non-templates using type erasure. This could reduce the compilation overhead when the number of nodes with item arrays increases. The main reason for why everything is templated now is that it made this refactor easier. Without this patch, all the code was essentially "manually templated". So the implementations look still similar to before now, just that concrete types are replaced with template parameters.

No user-visible changes are expected.

The goal of this refactor is to reduce the amount of boilerplate code that is necessary to have a dynamic number of sockets on nodes. This is achieved by making the code more reusable. Currently, only the simulation and repeat zone nodes make use of this. However, even with just those two, the amount of code is reduced already. The benefit of this refactor will become even more significant as more nodes support a dynamic number of sockets. For example, the bake node and for-each zone will also benefit from this. We could probably make some of the utility functions non-templates using type erasure. This could reduce the compilation overhead when the number of nodes with item arrays increases. The main reason for why everything is templated now is that it made this refactor easier. Without this patch, all the code was essentially "manually templated". So the implementations look still similar to before now, just that concrete types are replaced with template parameters. No user-visible changes are expected.

👍 1

Jacques Lucke added 29 commits 2023-10-01 14:47:39 +02:00

16db95d3f0 move simulation item removal to rna

0b76d0a19a deduplicate deletion code

4da0390f34 deduplicate more code

41eb432173 cleanup

1a5efd3d4d deduplicate more code

002aae6275 cleanup

ef3babaeb6 more deduplication

b11376ed97 cleanup

813e2815ce deduplicate

9c4bc764a4 deduplicate more code

482d4e00fe cleanup

8cf1672a2c cleanup

e5b445f409 progress

17b1082014 progress

f956bf66a7 cleanup

f4fb5b15d9 progress

a976556829 start moving to header

f26fab9e36 progress

342de07b79 move more to header

91995bd24c progress

8c9463efc0 progress

f2d9401d34 progress

f9259e3b00 progress

5405bb956f progress

4e4bbd82c4 remove dead code

3f948e54d0 progress

a41dcc6cd8 cleanup

5087b779ea progress

f1e46076da cleanup

Jacques Lucke added 1 commit 2023-10-01 14:48:18 +02:00

4f1abd57d6 improve name

Jacques Lucke added 7 commits 2023-10-01 15:51:53 +02:00

Jacques Lucke added 3 commits 2023-10-01 16:26:19 +02:00

f8dcfed956 remove using namespace

b7ad1e5e4d improve naming

d478bb813c add comments

Jacques Lucke requested review from Lukas Tönne 2023-10-01 16:54:18 +02:00

Jacques Lucke requested review from Hans Goudey 2023-10-01 16:54:19 +02:00

Jacques Lucke changed title from ~~WIP: Geometry Nodes: deduplicate item arrays code~~ to WIP: Geometry Nodes: deduplicate code to deal with dynamic socket amounts

2023-10-01 16:54:43 +02:00

Jacques Lucke changed title from ~~WIP: Geometry Nodes: deduplicate code to deal with dynamic socket amounts~~ to Geometry Nodes: deduplicate code to deal with dynamic socket amounts

2023-10-01 22:51:59 +02:00

Jacques Lucke referenced this pull request

2023-10-01 22:54:00 +02:00

RNA: support passing template arguments to functions #113121

Jacques Lucke added 1 commit 2023-10-01 22:57:56 +02:00

9e133fc124 improve function names

Jacques Lucke referenced this issue from a commit

2023-10-01 23:24:32 +02:00

RNA: support passing template arguments to functions

Jacques Lucke added 1 commit 2023-10-01 23:28:04 +02:00

cc71275608 Merge branch 'main' into simplify-item-array-handling

Jacques Lucke added 1 commit 2023-10-01 23:45:23 +02:00

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

1bddd378cf remove more rna redundancy

Jacques Lucke commented

2023-10-01 23:51:28 +02:00

@blender-bot build

Jacques Lucke referenced this issue from a commit

2023-10-02 12:17:11 +02:00

RNA: don't generate declarations for templated functions

Jacques Lucke added 2 commits 2023-10-02 12:20:12 +02:00

9d05e3f652 Merge branch 'main' into simplify-item-array-handling

73fb8e323c simplify code further

Jacques Lucke added 5 commits 2023-10-02 13:10:17 +02:00

7439ca0169 deduplicate more code

3c29d5266d progress

63d1763382 reduce redundancy

e300d4c9d6 deduplicate more rna code

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

4b45419fa7 reorder code

Hans Goudey reviewed 2023-10-02 13:47:45 +02:00

Hans Goudey left a comment

I guess my main comment is that it seems weird to have a "item array" abstraction that's specific to nodes. That confuses the design a bit IMO, since that concept is related to everything DNA/RNA, not just nodes. But then there are node-specific things mixed in right now.

It does seem like there's value in all of these abstractions/utilities, but maybe they could be organized a bit differently:

DNA C Array wrapper thing (maybe this isn't the long term design, but it makes things easier now)
RNA utilities for resizing/adding/clearing while maintaining unique names and active indices
Node utilities that use that code for the repeat and simulation zones

I guess my other larger comment is that it seems worth considering using the CPPType system to avoid the need for templates in lots of these cases.

I guess my main comment is that it seems weird to have a "item array" abstraction that's specific to nodes. That confuses the design a bit IMO, since that concept is related to everything DNA/RNA, not just nodes. But then there are node-specific things mixed in right now. It does seem like there's value in all of these abstractions/utilities, but maybe they could be organized a bit differently: 1. DNA C Array wrapper thing (maybe this isn't the long term design, but it makes things easier now) 2. RNA utilities for resizing/adding/clearing while maintaining unique names and active indices 3. Node utilities that use that code for the repeat and simulation zones I guess my other larger comment is that it seems worth considering using the `CPPType` system to avoid the need for templates in lots of these cases.

👍 1

Jacques Lucke commented

2023-10-02 14:06:17 +02:00

I guess my main comment is that it seems weird to have a "item array" abstraction that's specific to nodes. That confuses the design a bit IMO, since that concept is related to everything DNA/RNA, not just nodes. But then there are node-specific things mixed in right now.

I agree. I think we should eventually split the abstraction up more in the future, but I don't think we can't really do that without testing that in a smaller scope first to figure out what works well and what doesn't. For now the abstraction solves the problem that we have. If our problem changes we can change the abstraction with it.

I guess my other larger comment is that it seems worth considering using the CPPType system to avoid the need for templates in lots of these cases.

Not sure if you read my patch description, but I mention that. The thing is that right now the patch is mostly just deduplicating code that exists which was essentially templated (manually instantiated) already. I could look into removing templates from some functions and using something like CPPType separately. One thing that holds me back with using CPPType or other C++ wrappers is that we currently treat our DNA structs in ways that aren't super compatible with C++ objects. E.g. we relocate them with memcpy a lot, but that's not really a concept that C++ has out of the box. Instead it uses move-semantics which we don't have for DNA structs and likely don't want to implement.

DNA C Array wrapper thing (maybe this isn't the long term design, but it makes things easier now)

RNA utilities for resizing/adding/clearing while maintaining unique names and active indices

Node utilities that use that code for the repeat and simulation zones

I did try a fair amount of different refactors already, including the DNA C array wrapper. What I have in this patch is by far the cleanest cleanup I could come up with so far. I think it also makes testing the other things you mention much easier to try.

> I guess my main comment is that it seems weird to have a "item array" abstraction that's specific to nodes. That confuses the design a bit IMO, since that concept is related to everything DNA/RNA, not just nodes. But then there are node-specific things mixed in right now. I agree. I think we should eventually split the abstraction up more in the future, but I don't think we can't really do that without testing that in a smaller scope first to figure out what works well and what doesn't. For now the abstraction solves the problem that we have. If our problem changes we can change the abstraction with it. > I guess my other larger comment is that it seems worth considering using the `CPPType` system to avoid the need for templates in lots of these cases. Not sure if you read my patch description, but I mention that. The thing is that right now the patch is mostly just deduplicating code that exists which was essentially templated (manually instantiated) already. I could look into removing templates from some functions and using something like `CPPType` separately. One thing that holds me back with using `CPPType` or other C++ wrappers is that we currently treat our DNA structs in ways that aren't super compatible with C++ objects. E.g. we relocate them with memcpy a lot, but that's not really a concept that C++ has out of the box. Instead it uses move-semantics which we don't have for DNA structs and likely don't want to implement. > 1. DNA C Array wrapper thing (maybe this isn't the long term design, but it makes things easier now) > 2. RNA utilities for resizing/adding/clearing while maintaining unique names and active indices > 3. Node utilities that use that code for the repeat and simulation zones I did try a fair amount of different refactors already, including the DNA C array wrapper. What I have in this patch is by far the cleanest cleanup I could come up with so far. I think it also makes testing the other things you mention much easier to try.

Jacques Lucke commented

2023-10-02 14:10:23 +02:00

@blender-bot build

Jacques Lucke commented

2023-10-02 14:27:43 +02:00

I think even with the other refactors you mention it seems like a good idea to have a central place where all the behavior of a specific node items array is defined (e.g. for simulation zones). That doesn't mean that all code that deals with the item arrays has to be templated on the same struct going forward. Code that deals with a subset of the behavior of item arrays could also be made to just depend on that subset in the future.

Doing that doesn't seem too hard but I think it would be premature abstraction before we don't have more code that could make use of the abtraction. For the nodes that I'll work on soonish (for-each, bake), the abstraction in this patch seems like exactly what I need.

I think even with the other refactors you mention it seems like a good idea to have a central place where all the behavior of a specific node items array is defined (e.g. for simulation zones). That doesn't mean that all code that deals with the item arrays has to be templated on the same struct going forward. Code that deals with a subset of the behavior of item arrays could also be made to just depend on that subset in the future. Doing that doesn't seem too hard but I think it would be premature abstraction before we don't have more code that could make use of the abtraction. For the nodes that I'll work on soonish (for-each, bake), the abstraction in this patch seems like exactly what I need.

Hans Goudey commented

2023-10-02 21:47:23 +02:00

I agree. I think we should eventually split the abstraction up more in the future, but I don't think we can't really do that without testing that in a smaller scope first to figure out what works well and what doesn't. For now the abstraction solves the problem that we have. If our problem changes we can change the abstraction with it.

I hear your point, but even if it's just temporary, it's hard for me to get behind a class like nodes::item_arrays::ItemArrayRef. It's just confusing, because there's nothing specific to nodes in part of it, but then there is. It's just confusing organization. And as for the process of improving things, I agree it's nice to test things at a smaller scale, but that doesn't mean it has to be specific to nodes by design at the beginning. As an alternative, I would suggest discussing this a bit with other people interested in RNA/C++ (Sergey/Brecht/Campbell?) and agree to test this as an initial solution. Otherwise the risk is that we use this, it solves some local problem, and it just starts spreading without any proper discussion.

As a side-point, the name "Item Array" isn't super great. I wonder if something like "CArrayRef" of "TrivialArrayRef" is clearer. Every array is an "item array", so the word "item" doesn't really help, except maybe when the concept of "node interface item" gets involved, but even there the word "item" hardly pulls its weight.

One thing that holds me back with using CPPType or other C++ wrappers is that we currently treat our DNA structs in ways that aren't super compatible with C++ objects. E.g. we relocate them with memcpy a lot, but that's not really a concept that C++ has out of the box. Instead it uses move-semantics which we don't have for DNA structs and likely don't want to implement.

This is one case where CPPType could help. It gives us the opportunity to add this C-like dumb shallow copy into C++ code, and add some assets and checks to make it a bit safer.

> I agree. I think we should eventually split the abstraction up more in the future, but I don't think we can't really do that without testing that in a smaller scope first to figure out what works well and what doesn't. For now the abstraction solves the problem that we have. If our problem changes we can change the abstraction with it. I hear your point, but even if it's just temporary, it's hard for me to get behind a class like `nodes::item_arrays::ItemArrayRef`. It's just confusing, because there's nothing specific to nodes in part of it, but then there is. It's just confusing organization. And as for the process of improving things, I agree it's nice to test things at a smaller scale, but that doesn't mean it has to be specific to nodes _by design_ at the beginning. As an alternative, I would suggest discussing this a bit with other people interested in RNA/C++ (Sergey/Brecht/Campbell?) and agree to test this as an initial solution. Otherwise the risk is that we use this, it solves some local problem, and it just starts spreading without any proper discussion. As a side-point, the name "Item Array" isn't super great. I wonder if something like "CArrayRef" of "TrivialArrayRef" is clearer. Every array is an "item array", so the word "item" doesn't really help, except maybe when the concept of "node interface item" gets involved, but even there the word "item" hardly pulls its weight. > One thing that holds me back with using CPPType or other C++ wrappers is that we currently treat our DNA structs in ways that aren't super compatible with C++ objects. E.g. we relocate them with memcpy a lot, but that's not really a concept that C++ has out of the box. Instead it uses move-semantics which we don't have for DNA structs and likely don't want to implement. This is one case where `CPPType` could help. It gives us the opportunity to add this C-like dumb shallow copy into C++ code, and add some assets and checks to make it a bit safer.

Jacques Lucke commented

2023-10-02 22:52:27 +02:00

I guess, I'm mostly concerned that opening up the larger discussion will stall this patch for an unknown amount of time without acknowledging that what we have in this patch is significantly more maintainable and extensible than what we have in main now (let me know if you don't agree with that part). It doesn't feel like for the larger picture with DNA/RNA it makes a significant difference whether this patch is in main or not.

Right now, I'm undetermined whether I should make this patch less general and more specific to nodes by renaming a few things (e.g. ItemArrayRef to SocketItemsRef), or whether I should try to add more general abstractions. What do you think is the most productive way going forward to be able to implement the features we want to implement? I definitely don't want to add all the boilerplate another three times for the bake/foreach-input/foreach-output nodes.

RNA utilities for resizing/adding/clearing while maintaining unique names and active indices

Note that this stuff is also not fully general, because different arrays have different update callbacks and need to maintain different invariants (like unique names, socket identifiers, supported socket types, ...). So most arrays will likely still need some special code of RNA.

I wonder if something like "CArrayRef" of "TrivialArrayRef" is clearer.

The problem with those names is that CArray could mean many different things, but usually it's used to refer to something like int[10]. TrivialArrayRef is also difficult because while the underlying type may be trivial, the "real" type is neither trivially copyable nor destructible.

Every array is an "item array", so the word "item" doesn't really help

I use the term "item", because we use the names SimulationItem and RepeatItem. So maybe if I rename everything to "socket items" instead of "item array", things become clearer and less general for now.

I would suggest discussing this a bit with other people interested in RNA/C++ (Sergey/Brecht/Campbell?) and agree to test this as an initial solution.

I can surely discuss this with them but it still feels like something independent of this patch, since here I'm mostly concerned about code deduplication for dynamic number of sockets and not so much how to change DNA/RNA to integrate better with C++. Conflating the two topics doesn't seem useful. Maybe it's my fault that these topics are conflated now due to how I formulated the patch description.

This is one case where CPPType could help. It gives us the opportunity to add this C-like dumb shallow copy into C++ code, and add some assets and checks to make it a bit safer.

Not quite sure what kinds of asserts you mean here. Maybe you can give an example?

I guess, I'm mostly concerned that opening up the larger discussion will stall this patch for an unknown amount of time without acknowledging that what we have in this patch is significantly more maintainable and extensible than what we have in `main` now (let me know if you don't agree with that part). It doesn't feel like for the larger picture with DNA/RNA it makes a significant difference whether this patch is in `main` or not. Right now, I'm undetermined whether I should make this patch less general and more specific to nodes by renaming a few things (e.g. `ItemArrayRef` to `SocketItemsRef`), or whether I should try to add more general abstractions. What do you think is the most productive way going forward to be able to implement the features we want to implement? I definitely don't want to add all the boilerplate another three times for the bake/foreach-input/foreach-output nodes. > 2. RNA utilities for resizing/adding/clearing while maintaining unique names and active indices Note that this stuff is also not fully general, because different arrays have different update callbacks and need to maintain different invariants (like unique names, socket identifiers, supported socket types, ...). So most arrays will likely still need some special code of RNA. > I wonder if something like "CArrayRef" of "TrivialArrayRef" is clearer. The problem with those names is that `CArray` could mean many different things, but usually it's used to refer to something like `int[10]`. `TrivialArrayRef` is also difficult because while the underlying type may be trivial, the "real" type is neither trivially copyable nor destructible. > Every array is an "item array", so the word "item" doesn't really help I use the term "item", because we use the names `SimulationItem` and `RepeatItem`. So maybe if I rename everything to "socket items" instead of "item array", things become clearer and less general for now. > I would suggest discussing this a bit with other people interested in RNA/C++ (Sergey/Brecht/Campbell?) and agree to test this as an initial solution. I can surely discuss this with them but it still feels like something independent of this patch, since here I'm mostly concerned about code deduplication for dynamic number of sockets and not so much how to change DNA/RNA to integrate better with C++. Conflating the two topics doesn't seem useful. Maybe it's my fault that these topics are conflated now due to how I formulated the patch description. > This is one case where CPPType could help. It gives us the opportunity to add this C-like dumb shallow copy into C++ code, and add some assets and checks to make it a bit safer. Not quite sure what kinds of asserts you mean here. Maybe you can give an example?

Hans Goudey commented

2023-10-02 23:03:35 +02:00

Right now, I'm undetermined whether I should make this patch less general and more specific to nodes by renaming a few things (e.g. ItemArrayRef to SocketItemsRef),

Thanks for summing it up constructively like that. I think making things more clearly node-specific would help (while still organizing the internal code so that it's "generalize-able" later if we want to.

I guess I was coming at this with the perspective of it being a continuation of the discussion from the devtalk thread. Considering it as a local thing changes that, but indeed, more specific naming would really help.

Not quite sure what kinds of asserts you mean here. Maybe you can give an example?

I meant something stupidly simple like:

void CPPType::shallow_copy(const void *src, void *dst)
{
  BLI_assert(this->is_trivial());
  ...
}

>Right now, I'm undetermined whether I should make this patch less general and more specific to nodes by renaming a few things (e.g. `ItemArrayRef` to `SocketItemsRef`), Thanks for summing it up constructively like that. I think making things more clearly node-specific would help (while still organizing the internal code so that it's "generalize-able" later if we want to. I guess I was coming at this with the perspective of it being a continuation of the discussion from the devtalk thread. Considering it as a local thing changes that, but indeed, more specific naming would really help. >Not quite sure what kinds of asserts you mean here. Maybe you can give an example? I meant something stupidly simple like: ``` void CPPType::shallow_copy(const void *src, void *dst) { BLI_assert(this->is_trivial()); ... } ```

Jacques Lucke added 4 commits 2023-10-03 12:10:43 +02:00

02c743dcbf Merge branch 'main' into simplify-item-array-handling

3a3c39710b rename files

012a719d4f rename more

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

93b8761a16 improve naming

Hans Goudey approved these changes 2023-10-03 23:36:55 +02:00

Hans Goudey left a comment

Very nice now!

source/blender/makesrna/intern/rna_nodetree.cc Outdated

						
				@ -8916,0 +8810,4 @@

				  SNPRINTF(name, "rna_Node_ItemArray_new_with_socket_and_name<%s>", accessor_name);

				  func = RNA_def_function(srna, "new", allocator.copy_string(name).c_str());

				  RNA_def_function_ui_description(func, "Add a item at the end");

Hans Goudey commented

2023-10-03 23:28:40 +02:00

a item -> an item

`a item` -> `an item`

JacquesLucke marked this conversation as resolved

source/blender/nodes/NOD_socket_items.hh Outdated

						
				@ -0,0 +211,4 @@

				}

				/**

				 * Check if the links connects to the `extend_socket`. If yes, create a new item for the linked

Hans Goudey commented

2023-10-03 23:33:54 +02:00

the links connects -> the link connects

`the links connects` -> `the link connects`

JacquesLucke marked this conversation as resolved

source/blender/nodes/intern/node_zone_socket_items.cc Outdated

						
				@ -0,0 +11,4 @@

				namespace blender::nodes {

				StructRNA *SimulationItemsAccessor::item_srna = &RNA_SimulationStateItem;

				int SimulationItemsAccessor::node_type = GEO_NODE_SIMULATION_OUTPUT;