Fix cannot update remote after disabling use_global_coordinates in RemoteTransform2D

[Rindbee]

Due to the optimization in CanvasItem, global_transform is only updated when get_global_transform() is called, and then notify NOTIFICATION_TRANSFORM_CHANGED. That is, in the case where global_transform is not obtained, the notification NOTIFICATION_TRANSFORM_CHANGED will not be sent.

So we use NOTIFICATION_LOCAL_TRANSFORM_CHANGED in this case. Use in combination to prevent certain optimizations.

Fix #83297 (comment).

Same change for RemoteTransform3D, to prevent the same optimization from being used in Node3D in the future.

[Rindbee]

Provide a scene for testing.
node_2d.tscn.txt

[kleonc]

Due to the optimization in CanvasItem, global_transform is only updated when get_global_transform() is called, and then notify NOTIFICATION_TRANSFORM_CHANGED. That is, in the case where global_transform is not obtained, the notification NOTIFICATION_TRANSFORM_CHANGED will not be sent.

That's not really true as both Node2D and Control do call CanvasItem::_notify_transform from their methods when their local transform is being changed.

So AFAICT for RemoteTransform2D the NOTIFICATION_TRANSFORM_CHANGED is being sent just fine when its transform changes. Meaning regardless of the value of use_global_coordinates the updating already works.

Given all this, this PR still makes sense because when use_global_coordinates == false then RemoteTransform2D still propagates its local transform on any change of its global transform. And that's not needed, in such case the propagation is needed only when the local transform changes (if global transform changes because some parent transform has changed then it's irrelevant).

[Rindbee]

godot/scene/main/canvas_item.cpp

Lines 890 to 892 in a574c02

	if (/*p_node->xform_change.in_list() &&*/ p_node->_is_global_invalid()) {
	return; //nothing to do
	}

godot/scene/main/canvas_item.cpp

Line 181 in a574c02

_set_global_invalid(false);

This flag doesn't seem to change in Node2d/Control. The optimization still works. Maybe get_global_transform() is called somewhere else.

[kleonc]

I wrongly assumed that _set_global_invalid(true) is called only from CanvasItem::_notify_transform. 🤦‍♂️

But because it's called on NOTIFICATION_PARENTED:

godot/scene/main/canvas_item.cpp

Lines 371 to 376 in a574c02

	case NOTIFICATION_PARENTED: {
	// The node is not inside the tree during this notification.
	ERR_MAIN_THREAD_GUARD;
	_notify_transform();
	} break;

and because NOTIFICATION_PARENTED is received before a node is added to the tree, then it results in is_inside_tree() check to fail:

godot/scene/main/canvas_item.cpp

Lines 896 to 907 in a574c02

	if (p_node->notify_transform && !p_node->xform_change.in_list()) {
	if (!p_node->block_transform_notify) {
	if (p_node->is_inside_tree()) {
	if (is_accessible_from_caller_thread()) {
	get_tree()->xform_change_list.add(&p_node->xform_change);
	} else {
	// Should be rare, but still needs to be handled.
	MessageQueue::get_singleton()->push_callable(callable_mp(p_node, &CanvasItem::_notify_transform_deferred));
	}
	}
	}
	}

Hence after such CanvasItem is added to the SceneTree it's marked as having invalid global but it's not added to the SceneTree::xform_change_list which would result in propagating NOTIFICATION_TRANSFORM_CHANGED:

godot/scene/main/scene_tree.cpp

Lines 179 to 190 in a574c02

	void SceneTree::flush_transform_notifications() {
	_THREAD_SAFE_METHOD_
	SelfList<Node> *n = xform_change_list.first();
	while (n) {
	Node *node = n->self();
	SelfList<Node> *nx = n->next();
	xform_change_list.remove(n);
	n = nx;
	node->notification(NOTIFICATION_TRANSFORM_CHANGED);
	}
	}

Seems like potentially a bigger issue? 🤔

---

Regarding specifically the changes in this PR the 2D part looks good to me anyway. I haven't dived into 3D notifications.

[Rindbee]

Regarding specifically the changes in this PR the 2D part looks good to me anyway. I haven't dived into 3D notifications.

There is currently no relevant optimization in 3D. If it is not needed, I can reset that commit.

I wrongly assumed that _set_global_invalid(true) is called only from CanvasItem::_notify_transform.

_set_global_invalid(false) is only called in CanvasItem::get_global_transform(), so calling this method (or get_global_*()) is a necessary condition for notify NOTIFICATION_TRANSFORM_CHANGED next time.

Edit:
Also worth noting is the name of this flag: global_invalid, false means global_transform is valid.
That is, if get_global_*() is not called, global_transform is invalid and global_invalid is true.

[kleonc]

There is currently no relevant optimization in 3D.

Indeed, so what I've said in #83323 (comment) is true for the RemoteTransform3D (but not for the RemoteTransform2D because of the discussed optimization). Meaning only 2D was bugged.

If it is not needed, I can reset that commit.

No, the changes are good. 👍 Quoting myself (just replace RemoteTransform2D with RemoteTransform3D):

Given all this, this PR still makes sense because when use_global_coordinates == false then RemoteTransform2D still propagates its local transform on any change of its global transform. And that's not needed, in such case the propagation is needed only when the local transform changes (if global transform changes because some parent transform has changed then it's irrelevant).

[kleonc]

LGTM! 👍
I've also tested it is some more (debug-stepped through some notification code etc.), haven't noticed any issue.

Changed milestone to 4.2 as 2D part is a bugfix (3D part can be treated as an enhancement as it effectively only removes some redundant transform updates for the target node).

[akien-mga]

Thanks!