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

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

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

Merge branch 'master' into remove-node-tree-ref
cleanup

Harbormaster completed remote builds in B23421: Diff 54859.Aug 22 2022, 12:07 AM

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

Realtime compositor changes looks good.

Merge branch 'master' into remove-node-tree-ref

Harbormaster completed remote builds in B23451: Diff 54909.Aug 23 2022, 12:43 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
2	What about calling this file `node_topology_cache.cc`? I think being more specific would be an improvement-- "runtime" is a bit vague.
16–34	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.

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

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
2	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.
16–34	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.

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.

@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

Merge branch 'master' into remove-node-tree-ref

Harbormaster completed remote builds in B23539: Diff 55105.Aug 30 2022, 1:08 PM

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.

Nodes: Move NodeTreeRef functionality into node runtime data.ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 55138

source/blender/blenkernel/BKE_node_runtime.hh

source/blender/blenkernel/CMakeLists.txt

source/blender/blenkernel/intern/node.cc

source/blender/blenkernel/intern/node_runtime.cc

source/blender/blenkernel/intern/node_tree_update.cc

source/blender/blenlib/BLI_multi_value_map.hh

source/blender/compositor/realtime_compositor/COM_evaluator.hh

source/blender/compositor/realtime_compositor/COM_utilities.hh

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

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

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

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

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

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

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

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

source/blender/editors/space_node/node_relationships.cc

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

source/blender/makesdna/DNA_node_types.h

source/blender/makesdna/intern/makesdna.c

source/blender/modifiers/intern/MOD_nodes.cc

source/blender/modifiers/intern/MOD_nodes_evaluator.cc

source/blender/nodes/CMakeLists.txt

source/blender/nodes/NOD_derived_node_tree.hh

source/blender/nodes/NOD_geometry_exec.hh

source/blender/nodes/NOD_multi_function.hh

source/blender/nodes/NOD_node_tree_ref.hh

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

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

source/blender/nodes/function/node_function_util.hh

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

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

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

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

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

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

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

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

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

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

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

source/blender/nodes/geometry/node_geometry_util.hh

source/blender/nodes/intern/derived_node_tree.cc

source/blender/nodes/intern/geometry_nodes_eval_log.cc

source/blender/nodes/intern/node_geometry_exec.cc

source/blender/nodes/intern/node_multi_function.cc

source/blender/nodes/intern/node_tree_ref.cc

source/blender/nodes/shader/node_shader_util.hh

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

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

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

source/blender/nodes/shader/nodes/node_shader_mix.cc

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

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

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

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

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

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

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

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

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

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

Revision Contents
Changeset List