Node Tools: Avoid depsgraph evaluation when possible #120723

Merged
Hans Goudey merged 25 commits from HooglyBoogly/blender:node-tools-no-evaluate into main 2024-04-24 17:08:36 +02:00
Member

Currently for node tools we create and evaluate a temporary depsgraph
with all the selected object data-blocks and all data-blocks referenced
by the node tree.

Needless to say, this can be very slow when those data-blocks contain
arbitrary procedural operations. Re-evaluating all the selected objects
is particularly because it will give a slowdown even in very basic uses
of node tools.

Originally I hoped that geometry nodes could be made to work with
original as well as evaluated data-blocks. But that would require far
too many tricky changes and arguably isn't right design-wise anyway.
Instead of that, this commit makes node tools dependency graph
evaluation more fine-grained in a few ways.

  1. Remove the evaluation of selected objects. These are always visible
    in the viewport and part of the active depsgraph anyway. To protect
    against cyclic dependencies, we now compare orig_id instead of the
    object pointer itself.
  2. Evaluate the node group and its dependencies in a separate depsgraph
    used only when necessary. This allows using the original node tree
    without any copies when it doesn't reference any data-blocks.
  3. Evaluate IDs from node group inputs (from the redo panel) in the extra
    depsgraph as well, only when necessary.
Currently for node tools we create and evaluate a temporary depsgraph with all the selected object data-blocks and all data-blocks referenced by the node tree. Needless to say, this can be very slow when those data-blocks contain arbitrary procedural operations. Re-evaluating all the selected objects is particularly because it will give a slowdown even in very basic uses of node tools. Originally I hoped that geometry nodes could be made to work with original as well as evaluated data-blocks. But that would require far too many tricky changes and arguably isn't right design-wise anyway. Instead of that, this commit makes node tools dependency graph evaluation more fine-grained in a few ways. 1. Remove the evaluation of selected objects. These are always visible in the viewport and part of the active depsgraph anyway. To protect against cyclic dependencies, we now compare `orig_id` instead of the object pointer itself. 2. Evaluate the node group and its dependencies in a separate depsgraph used only when necessary. This allows using the original node tree without any copies when it doesn't reference any data-blocks. 3. Evaluate IDs from node group inputs (from the redo panel) in the extra depsgraph as well, only when necessary.
Hans Goudey added 5 commits 2024-04-16 23:15:03 +02:00
Hans Goudey added 5 commits 2024-04-17 23:11:33 +02:00
Hans Goudey requested review from Jacques Lucke 2024-04-17 23:22:52 +02:00
Hans Goudey added this to the Nodes & Physics project 2024-04-17 23:22:58 +02:00
Hans Goudey added the
Interest
Geometry Nodes
label 2024-04-17 23:23:02 +02:00
Jacques Lucke requested changes 2024-04-22 10:05:16 +02:00
Jacques Lucke left a comment
Member

Would be useful to have one or two specific examples where this is faster than before.
I could also imagine it to become slower in some cases, e.g. when the same object is referenced by the node-tree as well as the inputs because now this object is evaluated twice.

Maybe only a single temporary depsgraph should be when the node-tree references data-blocks?

I don't quite remember why we had to put selected objects into the depsgraph, can you remind me? Couldn't we just optimize this part and leave the rest as is?

Would be useful to have one or two specific examples where this is faster than before. I could also imagine it to become slower in some cases, e.g. when the same object is referenced by the node-tree as well as the inputs because now this object is evaluated twice. Maybe only a single temporary depsgraph should be when the node-tree references data-blocks? I don't quite remember why we had to put selected objects into the depsgraph, can you remind me? Couldn't we just optimize this part and leave the rest as is?
@ -1055,6 +1055,17 @@ bool BKE_collection_has_object_recursive_instanced(Collection *collection, Objec
return BLI_findptr(&objects, ob, offsetof(Base, object));
}
bool BKE_collection_has_object_recursive_instanced_orig_id(Collection *collection, Object *ob)
Member

It's not obvious to me whether the parameters are expected to be original or evaluated data-blocks. Adding asserts for that would be good too.

It's not obvious to me whether the parameters are expected to be original or evaluated data-blocks. Adding asserts for that would be good too.
HooglyBoogly marked this conversation as resolved
@ -1058,0 +1059,4 @@
{
const ListBase objects = BKE_collection_object_cache_instanced_get(collection);
LISTBASE_FOREACH (Base *, base, &objects) {
if (base->object->id.orig_id == ob->id.orig_id) {
Member

Should probably use DEG_get_original_id.

Should probably use `DEG_get_original_id`.
HooglyBoogly marked this conversation as resolved
@ -314,0 +299,4 @@
static bool deg_has_evaluated_id(const Depsgraph &depsgraph, ID *id)
{
if (!ID_TYPE_USE_COPY_ON_EVAL(GS(id->name))) {
return true;
Member

Based on the function name I'd expect this to return false.

Based on the function name I'd expect this to return `false`.
HooglyBoogly marked this conversation as resolved
@ -72,2 +72,3 @@
if (params.output_is_required("Geometry")) {
if (object == self_object) {
/* Compare by `orig_id` because objects may be copied into separate depsgraphs. */
if (object->id.orig_id == self_object->id.orig_id) {
Member

DEG_get_original_id

`DEG_get_original_id`
HooglyBoogly marked this conversation as resolved
Author
Member

This is faster in any case where object evaluation does anything. Currently there is an extra depsgraph evaluation for every selected object as part of the operator itself. That includes BMesh to Mesh conversion in edit mode, which is particularly bad because we already have to do that twice when the operator runs. So I could easily make any test file that's arbitrarily slower just by adding some modifiers. Not sure how meaningful that is TBH.

I could also imagine it to become slower in some cases, e.g. when the same object is referenced by the node-tree as well as the inputs because now this object is evaluated twice.

I don't think this will be common, especially as node tools are used more as assets in the future. IDs from redo panel inputs will generally come from the local file, whereas IDs from the node group will generally come from the asset library, so there won't be much overlap.

Maybe only a single temporary depsgraph should be when the node-tree references data-blocks?

This is arguably not necessary, there are lots of ways to structure this. Reading the code again I think it's a nice way to skip evaluation of the node group data-block if it doesn't reference any IDs itself. It also reflects that we can copy the input properties to replace original ID pointers there, but replacing evaluated pointers in the node group itself requires evaluating the node group.

That said, I agree it's a bit simpler with just one extra depsgraph. I'll switch to that for now.

I don't quite remember why we had to put selected objects into the depsgraph, can you remind me?

That's because we compared them by pointer. The part of the PR most related to that is the DEG_get_orig_id changes.

This is faster in any case where object evaluation does anything. Currently there is an extra depsgraph evaluation for every selected object as part of the operator itself. That includes BMesh to Mesh conversion in edit mode, which is particularly bad because we already have to do that twice when the operator runs. So I could easily make any test file that's arbitrarily slower just by adding some modifiers. Not sure how meaningful that is TBH. >I could also imagine it to become slower in some cases, e.g. when the same object is referenced by the node-tree as well as the inputs because now this object is evaluated twice. I don't think this will be common, especially as node tools are used more as assets in the future. IDs from redo panel inputs will generally come from the local file, whereas IDs from the node group will generally come from the asset library, so there won't be much overlap. >Maybe only a single temporary depsgraph should be when the node-tree references data-blocks? This is arguably not necessary, there are lots of ways to structure this. Reading the code again I think it's a nice way to skip evaluation of the node group data-block if it doesn't reference any IDs itself. It also reflects that we can copy the input properties to replace original ID pointers there, but replacing evaluated pointers in the node group itself requires evaluating the node group. That said, I agree it's a bit simpler with just one extra depsgraph. I'll switch to that for now. >I don't quite remember why we had to put selected objects into the depsgraph, can you remind me? That's because we compared them by pointer. The part of the PR most related to that is the `DEG_get_orig_id` changes.
Hans Goudey added 5 commits 2024-04-22 18:48:52 +02:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
97e8708978
Switch to using a single extra depsgraph
Author
Member

@blender-bot build

@blender-bot build
Hans Goudey requested review from Jacques Lucke 2024-04-22 18:51:18 +02:00
Jacques Lucke reviewed 2024-04-22 20:06:21 +02:00
@ -104,0 +105,4 @@
* Find whether an evaluated object's original ID is contained or instanced by any object in this
* collection. The collection is expected to be an evaluated data-block too.
*/
bool BKE_collection_has_object_recursive_instanced_orig_id(Collection *collection, Object *ob);
Member

Would be most useful if the _orig/_eval state would be part of the parameter name.

Would be most useful if the `_orig`/`_eval` state would be part of the parameter name.
HooglyBoogly marked this conversation as resolved
Hans Goudey added 2 commits 2024-04-22 20:25:49 +02:00
Jacques Lucke requested changes 2024-04-23 13:02:54 +02:00
@ -295,0 +297,4 @@
return DEG_get_evaluated_id(&depsgraph, id) == id;
}
static void gather_input_ids(const Depsgraph &depsgraph_active,
Member

A comment on this function would be useful. It doesn't seem obvious how it decides which IDs to gather.
Also note, just because an ID is in a depsgraph might not mean that it is fully evaluated in that depsgraph. It may have animated visibility.

A comment on this function would be useful. It doesn't seem obvious how it decides which IDs to gather. Also note, just because an ID is in a depsgraph might not mean that it is fully evaluated in that depsgraph. It may have animated visibility.
Author
Member

Thanks for bringing that up! With Sergey's help I added DEG_id_is_fully_evaluated in the latest PR update.

Thanks for bringing that up! With Sergey's help I added `DEG_id_is_fully_evaluated` in the latest PR update.
HooglyBoogly marked this conversation as resolved
@ -423,3 +439,1 @@
return OPERATOR_CANCELLED;
}
}
// for (const bNodeTreeInterfaceSocket *input : node_tree->interface_inputs()) {
Member

Should be removed or reverted.

Should be removed or reverted.
HooglyBoogly marked this conversation as resolved
@ -4330,0 +4343,4 @@
return id;
}
const ID *GeoNodesOperatorDepsgraphs::get_evaluated_id(const ID &id_orig) const
Member

The implementation details of this function could use a comment too.

The implementation details of this function could use a comment too.
HooglyBoogly marked this conversation as resolved
Hans Goudey added 3 commits 2024-04-23 16:59:06 +02:00
Hans Goudey added 1 commit 2024-04-23 17:00:47 +02:00
39d9f43b40 Revert support for data-blocks in group input
This will be added properly in the next step of development,
using session_uid properties to avoid having pointer operator
properties.
Hans Goudey requested review from Sergey Sharybin 2024-04-23 17:00:54 +02:00
Hans Goudey requested review from Jacques Lucke 2024-04-23 17:00:59 +02:00
Sergey Sharybin reviewed 2024-04-24 10:02:57 +02:00
Sergey Sharybin left a comment
Owner

I am here more like an observer. I just wanted to be involved to some extent here, just to see what are the new requirements to the DEG are, how we can improve that from its side, etc. I don't feel I can efficiently give full proper review. So, do not consider me as a blocking reviwer.

We've talked to Hans in the chat, and I could not think of a better approach from the depsgraph perspective. Not a short-term one anyway. I think it should work, but we are going to uncharted territories, so allow for some monsters be there :) Poke me if you find some case where it does not work the way we expect it to.

I am here more like an observer. I just wanted to be involved to some extent here, just to see what are the new requirements to the DEG are, how we can improve that from its side, etc. I don't feel I can efficiently give full proper review. So, do not consider me as a blocking reviwer. We've talked to Hans in the chat, and I could not think of a better approach from the depsgraph perspective. Not a short-term one anyway. I think it should work, but we are going to uncharted territories, so allow for some monsters be there :) Poke me if you find some case where it does not work the way we expect it to.
@ -343,0 +345,4 @@
bool DEG_id_is_fully_evaluated(const Depsgraph *depsgraph, const ID *id_eval)
{
const deg::Depsgraph *deg_graph = reinterpret_cast<const deg::Depsgraph *>(depsgraph);
const deg::IDNode *id_node = deg_graph->find_id_node(deg::get_original_id(id_eval));

Not sure, but maybe it might worth adding an explicit comment saying that it is only the pointer of the original object which is used.

There might be better wording, but the main message is basically: looking inside of deg::get_original_id(id_eval) is not safe, the API might be used from a render thread, and the original ID might have been deleted from the interface. But here we do not look inside, so it is fine.

More like suggestion, as to me it is quite clear what's going on, but maybe not for others.

Not sure, but maybe it might worth adding an explicit comment saying that it is only the pointer of the original object which is used. There might be better wording, but the main message is basically: looking inside of `deg::get_original_id(id_eval)` is not safe, the API might be used from a render thread, and the original ID might have been deleted from the interface. But here we do not look inside, so it is fine. More like suggestion, as to me it is quite clear what's going on, but maybe not for others.
HooglyBoogly marked this conversation as resolved
Hans Goudey added 2 commits 2024-04-24 14:25:00 +02:00
Jacques Lucke approved these changes 2024-04-24 15:49:22 +02:00
@ -172,2 +190,2 @@
Depsgraph *depsgraph = nullptr;
Scene *scene = nullptr;
const Object *self_object_orig = nullptr;
const GeoNodesOperatorDepsgraphs *depsgraphs;
Member

= nullptr, same in GeoNodesOperatorDepsgraphs

`= nullptr`, same in `GeoNodesOperatorDepsgraphs`
HooglyBoogly marked this conversation as resolved
Hans Goudey added 2 commits 2024-04-24 17:06:16 +02:00
Hans Goudey merged commit b3ecfcd77d into main 2024-04-24 17:08:36 +02:00
Hans Goudey deleted branch node-tools-no-evaluate 2024-04-24 17:08:39 +02:00
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#120723
No description provided.