Cycles: introduce an ownership system to define if nodes should be removed from the scene.
ClosedPublic
Actions

Authored by Kévin Dietrich (kevindietrich) on Aug 11 2020, 4:24 PM.

Details

Reviewers

Brecht Van Lommel (brecht)

Group Reviewers

Cycles

Maniphest Tasks

T79131: Cycles/Blender API redesign

Commits

rCc19302e499c2: Introduce an ownership system to protect nodes from unwanted deletions.
rB429afe0c626a: Cycles: introduce an ownership system to protect nodes from unwanted deletions.

Summary

Problem: the Blender synchronization process creates and tags nodes for usage. It does
this by directly adding and removing nodes from the scene data. If some node is not tagged
as used at the end of a synchronization, it then deletes the node from the scene. This poses
a problem when it comes to supporting procedural nodes who can create other nodes not known
by the Blender synchonization system, which will remove them.

Nodes now have a NodeOwner, which has to be set on creation. Node creation is done through
an API at the scene level, which takes the owner as parameter. Before rendering, the Scene
will ask the various owners if a node should be removed, this is done via Scene::update_nodes.
When a node is removed, the scene tags the appropriate node manager for an update, freeing
this responsability from BlenderSync.

Concerning BlenderSync, the id_maps do not explicitely manipulate scene data and delete nodes
anymore, they now only keep track of which node is used and ask the scene to create nodes of
the right type. To achieve this, the ParticleSystem had to be turned into a Node, although it
does not have any properties/sockets.

Since every node has to have an owner, the Scene becomes the owner of the default shaders.

There is a slight synchronization issue when deleting an object in the Blender scene during
viewport render, where objects do not disappear immediately. To fix this an extra call to
Scene::update_nodes was added in the Blender session which isn't nice (ideally the session
should just create and update node data, without explicitely telling the scene to update the
nodes arrays).

This is part of T79131.

Diff Detail

Repository

rB Blender

Branch

cycles_node_ownership (branched from master)

Build Status

Buildable 9792
Build 9792: arc lint + arc unit

Event Timeline

Kévin Dietrich (kevindietrich) requested review of this revision.Aug 11 2020, 4:24 PM

Kévin Dietrich (kevindietrich) created this revision.

Harbormaster completed remote builds in B9466: Diff 27621.Aug 11 2020, 4:24 PM

Kévin Dietrich (kevindietrich) added a task: T79131: Cycles/Blender API redesign.Aug 11 2020, 4:26 PM

Kévin Dietrich (kevindietrich) mentioned this in D8544: Cycles API: use getters and setters for accessing Node sockets.Aug 11 2020, 6:41 PM

Every node should be allocated with create_node, also internally.
Rather than passing node owner to the constructor, this can be assigned by create_node after allocation?
Scene and Procedural can have create_node functions, assigning themselves as owners.
The Blender exporter could then either use a procedural or directly create nodes on the scene, rather than having the concept of an abstract owner that could be anything.
I don't think that Cycles should be asking if nodes should be removed. Deleting nodes should be done by the Blender exporter and not delayed as part of scene update. We could have a delete_node function for that.

Kévin Dietrich (kevindietrich) added a child revision: D8544: Cycles API: use getters and setters for accessing Node sockets.Aug 11 2020, 7:39 PM

For reference: after discussion it's not clear anymore we even need an owner pointer. The Blender exporter itself can keep track of which nodes it owns, and so can other procedurals.

We could still add an owner pointer if we want automatic cleanup of nodes after a procedural node is deleted, or for debugging to make sure procedurals are implemented correctly. But this then becomes almost a small implementation detail, where create_node can set the owner pointer after the node has been created, and no code outside of the procedural class needs to be aware of it.

remove the owner from the constructors, introduce Scene::delete_node which checks for ownership
use Scene::create_node to allocate every node, the scene sets itself as the owner

Harbormaster completed remote builds in B9665: Diff 27944.Aug 19 2020, 11:44 PM

In this new patch the owner is merely some tag that is set on node creation and when deleting a node we assert that the owner is the right one (by default we assume the scene is the owner), the assertion is only in debug builds, in release builds if someone removes somehow a node that is owned (needed) by someone else we will get most likely get a crash, which is a good indicator that something is wrong.

ShaderGraph now stores a pointer to the scene, as it is required to create the nodes, and passing the scene to each of its method would be a bit obnoxious I think considering the number of call sites that would be affected.

Brecht Van Lommel (brecht) mentioned this in T79131: Cycles/Blender API redesign.Aug 21 2020, 3:45 PM

Brecht Van Lommel (brecht) added inline comments.Aug 21 2020, 4:01 PM

intern/cycles/render/scene.h
285–302	Add comments here explaining what these functions are for.
intern/cycles/render/shader.cpp
626–628	For shader nodes it should be `graph->create_node` I think, it can be the graph that owns the node and manages when they are freed? I don't think the shader graph needs a scene pointer.