Skip to content

[3.x] Pre-calculate is_visible_in_tree() #107324

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 12, 2025
Merged

[3.x] Pre-calculate is_visible_in_tree() #107324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 43 additions & 1 deletion scene/3d/spatial.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ void Spatial::_notification(int p_what) {
} else {
data.C = nullptr;
}
_update_visible_in_tree();

if (data.toplevel && !Engine::get_singleton()->is_editor_hint()) {
if (data.parent) {
Expand Down Expand Up @@ -216,6 +217,8 @@ void Spatial::_notification(int p_what) {
data.parent = nullptr;
data.C = nullptr;
data.toplevel_active = false;

_update_visible_in_tree();
_disable_client_physics_interpolation();
} break;
case NOTIFICATION_ENTER_WORLD: {
Expand Down Expand Up @@ -732,6 +735,36 @@ Ref<World> Spatial::get_world() const {
return data.viewport->find_world();
}

void Spatial::_update_visible_in_tree() {
Spatial *parent = get_parent_spatial();

bool propagate_visible = parent ? parent->data.visible_in_tree : true;

// Only propagate visible when entering tree if we are visible.
propagate_visible &= is_visible();

_propagate_visible_in_tree(propagate_visible);
}

void Spatial::_propagate_visible_in_tree(bool p_visible_in_tree) {
// If any node is invisible, the propagation changes to invisible below.
p_visible_in_tree &= is_visible();

// No change.
if (data.visible_in_tree == p_visible_in_tree) {
return;
}

data.visible_in_tree = p_visible_in_tree;

for (int32_t n = 0; n < get_child_count(); n++) {
Spatial *s = Object::cast_to<Spatial>(get_child(n));
if (s) {
s->_propagate_visible_in_tree(p_visible_in_tree);
}
}
}
Comment on lines +749 to +766
Copy link
Member

@Ivorforce Ivorforce Jun 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrm, perhaps this should just be merged into the children iteration we're already doing for _propagate_visibility_changed?

Copy link
Member Author

@lawnjelly lawnjelly Jun 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing you meant combine with _propagate_visibility_changed(), and that's a great idea (I had it in mind already to investigate).

Combining their propagation is a bigger project to do as a follow up I think, as I think we need to revisit the Spatial children linked list (like probably remove it 😝 ).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this in RocketChat; merging these is something we want to pursue, but _propagate_visibility_changed is currently using linked list iteration. Lawnjelly look into profiling this before making changes.


void Spatial::_propagate_visibility_changed() {
notification(NOTIFICATION_VISIBILITY_CHANGED);
emit_signal(SceneStringNames::get_singleton()->visibility_changed);
Expand Down Expand Up @@ -791,6 +824,10 @@ void Spatial::show() {
return;
}

bool parent_visible = get_parent_spatial() ? get_parent_spatial()->data.visible_in_tree : true;
if (parent_visible) {
_propagate_visible_in_tree(true);
}
_propagate_visibility_changed();
}

Expand All @@ -805,10 +842,14 @@ void Spatial::hide() {
return;
}

bool parent_visible = get_parent_spatial() ? get_parent_spatial()->data.visible_in_tree : true;
if (parent_visible) {
_propagate_visible_in_tree(false);
}
_propagate_visibility_changed();
}

bool Spatial::is_visible_in_tree() const {
bool Spatial::_is_visible_in_tree_reference() const {
const Spatial *s = this;

while (s) {
Expand Down Expand Up @@ -1121,6 +1162,7 @@ Spatial::Spatial() :
data.viewport = nullptr;
data.inside_world = false;
data.visible = true;
data.visible_in_tree = true;
data.disable_scale = false;
data.vi_visible = true;
data.merging_allowed = true;
Expand Down
14 changes: 13 additions & 1 deletion scene/3d/spatial.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ class Spatial : public Node {
bool notify_transform : 1;

bool visible : 1;
bool visible_in_tree : 1;
bool disable_scale : 1;

// Scene tree interpolation
Expand Down Expand Up @@ -155,6 +156,9 @@ class Spatial : public Node {
void _notify_dirty();
void _propagate_transform_changed(Spatial *p_origin);

void _update_visible_in_tree();
bool _is_visible_in_tree_reference() const;
void _propagate_visible_in_tree(bool p_visible_in_tree);
void _propagate_visibility_changed();
void _propagate_merging_allowed(bool p_merging_allowed);

Expand Down Expand Up @@ -293,7 +297,15 @@ class Spatial : public Node {
bool is_visible() const;
void show();
void hide();
bool is_visible_in_tree() const;
bool is_visible_in_tree() const {
#if DEV_ENABLED
// As this is newly introduced, regression test the old method against the new in DEV builds.
// If no regressions, this can be removed after a beta.
bool visible = _is_visible_in_tree_reference();
ERR_FAIL_COND_V_MSG(data.visible_in_tree != visible, visible, "is_visible_in_tree regression detected, recovering.");
#endif
return data.visible_in_tree;
}

void force_update_transform();

Expand Down