Page MenuHome
Differential D15491

Nodes: Move NodeTreeRef functionality into node runtime data.
ClosedPublic
Actions

Authored by Jacques Lucke (JacquesLucke) on Jul 19 2022, 11:08 AM.

Details

Reviewers
Hans Goudey (HooglyBoogly)
Omar Emara (OmarSquircleArt)
Campbell Barton (campbellbarton)
Sergey Sharybin (sergey)
Commits
rB25e307d725d0: Nodes: move NodeTreeRef functionality into node runtime data
Summary

The purpose of NodeTreeRef was to speed up various queries on a read-only bNodeTree. Now that we have runtime data in nodes and sockets, we can also store the result of some queries there. This has some benefits:

  • No need for a read-only separate node tree data structure which increased complexity.
  • Makes it easier to make reuse cached queries in more parts of Blender that can benefit from it.

A downside is that we loose some type safety that we got by having different types for input and output sockets, as well as internal and non-internal links.

This patch refactors DerivedNodeTree so that it does not use NodeTreeRef anymore but uses bNodeTree directly.

To provide a convenient API (that also somewhat close to what NodeTreeRef had), a new approach is implemented: bNodeTree, bNode, bNodeSocket and bNodeLink now have C++ methods declared in DNA_node_types.h which are implemented in BKE_node_runtime.hh. To make this work, I had to change makesdna a bit to skip the C++ methods when parsing the dna header files. While this approach is not entirely clean, I couldn't think of a way to make the API work well without this yet.
In the future we may want to split DNA C structures more from C++ types (e.g. like we did with CurvesGeometry), but that should be done separately.

No user visible changes are expected.

Diff Detail

Repository
rB Blender
Branch
remove-node-tree-ref (branched from master)
Build Status
Buildable 23451
Build 23451: arc lint + arc unit

Event Timeline

Jacques Lucke (JacquesLucke) requested review of this revision.Jul 19 2022, 11:08 AM
Jacques Lucke (JacquesLucke) created this revision.
Harbormaster completed remote builds in B23058: Diff 53794.Jul 19 2022, 11:08 AM
Jacques Lucke (JacquesLucke) edited the summary of this revision. (Show Details)Jul 19 2022, 11:18 AM
Jacques Lucke (JacquesLucke) added subscribers: Hans Goudey (HooglyBoogly), Omar Emara (OmarSquircleArt).
Jacques Lucke (JacquesLucke) edited the summary of this revision. (Show Details)Jul 19 2022, 11:21 AM
Jacques Lucke (JacquesLucke) edited the summary of this revision. (Show Details)Jul 19 2022, 11:23 AM
Jacques Lucke (JacquesLucke) updated this revision to Diff 54839.Aug 20 2022, 8:45 PM
  • Merge branch 'master' into remove-node-tree-ref
  • use c++ methods in dna
Harbormaster completed remote builds in B23410: Diff 54839.Aug 20 2022, 8:45 PM
Jacques Lucke (JacquesLucke) edited the summary of this revision. (Show Details)Aug 20 2022, 9:09 PM
Jacques Lucke (JacquesLucke) updated this revision to Diff 54859.Aug 22 2022, 12:07 AM
  • Merge branch 'master' into remove-node-tree-ref
  • cleanup
Harbormaster completed remote builds in B23421: Diff 54859.Aug 22 2022, 12:07 AM
Jacques Lucke (JacquesLucke) retitled this revision from Nodes: Move NodeTreeRef functionality into node runtime data. (WIP) to Nodes: Move NodeTreeRef functionality into node runtime data..Aug 22 2022, 12:16 AM
Jacques Lucke (JacquesLucke) edited the summary of this revision. (Show Details)
Jacques Lucke (JacquesLucke) added reviewers: Hans Goudey (HooglyBoogly), Omar Emara (OmarSquircleArt), Campbell Barton (campbellbarton), Sergey Sharybin (sergey).

https://builder.blender.org/admin/#/builders/18/builds/626

Jacques Lucke (JacquesLucke) edited the summary of this revision. (Show Details)Aug 22 2022, 12:17 AM
Omar Emara (OmarSquircleArt) accepted this revision.Aug 22 2022, 10:30 AM

Realtime compositor changes looks good.

Jacques Lucke (JacquesLucke) updated this revision to Diff 54909.Aug 23 2022, 12:43 PM
  • Merge branch 'master' into remove-node-tree-ref
Harbormaster completed remote builds in B23451: Diff 54909.Aug 23 2022, 12:43 PM
Hans Goudey (HooglyBoogly) added a comment.EditedAug 24 2022, 11:15 PM

Generally I like the direction this is going. Getting rid of an unnecessary intermediate data structure is great, and this generic logic has the potential to simplify other code.

However, I'm still a bit hesitant about this way of adding C++ methods to our DNA structs.
If we want to end up using DNA just for IO (that seems like a nice idea), we might not want to define Blender's logic there.
A better approach might be separating blenkernel and DNA structs. blenkernel types might even have methods like .serialize(blo::Writer &writer) which could write in the DNA format or another.
In the short term that approach would look more like bke::CurvesGeometry does currently. It would significantly expand this patch though.

Maybe I'm over-complicating this, but I think it's worth having a solid plan since this will set precedent we can use elsewhere.
Anyway, I'm curious what you think about that.

source/blender/blenkernel/intern/node_runtime.cc
1

What about calling this file node_topology_cache.cc? I think being more specific would be an improvement-- "runtime" is a bit vague.

15–33

We could use this in quite a few places (curves code especially) if it was in blenlib. Could be a separate commit though I suppose.

source/blender/blenkernel/intern/node_tree_update.cc
53

It seems like this should only be set for tags that actually change the topology. For example, changing a default value shouldn't make the topology cache dirty.

In the future we could benefit from that by sharing the topology cache between the original and evaluated node tree in some cases, avoiding unnecessary rebuilding of the cache while editing nodes.

source/blender/makesdna/DNA_node_types.h
626

More granularity might be helpful. For example, it would be nice to use the spans when just iterating over the nodes, but the toposorts or type maps might not always be necessary.

We could wait until we see a performance bottleneck though, I'd be fine with that.

Jacques Lucke (JacquesLucke) planned changes to this revision.Aug 25 2022, 3:36 PM

Will try to use the same approach as with blender::bke::CurvesGeometry.

Jacques Lucke (JacquesLucke) added a comment.Aug 25 2022, 5:32 PM

Will try to use the same approach as with blender::bke::CurvesGeometry.

That didn't work out as well as we hoped. For CurvesGeometry the situation was much simpler, because it was a new type that wasn't used before. The bNode* types are used in many places, both in C and C++ code. Currently, a lot of casts would have to be inserted to make it compile, reducing readability and ease-of-use of the API quite significantly. Separating the C struct from the C++ type (as is done with CurvesGeometry) will be much easier as all related files (except for rna maybe) are in C++ in proper namespaces. Then almost all code would just use the C++ types, and no casts are necessary.

source/blender/blenkernel/intern/node_runtime.cc
1

I'm not sure if I'll stick to the term "topology" here longer term. Not all caches need to be related to topology. Could rename the file, but I also don't find the current name too bad.

15–33

Yeah, added this when I still had multiple separate caches, but that wasn't really worth it yet and also makes some things more complicated.

source/blender/blenkernel/intern/node_tree_update.cc
53

Yeah, guess that's mostly a problem with the name "topology" right now. Wanted to go the safe route for now and always invalidate the cache.

> In the future we could benefit from that by sharing the topology cache between the original and evaluated node tree in some cases, avoiding unnecessary rebuilding of the cache while editing nodes.

That will be difficult since the pointers are all different on the COW copy.

source/blender/makesdna/DNA_node_types.h
626

Yeah, I started out with that, but found it hard to properly split the cache up. I found that in many cases all caches are used anyway, so splitting it up wouldn't provide a benefit. Also, the caches often depend on each other, making the interface less clear (some ensure_* methods also ensure other caches, while others don't, that's difficult to document).
Anyway, definitely something we can improve later on when performance is an issue.

Hans Goudey (HooglyBoogly) accepted this revision.Aug 25 2022, 5:43 PM

The next steps seem to be making sure all users of BKE_node.h are in C++, then using namespaces. Since we're already gradually working on that anyway, it seems like a good plan.
The commit message should mention that declaring the methods in DNA isn't the long term goal, but other than that, the patch seems good to go to me.

Jacques Lucke (JacquesLucke) requested review of this revision.Aug 25 2022, 6:07 PM
Jacques Lucke (JacquesLucke) edited the summary of this revision. (Show Details)

@Campbell Barton (campbellbarton) Can you have a look at the change in makesdna?

Iliya Katueshenock (Moder) mentioned this in D15793: Fix T100627: Update frame node based on cache..Aug 26 2022, 11:03 PM
Paul Larson (GeorgiaPacific) added a subscriber: Paul Larson (GeorgiaPacific).Aug 29 2022, 6:43 PM
Jacques Lucke (JacquesLucke) updated this revision to Diff 55105.Aug 30 2022, 1:08 PM
  • Merge branch 'master' into remove-node-tree-ref
Harbormaster completed remote builds in B23539: Diff 55105.Aug 30 2022, 1:08 PM
Campbell Barton (campbellbarton) accepted this revision.Aug 31 2022, 11:30 AM

Regarding makesdna, while it's fine to add begin/end checks - any preprocessor indentation will cause problems (if there is ever nested checks), this patch supports arbitrary space after the # P3160.
I think it's better to support this so changes in the future don't break DNA parsing.

Besides this, ran into a warning when building the patch.

source/blender/nodes/shader/nodes/node_shader_mix.cc: In function ‘void blender::nodes::node_sh_mix_cc::sh_node_mix_build_multi_function(blender::nodes::NodeMultiFunctionBuilder&)’:
source/blender/nodes/shader/nodes/node_shader_mix.cc:418:66: error: binding reference of type ‘bNode&’ to ‘const bNode’ discards qualifiers
  418 |     const fn::MultiFunction *fn = get_multi_function(builder.node());
      |                                                      ~~~~~~~~~~~~^~

Accepting as I don't think an extra review iteration is needed.

source/blender/makesdna/intern/makesdna.c
510

Prefer to avoid exact matches with public functions, this kind of thing can make IDE's confuse function definitions str_startswith or BLI_str_startswith_inline would be OK, I don't have a strong opinion besides not matching names of public functions.

This revision is now accepted and ready to land.Aug 31 2022, 11:30 AM
Closed by commit rB25e307d725d0: Nodes: move NodeTreeRef functionality into node runtime data (authored by Jacques Lucke (JacquesLucke)). · Explain WhyAug 31 2022, 12:18 PM
This revision was automatically updated to reflect the committed changes.
Jacques Lucke (JacquesLucke) added a commit: rB25e307d725d0: Nodes: move NodeTreeRef functionality into node runtime data.

Revision Contents
Changeset List

PathSize
source/
blender/
blenkernel/
439 lines
1 line
intern/
2 lines
403 lines
534 lines
blenlib/
5 lines
compositor/
realtime_compositor/
3 lines
4 lines
intern/
10 lines
21 lines
10 lines
18 lines
67 lines
30 lines
38 lines
15 lines
editors/
space_node/
83 lines
io/
wavefront_obj/
exporter/
103 lines
makesdna/
180 lines
intern/
26 lines
modifiers/
intern/
84 lines
194 lines
nodes/
2 lines
172 lines
2 lines
18 lines
composite/
nodes/
18 lines
7 lines
function/
2 lines
nodes/
2 lines
2 lines
2 lines
2 lines
2 lines
2 lines
2 lines
2 lines
2 lines
2 lines
2 lines
geometry/
2 lines
intern/
139 lines
8 lines
34 lines
8 lines
shader/
2 lines
nodes/
2 lines
6 lines
2 lines
2 lines
2 lines
2 lines
2 lines
2 lines
2 lines
2 lines
2 lines
2 lines

Diff 54909

View Options

source/blender/blenkernel/BKE_node_runtime.hh

Loading...
View Options

source/blender/blenkernel/CMakeLists.txt

Loading...
View Options

source/blender/blenkernel/intern/node.cc

Loading...
View Options

source/blender/blenkernel/intern/node_runtime.cc

Loading...
View Options

source/blender/blenkernel/intern/node_tree_update.cc

Loading...
View Options

source/blender/blenlib/BLI_multi_value_map.hh

Loading...
View Options

source/blender/compositor/realtime_compositor/COM_evaluator.hh

Loading...
View Options

source/blender/compositor/realtime_compositor/COM_utilities.hh

Loading...
View Options

source/blender/compositor/realtime_compositor/intern/compile_state.cc

Loading...
View Options

source/blender/compositor/realtime_compositor/intern/evaluator.cc

Loading...
View Options

source/blender/compositor/realtime_compositor/intern/input_single_value_operation.cc

Loading...
View Options

source/blender/compositor/realtime_compositor/intern/node_operation.cc

Loading...
View Options

source/blender/compositor/realtime_compositor/intern/scheduler.cc

Loading...
View Options

source/blender/compositor/realtime_compositor/intern/shader_node.cc

Loading...
View Options

source/blender/compositor/realtime_compositor/intern/shader_operation.cc

Loading...
View Options

source/blender/compositor/realtime_compositor/intern/utilities.cc

Loading...
View Options

source/blender/editors/space_node/node_relationships.cc

Loading...
View Options

source/blender/io/wavefront_obj/exporter/obj_export_mtl.cc

Loading...
View Options

source/blender/makesdna/DNA_node_types.h

Loading...
View Options

source/blender/makesdna/intern/makesdna.c

Loading...
View Options

source/blender/modifiers/intern/MOD_nodes.cc

Loading...
View Options

source/blender/modifiers/intern/MOD_nodes_evaluator.cc

Loading...
View Options

source/blender/nodes/CMakeLists.txt

Loading...
View Options

source/blender/nodes/NOD_derived_node_tree.hh

Loading...
View Options

source/blender/nodes/NOD_geometry_exec.hh

Loading...
View Options

source/blender/nodes/NOD_multi_function.hh

Loading...
View Options

source/blender/nodes/NOD_node_tree_ref.hh

Loading...
View Options

source/blender/nodes/composite/nodes/node_composite_image.cc

Loading...
View Options

source/blender/nodes/composite/nodes/node_composite_normal.cc

Loading...
View Options

source/blender/nodes/function/node_function_util.hh

Loading...
View Options

source/blender/nodes/function/nodes/node_fn_align_euler_to_vector.cc

Loading...
View Options

source/blender/nodes/function/nodes/node_fn_boolean_math.cc

Loading...
View Options

source/blender/nodes/function/nodes/node_fn_combine_color.cc

Loading...
View Options

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

Loading...
View Options

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

Loading...
View Options

source/blender/nodes/function/nodes/node_fn_input_bool.cc

Loading...
View Options

source/blender/nodes/function/nodes/node_fn_input_color.cc

Loading...
View Options

source/blender/nodes/function/nodes/node_fn_input_int.cc

Loading...
View Options

source/blender/nodes/function/nodes/node_fn_input_string.cc

Loading...
View Options

source/blender/nodes/function/nodes/node_fn_input_vector.cc

Loading...
View Options

source/blender/nodes/function/nodes/node_fn_rotate_euler.cc

Loading...
View Options

source/blender/nodes/geometry/node_geometry_util.hh

Loading...
View Options

source/blender/nodes/intern/derived_node_tree.cc

Loading...
View Options

source/blender/nodes/intern/geometry_nodes_eval_log.cc

Loading...
View Options

source/blender/nodes/intern/node_geometry_exec.cc

Loading...
View Options

source/blender/nodes/intern/node_multi_function.cc

Loading...
View Options

source/blender/nodes/intern/node_tree_ref.cc

Loading...
View Options

source/blender/nodes/shader/node_shader_util.hh

Loading...
View Options

source/blender/nodes/shader/nodes/node_shader_color_ramp.cc

Loading...
View Options

source/blender/nodes/shader/nodes/node_shader_curves.cc

Loading...
View Options

source/blender/nodes/shader/nodes/node_shader_math.cc

Loading...
View Options

source/blender/nodes/shader/nodes/node_shader_mix_rgb.cc

Loading...
View Options

source/blender/nodes/shader/nodes/node_shader_tex_brick.cc

Loading...
View Options

source/blender/nodes/shader/nodes/node_shader_tex_gradient.cc

Loading...
View Options

source/blender/nodes/shader/nodes/node_shader_tex_magic.cc

Loading...
View Options

source/blender/nodes/shader/nodes/node_shader_tex_musgrave.cc

Loading...
View Options

source/blender/nodes/shader/nodes/node_shader_tex_wave.cc

Loading...
View Options

source/blender/nodes/shader/nodes/node_shader_tex_white_noise.cc

Loading...
View Options

source/blender/nodes/shader/nodes/node_shader_vector_math.cc

Loading...
View Options

source/blender/nodes/shader/nodes/node_shader_vector_rotate.cc

Loading...