Pre-calculate is_visible_in_tree()
#107330
Draft
+56
−2
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Greatly increases the efficiency and performance is the
is_visible_in_tree()
system.Forward port of #107324
Benchmarks
~6x faster in DEBUG and RELEASE with depth 10 scene tree
(repeated for the same node, so in cache, so likely the change would be faster real world)
Has to be benchmarked from c++ as gdscript is too slow to benchmark multiple calls to
is_visible_in_tree()
.Background
The 3D code for
is_visible_in_tree()
was added 14 years ago in 2ee4ac1however I long suspected it was not a good approach, as the runtime check is very expensive - it involves recursing up the chain to look for any invisible nodes. This kind of operation is very bad on modern CPUs, as it has a tendency to cause cache misses (like a linked list is bad).
As with many systems in scene graphs, there is a choice between:
In many situations Godot goes for "lazy updating", which is the cheap update, and then the expense is deferred to getting. In most cases the expensive getter is cached. Not so for
is_visible_in_tree()
. Everytime it is called, every frame, tick, it is expensive.Solutions
There are two obvious solutions here:
I think on balance this second approach makes sense. Adding / removing nodes from the tree is a relatively rare operation, as is showing / hiding, and in most cases the update will be cheap anyway as the
_propagate_visible_in_tree()
call terminates early when no change is detected._is_vi_visible()
This problem has come up before, and I'd already added a partial solution, which was the optimized
_is_vi_visible()
system. The only snag is that this only works forVisualInstance
derived nodes, and not allSpatials
, therefore we end up needing rather cumbersome checks forVisualInstance
cast_to before using it. It would be much better if we could replace this with a generic system for allSpatials
.This is what this PR does.
Rollout
As this has some potential for the odd bug, and might require rebasing a couple of (my) PRs, I figure rollout could be done in three stages:
is_visible_in_tree()
machinery for 3D (and optional regression testing code) - this PR_is_vi_visible()
and change callers to useis_visible_in_tree()
Notes
Node::iterate_children
as a fast way to iterate a node's children #107369 to be merged as we need quick access to children (this is not a problem in 3.x asget_child()
etc is fast there).