Geometry Nodes: refactor storage for socket values #116231

Jacques Lucke · 2023-12-15T19:41:39+01:00

Jacques Lucke commented

2023-12-15 19:41:39 +01:00

This refactors SocketValueVariant with the following goals in mind:

Support type erasure so that not all users of SocketValueVariant have to know about all the types sockets can have.
Move towards supporting "rainbow sockets" which are sockets whoose type is only known at run-time.
Reduce complexity when dealing with socket values in general. Previously, one had to use SocketValueVariantCPPType a lot to manage uninitialized memory. This is better abstracted away now.

One related change that I had to do that I didn't see coming at first was that I had to refactor set_default_remaining_outputs because now the default value of a SocketValueVariant would not contain any value. Previously, it was initialized the zero-value of the template parameter. Similarly, I had to change how implicit conversions are created, because comparing the CPPType of linked sockets was not enough anymore to determine if a conversion is necessary.

We could potentially use SocketValueVariant for the remaining socket types in the future as well. Not entirely sure if that helps yet. SocketValueVariant can easily be adapted to make that work though. That would also justify the name "SocketValueVariant" better.

This refactors `SocketValueVariant` with the following goals in mind: * Support type erasure so that not all users of `SocketValueVariant` have to know about all the types sockets can have. * Move towards supporting "rainbow sockets" which are sockets whoose type is only known at run-time. * Reduce complexity when dealing with socket values in general. Previously, one had to use `SocketValueVariantCPPType` a lot to manage uninitialized memory. This is better abstracted away now. One related change that I had to do that I didn't see coming at first was that I had to refactor `set_default_remaining_outputs` because now the default value of a `SocketValueVariant` would not contain any value. Previously, it was initialized the zero-value of the template parameter. Similarly, I had to change how implicit conversions are created, because comparing the `CPPType` of linked sockets was not enough anymore to determine if a conversion is necessary. We could potentially use `SocketValueVariant` for the remaining socket types in the future as well. Not entirely sure if that helps yet. `SocketValueVariant` can easily be adapted to make that work though. That would also justify the name "SocketValueVariant" better.

👍 1 🎉 1

Jacques Lucke added 20 commits 2023-12-15 19:41:48 +01:00

fc2838745e initial refactor

a8fd047adc progress

848bcbbff3 progrss

2a949c4584 progress

bb4ae6030e fix

44ea38f8d2 fix

fef42dffaa fixes

d91cec3cbe improve handling of default output

dc28c96b9f fix skip input

fd0dc6672c cleanup

fc98b9d285 improve error detection

43fd23290d progress

7d01828ecf progress

20d2d75dff cleanup

a768bcb12a cleanup

7941219936 improve comments

580cf52b36 cleanup

530dc6cc63 cleanup

73e6307356 docs

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

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

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

cf8ea423a5 cleanup

Jacques Lucke requested review from Lukas Tönne 2023-12-15 19:45:41 +01:00

Jacques Lucke requested review from Hans Goudey 2023-12-15 19:45:41 +01:00

Jacques Lucke commented

2023-12-15 19:50:40 +01:00

@blender-bot build

Hans Goudey reviewed 2023-12-16 02:25:23 +01:00

Hans Goudey left a comment

Looks great overall, especially how we can pass the type and the value with one variable now. Just a couple points inline.

Out of curiosity, did you test to see if there was a performance impact in some of the test files that emphasize per-node overhead?

Looks great overall, especially how we can pass the type and the value with one variable now. Just a couple points inline. Out of curiosity, did you test to see if there was a performance impact in some of the test files that emphasize per-node overhead?

source/blender/blenkernel/BKE_node_socket_value.hh

						
				@ -9,3 +9,3 @@

				#pragma once

				#include "FN_field.hh"

				#include "DNA_node_types.h"

Hans Goudey commented

2023-12-16 02:23:41 +01:00

DNA_node_types.h is a pretty large header, might be nice to avoid including it everywhere just for eNodeSocketDatatype. In practice it's probably already included nearly everywhere anyway, but in principle it's nice to avoid this. A few potential solutions for that I think:

Move these enums to blenkernel headers, use enum class at the same time
Support enum class :int8_t in DNA so we can use forward declarations.
Add a DNA_node_enums.h similar to other DNA headers (that's probably simplest to do for now)

Long as we agree on one of them, seems fine not to do anything for this PR :)

`DNA_node_types.h` is a pretty large header, might be nice to avoid including it everywhere just for `eNodeSocketDatatype`. In practice it's probably already included nearly everywhere anyway, but in principle it's nice to avoid this. A few potential solutions for that I think: - Move these enums to blenkernel headers, use `enum class` at the same time - Support `enum class :int8_t` in DNA so we can use forward declarations. - Add a `DNA_node_enums.h` similar to other DNA headers (that's probably simplest to do for now) Long as we agree on one of them, seems fine not to do anything for this PR :)

Jacques Lucke commented

2023-12-16 13:30:15 +01:00

I think the right solution is to split up dna headers.

👍 1

source/blender/blenkernel/BKE_node_socket_value.hh Outdated

						
				@ -15,2 +19,2 @@

				/* -------------------------------------------------------------------- */

				/** \name Value or Field Class

				/**

				 * #SocketValueVariant is used by geometry nodes to pass data between nodes. Specifically, it is

Hans Goudey commented

2023-12-16 00:45:04 +01:00

Maybe used by the lazy function evaluator (geometry nodes) to help justify the generic name of the file?

Maybe `used by the lazy function evaluator (geometry nodes)` to help justify the generic name of the file?

JacquesLucke marked this conversation as resolved

source/blender/blenkernel/BKE_node_socket_value.hh Outdated

						
				@ -64,0 +118,4 @@

				  bool is_context_dependent_field() const;

				  /**

				   * Cnvert the stored value into a single value. For simple value access, this is not necessary,

Hans Goudey commented

2023-12-16 00:50:10 +01:00

Cnvert -> Convert

`Cnvert` -> `Convert`

JacquesLucke marked this conversation as resolved

source/blender/blenlib/BLI_any.hh Outdated

						
				@ -315,3 +315,2 @@

				  {

				    BLI_assert(this->is<T>());

				    return *static_cast<std::decay_t<T> *>(this->get());

				    return const_cast<T &>(const_cast<const Any *>(this)->get<T>());

Hans Goudey commented

2023-12-16 00:42:34 +01:00

Would like to see some justification of the const casts in a comment, it's raising my eyebrows right now otherwise.

JacquesLucke marked this conversation as resolved

source/blender/nodes/NOD_geometry_nodes_lazy_function.hh Outdated

						
				@ -382,0 +382,4 @@

				/**

				 * Outputs the default value of each output socket that has not been output yet.

				 */

				void set_default_remaining_node_outputs(lf::Params &params, const bNode &node);

Hans Goudey commented

2023-12-16 01:08:28 +01:00

Why does this have to work at the bNode level now? I'd think it could just use the lazy function directly. Maybe that's what you're getting at in the description, but I don't see how that leads to this now.

Why does this have to work at the `bNode` level now? I'd think it could just use the lazy function directly. Maybe that's what you're getting at in the description, but I don't see how that leads to this now.

👍 1

Jacques Lucke commented

2023-12-16 13:35:46 +01:00

/**
 * Outputs the default value of each output socket that has not been output yet. This needs the
 * #bNode because otherwise the default values for the outputs are not known. The lazy-function
 * parameters do not differentiate between e.g. float and vector sockets. The #SocketValueVariant
 * type is used for both.
 */

``` /** * Outputs the default value of each output socket that has not been output yet. This needs the * #bNode because otherwise the default values for the outputs are not known. The lazy-function * parameters do not differentiate between e.g. float and vector sockets. The #SocketValueVariant * type is used for both. */ ```

👍 1

Jacques Lucke added 6 commits 2023-12-16 13:36:16 +01:00

2601262a4c Merge branch 'main' into generic-socket-value-variant

aacf15e666 optimize socket type to cpp type conversion

3892ff5a34 improve comments

166821c186 add const cast comment

d1ee9c43af improve comment

b2290dd52f typo

Jacques Lucke commented

2023-12-16 13:41:06 +01:00

Out of curiosity, did you test to see if there was a performance impact in some of the test files that emphasize per-node overhead?

Right now I'm measuring about 5% slowdown in the many math nodes benchmark (with ~400k math node evaluations on single values). The seems to be mostly caused by conversions between socket and cpp types. I think we can probably optimize those away for the most part by spending some time optimizing the individual uses of get_single_ptr and new_single_for_write.

> Out of curiosity, did you test to see if there was a performance impact in some of the test files that emphasize per-node overhead? Right now I'm measuring about 5% slowdown in the many math nodes benchmark (with ~400k math node evaluations on single values). The seems to be mostly caused by conversions between socket and cpp types. I think we can probably optimize those away for the most part by spending some time optimizing the individual uses of `get_single_ptr` and `new_single_for_write`.

Jacques Lucke added 1 commit 2023-12-16 13:41:24 +01:00

b188e643d9 fix

Hans Goudey approved these changes 2023-12-16 14:46:51 +01:00

Jacques Lucke merged commit e1d0d70911 into main

2023-12-17 14:00:17 +01:00

Jacques Lucke referenced this issue from a commit

2023-12-17 14:00:18 +01:00

Geometry Nodes: refactor storage for socket values

Jacques Lucke deleted branch generic-socket-value-variant

2023-12-17 14:00:19 +01:00

Brian Savery (AMD) referenced this issue from a commit

2023-12-28 20:07:45 +01:00