Node Tools: Avoid depsgraph evaluation when possible #120723
No reviewers
Labels
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 project
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#120723
Loading…
Reference in New Issue
No description provided.
Delete Branch "HooglyBoogly/blender:node-tools-no-evaluate"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.
in the viewport and part of the active depsgraph anyway. To protect
against cyclic dependencies, we now compare
orig_id
instead of theobject pointer itself.
used only when necessary. This allows using the original node tree
without any copies when it doesn't reference any data-blocks.
depsgraph as well, only when necessary.
Hans Goudey referenced this pull request2024-04-17 21:38:47 +02:00
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)
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.
@ -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) {
Should probably use
DEG_get_original_id
.@ -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;
Based on the function name I'd expect this to return
false
.@ -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) {
DEG_get_original_id
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 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.
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.
That's because we compared them by pointer. The part of the PR most related to that is the
DEG_get_orig_id
changes.@blender-bot build
@ -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);
Would be most useful if the
_orig
/_eval
state would be part of the parameter name.@ -295,0 +297,4 @@
return DEG_get_evaluated_id(&depsgraph, id) == id;
}
static void gather_input_ids(const Depsgraph &depsgraph_active,
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.
Thanks for bringing that up! With Sergey's help I added
DEG_id_is_fully_evaluated
in the latest PR update.@ -423,3 +439,1 @@
return OPERATOR_CANCELLED;
}
}
// for (const bNodeTreeInterfaceSocket *input : node_tree->interface_inputs()) {
Should be removed or reverted.
@ -4330,0 +4343,4 @@
return id;
}
const ID *GeoNodesOperatorDepsgraphs::get_evaluated_id(const ID &id_orig) const
The implementation details of this function could use a comment too.
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.
@ -172,2 +190,2 @@
Depsgraph *depsgraph = nullptr;
Scene *scene = nullptr;
const Object *self_object_orig = nullptr;
const GeoNodesOperatorDepsgraphs *depsgraphs;
= nullptr
, same inGeoNodesOperatorDepsgraphs