Improve check for whether a node tree references an animated Image.
ClosedPublic
Actions

Authored by Jacques Lucke (JacquesLucke) on May 16 2022, 11:28 AM.

Details

Reviewers

Hans Goudey (HooglyBoogly)
Jeroen Bakker (jbakker)

Maniphest Tasks

T98157: Regression: Massive drops in Animation playback (fps) in the viewport (Depsgraph)

Commits

rBf517b3a29568: Fix T98157: improve animation fps with better check in depsgraph

Summary

Previously, the depsgraph assumed that every node tree might contain a reference to a video. This resulted noticeable overhead when there was no video.
Checking whether a node tree contained a video was relatively expensive to do in the depsgraph. It is cheaper now due to the structure of the new node tree updater.

This also adds an additional run-time field to bNodeTree (there are quite a few already). We should move those to a separate run-time struct, but not as part of a bug fix.

The tests still fail unfortunately, but I think this is unrelated to this patch, since it's working fine locally and there was a recent related commit rBL62927.

Diff Detail

Repository: rB Blender

Event Timeline

Jacques Lucke (JacquesLucke) requested review of this revision.May 16 2022, 11:28 AM

Jacques Lucke (JacquesLucke) created this revision.

Harbormaster completed remote builds in B22113: Diff 51517.May 16 2022, 11:28 AM

Jacques Lucke (JacquesLucke) added a task: T98157: Regression: Massive drops in Animation playback (fps) in the viewport (Depsgraph).May 16 2022, 11:31 AM

Jacques Lucke (JacquesLucke) retitled this revision from Improve check for whether a node tree references an animated Image. to Improve check for whether a node tree references an animated Image. (WIP).

Operating system: Linux-5.17.5-76051705-generic-x86_64-with-glibc2.34 64 Bits
Graphics card: NVIDIA GeForce GTX 1080 Ti/PCIe/SSE2 NVIDIA Corporation 4.5.0 NVIDIA 470.103.01

3.0 => 240
3.1 => 140
3.2 with this patch => 220

Merge branch 'blender-v3.2-release' into has-image-animation-check
cleanup

Harbormaster completed remote builds in B22115: Diff 51526.May 16 2022, 2:44 PM

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

Hans Goudey (HooglyBoogly) added a subscriber: Hans Goudey (HooglyBoogly).May 16 2022, 2:59 PM

Hans Goudey (HooglyBoogly) added inline comments.

source/blender/blenkernel/intern/node_tree_update.cc
1264	Maybe this comment could mention the requirement that the child node trees have been updated first?
1266

Jeroen Bakker (jbakker) added inline comments.May 16 2022, 3:06 PM

source/blender/makesdna/DNA_node_types.h
506	Would it be an idea to introduce a bNodeTree_Runtime struct?

Jacques Lucke (JacquesLucke) added inline comments.May 16 2022, 4:10 PM

source/blender/blenkernel/intern/node_tree_update.cc
1264	Could be added, but generally, the entire update mechanism assumes that child node trees are updated first. It's hardly a special case in this function.
source/blender/makesdna/DNA_node_types.h
506	Yeah, we wanted to do that for a while now, but postponed it every time so far.. We'd probably want to run-time data to be a c++ struct, so it can't be defined in dna, that might it a bit more complex than we want for a simple bug fix.

Hans Goudey (HooglyBoogly) added inline comments.May 16 2022, 4:24 PM

source/blender/blenkernel/intern/node_tree_update.cc
1264	Ah, sure, fair enough.

missing null check
Merge branch 'blender-v3.2-release' into has-image-animation-check

Harbormaster completed remote builds in B22130: Diff 51557.May 17 2022, 10:25 AM

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

Jacques Lucke (JacquesLucke) retitled this revision from Improve check for whether a node tree references an animated Image. (WIP) to Improve check for whether a node tree references an animated Image..May 18 2022, 1:20 PM

Jacques Lucke (JacquesLucke) edited the summary of this revision. (Show Details)

Jacques Lucke (JacquesLucke) added reviewers: Hans Goudey (HooglyBoogly), Jeroen Bakker (jbakker).

Seems fairly straightforward. Just a suggestion for a possible further performance improvement inline.

source/blender/blenkernel/intern/node_tree_update.cc
1263	Maybe it verges on negligible, but I think this could return early for non-shader node trees. That would avoid two map lookups (`nodeTypeFind` does show up in profiles the last time I checked), and could potentially avoid the `nodes_by_type` map if that becomes lazy.

This revision is now accepted and ready to land.May 18 2022, 2:20 PM

Jeroen Bakker (jbakker) accepted this revision.May 18 2022, 4:29 PM

Jeroen Bakker (jbakker) added inline comments.

source/blender/makesdna/DNA_node_types.h
506	Ok that is definitely more work. So ok from me.

Jacques Lucke (JacquesLucke) added inline comments.May 18 2022, 4:39 PM

source/blender/blenkernel/intern/node_tree_update.cc
1263	Returning early should be fine, but yeah, that's mostly negilible. I think the `nodes_by_type` method is quite fundamental, it's used to find e.g. group inputs and outputs as well.