Fix global transform validity for Node2D and Control

[Sauermann]

Set global transform to invalid when changing transform

resolve #79453

Add unit-tests for the bug

[kleonc]

This is not a correct fix.

The global_invalid flag being initialized to false is correct. A newly created CanvasItem has no parent and its own transform is an identity, hence global_transform being an identity by default is valid.

Marking global transform as invalid by default would result in some unnecessary one-time updates for actually unchanged global transforms. But it doesn't solve the issue which is: some out of tree modifications (like Node2D.set_global_position) result in not marking the global transform as dirty.

E.g. with this PR:

@tool
extends EditorScript

func _run() -> void:
	var a = Node2D.new()
	a.global_position = Vector2(1, 1)
	assert(a.global_position == Vector2(1, 1)) # Passes because of invalid by default.
	a.global_position = Vector2(2, 2)
	assert(a.global_position == Vector2(2, 2)) # Still fails.

Also I'm not sure if the global_invalid to global_valid name change is needed/wanted at all.

[Sauermann]

@kleonc Thank you for pointing me in the right direction. I have included your improvements in the PR.

[kleonc]

@Sauermann There's also Control.global_position having the same issue.

[Sauermann]

I have included the invalidation for transform-changing operations in Control.

[Sauermann]

@kleonc may I ask if your requested changes have been resolved by my updates?

[kleonc]

@kleonc may I ask if your requested changes have been resolved by my updates?

Hmm, seems like it should now be properly invalidating the global transforms. 👍

However, I'm not sure if these added invalidations don't affect transform notifications in some unwanted manner. Specifically, after brief look at the code, when in the tree CanvasItem::_notify_transform calls _notify_transform(this):

godot/scene/main/canvas_item.h

Lines 154 to 162 in 16a9356

	_FORCE_INLINE_ void _notify_transform() {
	if (!is_inside_tree()) {
	return;
	}
	_notify_transform(this);
	if (!block_transform_notify && notify_local_transform) {
	notification(NOTIFICATION_LOCAL_TRANSFORM_CHANGED);
	}
	}

which early returns in case global transform is invalid:

godot/scene/main/canvas_item.cpp

Lines 877 to 886 in 16a9356

	void CanvasItem::_notify_transform(CanvasItem *p_node) {
	/* This check exists to avoid re-propagating the transform
	* notification down the tree on dirty nodes. It provides
	* optimization by avoiding redundancy (nodes are dirty, will get the
	* notification anyway).
	*/
	if (/*p_node->xform_change.in_list() &&*/ p_node->_is_global_invalid()) {
	return; //nothing to do
	}

Hence I wonder if adding _invalidate_global_transform(); before _notify_transform(); (like it's done in many of the changed methods) could have broken some notification propagation or something like that. 🤔 We'll need to ensure nothing is broken I guess (or maybe you've already ensured that?).

Outdated.

[Sauermann]

Thank you for taking the time to guide me through the maze of Transform-caching.
The case, that you have identified was indeed problematic.

With the latest updated I made sure that

• the behavior while is_inside_tree() == true is not changed
• while not inside tree, caching status traverses down the scene-tree

[Calinou]

Tested locally (rebased on top of master 6758a7f), it works as expected.

I've also tested a few demo projects (3D Platformer, Control Gallery) and they behave correctly.

[kleonc]

AFAICT it should work fine now. 👍

[akien-mga]

Thanks!

[YuriSizov]

Cherry-picked for 4.1.2.