Merge pull request #107493 from lawnjelly/remove_vi_visible

[3.x] Remove `vi_visible` flag from `Spatial`.

scene/3d/spatial.cpp

@@ -496,36 +496,26 @@ Transform Spatial::get_global_transform_interpolated() {
 		// with physics interpolation set to OFF, if it has a parent that is ON.
 
 		// Cheap case.
-		// We already pre-cache the visible_in_tree for VisualInstances, but NOT for Spatials, so we have to
-		// deal with non-VIs the slow way.
-		if (Object::cast_to<VisualInstance>(this) && _is_vi_visible() && data.fti_global_xform_interp_set) {
-			return data.global_transform_interpolated;
+		if (is_visible_in_tree()) {
+			return data.fti_global_xform_interp_set ? data.global_transform_interpolated : get_global_transform();
 		}
 
-		// Find out if visible in tree.
-		// If not visible in tree, find the FIRST ancestor that is visible in tree.
+		// If we got here we are invisible.
+		// But if there is a FIRST ancestor that is visible in tree, we can back calculate the interpolated transform.
 		const Spatial *visible_parent = nullptr;
 		const Spatial *s = this;
-		bool visible = true;
-		bool visible_in_tree = true;
 
 		while (s) {
-			if (!s->data.visible) {
-				visible_in_tree = false;
-				visible = false;
-			} else {
-				if (!visible) {
-					visible_parent = s;
-					visible = true;
-				}
+			if (s->data.visible_in_tree) {
+				visible_parent = s;
+				break;
 			}
 			s = s->data.parent;
 		}
 
-		// Simplest case, we can return the interpolated xform calculated by SceneTreeFTI.
-		if (visible_in_tree) {
-			return data.fti_global_xform_interp_set ? data.global_transform_interpolated : get_global_transform();
-		} else if (visible_parent) {
+		// We aren't bothering with the no visible parent case, because that would mean the root node
+		// was hidden.
+		if (visible_parent) {
 			// INVISIBLE case. Not visible, but there is a visible ancestor somewhere in the chain.
 			if (_get_scene_tree_depth() < 1) {
 				// This should not happen unless there a problem has been introduced in the scene tree depth code.
@@ -637,10 +627,6 @@ Spatial *Spatial::get_parent_spatial() const {
 	return data.parent;
 }
 
-void Spatial::_set_vi_visible(bool p_visible) {
-	data.vi_visible = p_visible;
-}
-
 Transform Spatial::get_relative_transform(const Node *p_parent) const {
 	if (p_parent == this) {
 		return Transform();
@@ -1282,7 +1268,6 @@ Spatial::Spatial() :
 	data.visible = true;
 	data.visible_in_tree = true;
 	data.disable_scale = false;
-	data.vi_visible = true;
 	data.merging_allowed = true;
 
 	data.fti_on_frame_xform_list = false;

scene/3d/spatial.h

@@ -111,11 +111,6 @@ private:
 		bool toplevel : 1;
 		bool inside_world : 1;
 
-		// this is cached, and only currently kept up to date in visual instances
-		// this is set if a visual instance is
-		// (a) in the tree AND (b) visible via is_visible_in_tree() call
-		bool vi_visible : 1;
-
 		bool ignore_notification : 1;
 		bool notify_local_transform : 1;
 		bool notify_transform : 1;
@@ -173,9 +168,6 @@ protected:
 	_FORCE_INLINE_ void set_ignore_transform_notification(bool p_ignore) { data.ignore_notification = p_ignore; }
 	_FORCE_INLINE_ void _update_local_transform() const;
 
-	void _set_vi_visible(bool p_visible);
-	bool _is_vi_visible() const { return data.vi_visible; }
-
 	Transform _get_global_transform_interpolated(real_t p_interpolation_fraction);
 	const Transform &_get_cached_global_transform_interpolated() const { return data.global_transform_interpolated; }
 	void _disable_client_physics_interpolation();

scene/3d/visual_instance.cpp

@@ -42,27 +42,26 @@ void VisualInstance::_refresh_portal_mode() {
 	VisualServer::get_singleton()->instance_set_portal_mode(instance, (VisualServer::InstancePortalMode)get_portal_mode());
 }
 
-void VisualInstance::_update_visibility() {
+void VisualInstance::_update_server_visibility_and_xform(bool p_force_refresh_server) {
 	if (!is_inside_tree()) {
 		return;
 	}
 
 	bool visible = is_visible_in_tree();
 
-	// keep a quick flag available in each node.
-	// no need to call is_visible_in_tree all over the place,
-	// providing it is propagated with a notification.
-	bool already_visible = _is_vi_visible();
-	_set_vi_visible(visible);
-
-	// if making visible, make sure the visual server is up to date with the transform
-	if (visible && (!already_visible)) {
+	// As xforms are not always updated for invisible nodes, there are two circumstances
+	// where we want to ensure the server has an up to date xform:
+	// 1) When making a node visible.
+	// 2) When the node enters the scene.
+	if (visible || p_force_refresh_server) {
 		if (!_is_using_identity_transform()) {
 			Transform gt = get_global_transform();
 			VisualServer::get_singleton()->instance_set_transform(instance, gt);
 		}
 	}
 
+	// Aside from entering the scene, there will always have been a visibility change,
+	// so update this in all cases.
 	_change_notify("visible");
 	VS::get_singleton()->instance_set_visible(get_instance(), visible);
 }
@@ -99,14 +98,14 @@ void VisualInstance::_notification(int p_what) {
 			*/
 			ERR_FAIL_COND(get_world().is_null());
 			VisualServer::get_singleton()->instance_set_scenario(instance, get_world()->get_scenario());
-			_update_visibility();
+			_update_server_visibility_and_xform(true);
 
 		} break;
 		case NOTIFICATION_TRANSFORM_CHANGED: {
 			// NOTIFICATION normally turned off for physics interpolated cases (via
 			// `notify_transform_when_fti_off`), however derived classes can still turn this back on,
 			// so always wrap with is_physics_interpolation_enabled().
-			if (_is_vi_visible() && !(is_inside_tree() && get_tree()->is_physics_interpolation_enabled()) && !_is_using_identity_transform()) {
+			if (is_visible_in_tree() && !(is_inside_tree() && get_tree()->is_physics_interpolation_enabled()) && !_is_using_identity_transform()) {
 				// Physics interpolation global off, always send.
 				VisualServer::get_singleton()->instance_set_transform(instance, get_global_transform());
 			}
@@ -115,14 +114,9 @@ void VisualInstance::_notification(int p_what) {
 			VisualServer::get_singleton()->instance_set_scenario(instance, RID());
 			VisualServer::get_singleton()->instance_attach_skeleton(instance, RID());
 			//VS::get_singleton()->instance_geometry_set_baked_light_sampler(instance, RID() );
-
-			// the vi visible flag is always set to invisible when outside the tree,
-			// so it can detect re-entering the tree and becoming visible, and send
-			// the transform to the visual server
-			_set_vi_visible(false);
 		} break;
 		case NOTIFICATION_VISIBILITY_CHANGED: {
-			_update_visibility();
+			_update_server_visibility_and_xform(false);
 		} break;
 	}
 }

scene/3d/visual_instance.h

@@ -49,7 +49,7 @@ class VisualInstance : public CullInstance {
 	RID _get_visual_instance_rid() const;
 
 protected:
-	void _update_visibility();
+	void _update_server_visibility_and_xform(bool p_force_refresh_server);
 	virtual void _refresh_portal_mode();
 	void set_instance_use_identity_transform(bool p_enable);
 	virtual void fti_update_servers_xform();

scene/main/scene_tree_fti.cpp

@@ -194,7 +194,7 @@ void SceneTreeFTI::_update_request_resets() {
 	for (uint32_t n = 0; n < data.request_reset_list.size(); n++) {
 		Spatial *s = data.request_reset_list[n];
 		if (s->_is_physics_interpolation_reset_requested()) {
-			if (s->_is_vi_visible() && !s->_is_using_identity_transform()) {
+			if (s->is_visible_in_tree() && !s->_is_using_identity_transform()) {
 				s->notification(Spatial::NOTIFICATION_RESET_PHYSICS_INTERPOLATION);
 			}
 
@@ -675,17 +675,7 @@ void SceneTreeFTI::frame_update(Node *p_root, bool p_frame_start) {
 					continue;
 				}
 
-				// The first node requires a recursive visibility check
-				// up the tree, because `is_visible()` only returns the node
-				// local flag.
-				if (Object::cast_to<VisualInstance>(s)) {
-					if (!s->_is_vi_visible()) {
-#ifdef DEBUG_ENABLED
-						skipped++;
-#endif
-						continue;
-					}
-				} else if (!s->is_visible_in_tree()) {
+				if (!s->is_visible_in_tree()) {
 #ifdef DEBUG_ENABLED
 					skipped++;
 #endif