Node3D: Allow using global transform when not inside tree

[akien-mga]

Complements #67710.
Fixes #30445.

[akien-mga]

Did some testing, I'm not sure what I documented is true.

Would need to flag the global transform as dirty when exiting the tree to force recomputing the out-of-tree "global" transform.

[akien-mga]

Would need to flag the global transform as dirty when exiting the tree to force recomputing the out-of-tree "global" transform.

Did that, and it seems to work as I'd expect it, in a very simple test. I don't do much 3D so I'd appreciate more extensive testing from others.

Here's how I tested so far:

extends Node3D

var room: Node3D
var cube: MeshInstance3D

func _ready():
	room = Node3D.new()
	add_child(room)
	room.transform.origin = Vector3(1, 10, 100)
	cube = MeshInstance3D.new()
	cube.mesh = BoxMesh.new()
	room.add_child(cube)
	cube.transform.origin = Vector3(4, 40, 400)

var count = 0

func _process(delta):
	count += 1
	if count % 60 == 0:
		print("Cube Local:  " + var_to_str(cube.transform))
		print("Cube Global: " + var_to_str(cube.global_transform))
	# 60 prints base state
	if count == 90:
		print("### Adding (2, 20, 200) to in-tree global position.")
		cube.global_transform.origin += Vector3(2, 20, 200)
	# 120 prints change
	if count == 150:
		print("### Removing from tree.")
		room.remove_child(cube)
	# 180 prints change
	if count == 210:
		print("### Adding (2, 20, 200) to out-of-tree global position.")
		cube.global_transform.origin += Vector3(2, 20, 200)
	# 240 prints change
	if count == 270:
		print("### Reparent to tree.")
		room.add_child(cube)
	# 300 prints change
	if count == 330:
		get_tree().quit()

Prints:

Cube Local:  Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 4, 40, 400)
Cube Global: Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 5, 50, 500)
### Adding (2, 20, 200) to in-tree global position.
Cube Local:  Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 6, 60, 600)
Cube Global: Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 7, 70, 700)
### Removing from tree.
Cube Local:  Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 6, 60, 600)
Cube Global: Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 6, 60, 600)
### Adding (2, 20, 200) to out-of-tree global position.
Cube Local:  Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 8, 80, 800)
Cube Global: Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 8, 80, 800)
### Reparent to tree.
Cube Local:  Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 8, 80, 800)
Cube Global: Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 9, 90, 900)

I also had to run the _propagate_transform_changed() when not inside tree, or "Adding (2, 20, 200) to out-of-tree global position." would update the local transform but not the global one.

[lawnjelly]

I can only give rough feedback as away from home for xmas on tablet but im in favour of something like this.

There are two obvious variations:

1. Make current functions work outside tree, remove warnings
2. Add new functions that explicitly work outside tree, keep warnings on original versions

They both have advantages and disadvantages but i would probably tend towards the approach in this pr .. it is arguable either way.

[fire]

@Zylann I think you were looking for something like this.

[capnm]

🤔 shouldn't a node without (connected) parent always have a global initial state of zero?

With current patch:

room = Node3D.new()
### node state
Room Local:        Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 0, 0, 0)
Room Global:      Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 0, 0, 0)

room.transform.origin = Vector3(10, 20, 30)
### node state
Room Local:        Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 10, 20, 30)
Room Global:      Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 10, 20, 30)
-> ? Room Global:      Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 0, 0, 0)

room.global_transform.origin += Vector3(10, 20, 30)
### node state
Room Local:        Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 20, 40, 60)
Room Global:      Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 20, 40, 60)
-> ? Room Global:      Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 0, 0, 0)

[lawnjelly]

shouldn't a node without (connected) parent always have a global initial state of zero?

No. That's kind of the point of having out of tree global transforms.
Relative to the branch, the global transform of a node without a parent is the local transform.

[capnm]

shouldn't a node without (connected) parent always have a global initial state of zero?

No. That's kind of the point of having out of tree global transforms. Relative to the branch, the global transform of a node without a parent is the local transform.

At least from the logic standpoint, the global transformation in nowhere doesn't make much sense and can probably just be ignored. For a global transformation (which does what it says in the name), you could always attach it to a temporary parent. No?

[lawnjelly]

This is perhaps why this was treated as an error before, it seems you are misunderstanding the coordinate spaces, and perhaps this will be a common problem with users. Maybe we could use a different function name, like get_branch_transform()? 🤔

Global is used as a godot-ism for what is usually referred to as world space, and so it's natural to get this confusion. The function after this PR doesn't return world space, it returns branch space. That is what world space is too, but it is relative to the scene tree root, whereas this is just local to the root of the local branch.

I wonder whether a way to address this might be to make the main function get_branch_transform() (or synonym) and make the old get_global_transform() just be a wrapper around this with an ERR_FAIL if out of tree? 🤔

[capnm]

Edit: I thought the behaviors were intentional and not a bug.

Some tests with sub-nodes

(tip with this PR)
var tr := Vector3(10,20,30)

room = Node3D.new()
couch = MeshInstance3D.new()
couch.mesh = BoxMesh.new()
room.add_child(couch)

Room Local:       Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 0, 0, 0)
Room Global:      Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 0, 0, 0)
'→Couch Local:    Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 0, 0, 0)
'→Couch Global:   Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 0, 0, 0)

## out-of-scene room transform.
Room Local:       Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 100, 100, 100)
Room Global:      Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 100, 100, 100)
'→Couch Local:    Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 0, 0, 0)
'→Couch Global:   Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 0, 0, 0)

## out-of-scene room, global transform.
Room Local:       Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 110, 120, 130)
Room Global:      Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 110, 120, 130)
'→Couch Local:    Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 0, 0, 0)
'→Couch Global:   Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 0, 0, 0)

### out-of-scene room->couch global transform.
Room Local:       Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 110, 120, 130)
Room Global:      Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 110, 120, 130)
'→Couch Local:    Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 10, 20, 30)
'→Couch Global:   Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 10, 20, 30)

### out-of-scene room->couch transform.
Room Local:       Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 110, 120, 130)
Room Global:      Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 110, 120, 130)
'→Couch Local:    Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 20, 40, 60)
'→Couch Global:   Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 20, 40, 60)

table = Node3D.new()
book = Node3D.new()
table.add_child(book)
add_child(table) # <- scene root Node3D

$"..".global_transform.origin += Vector3(1000, 1000, 1000)
$"..".transform.origin += Vector3(1000, 1000, 1000)

Table Local:  Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 0, 0, 0)
Table Global: Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 2000, 2000, 2000)
'→Book Local:     Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 0, 0, 0)
'→Book Global:    Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 2000, 2000, 2000)

### in-scene table, transform
Table Local:  Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 10, 20, 30)
Table Global: Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 2010, 2020, 2030)
'→Book Local:     Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 0, 0, 0)
'→Book Global:    Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 2010, 2020, 2030)

### in-scene table, global transform
Table Local:  Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 20, 40, 60)
Table Global: Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 2020, 2040, 2060)
'→Book Local:     Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 0, 0, 0)
'→Book Global:    Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 2020, 2040, 2060)

[lawnjelly]

I wasn't quite sure immediately what you meant, but there is indeed a bug (which occurs on my PR too which is just a variation on this one).

When calling add_child() out of tree, the data.parent in the Node3D appears to be never set up, thus in the get_global_transform(), the check for the parent fails:

Transform3D Node3D::get_branch_transform() const {
	if (data.dirty & DIRTY_GLOBAL_TRANSFORM) {
		if (data.dirty & DIRTY_LOCAL_TRANSFORM) {
			_update_local_transform();
		}

>>>>>>>>>>>>>>>>>> THIS LINE FAILS AS data.parent is not set
		if (data.parent && !data.top_level_active) {
			data.global_transform = data.parent->get_global_transform() * data.local_transform;
		} else {
			data.global_transform = data.local_transform;
		}

		if (data.disable_scale) {
			data.global_transform.basis.orthonormalize();
		}

		data.dirty &= ~DIRTY_GLOBAL_TRANSFORM;
	}

	return data.global_transform;
}

I'll see how this can be fixed, it's kind of strange behaviour I'm not sure why the parent is not set immediately on add_child()... 🤔

Yes it seems that the current paradigm is that the Node3D parent is only set when the node is in the tree - it is set on entering the tree, and unset when exiting the tree. This does not seem to work outside the main tree as ENTER_TREE and EXIT_TREE are never sent, so this data.parent would need to be set on add_child() and remove_child() rather than on these tree notifications.

This same behaviour occurs in 3.x, so it seems historical.

Interestingly the Node "parent" is set out of tree, so you could potentially cast to Node3D on the fly, but that would probably be expensive.

This c++ code triggers the bug:

	Transform3D tr;
	tr.origin = Vector3(10, 0, 0);
	Node3D * room = memnew(Node3D);
	room->set_transform(tr);
	Node3D * couch = memnew(Node3D);
	couch->set_transform(tr);
	
	room->add_child(couch);
	
>>>>>>>>>>>>>>>>> res is incorrect, it returns (10, 0, 0) instead of (20, 0, 0)
	Transform3D res = couch->get_global_transform();

We can probably solve this by moving some of the Node3D functionality from ENTER_TREE and EXIT_TREE to NOTIFICATION_PARENTED and NOTIFICATION_UNPARENTED. I'm not sure whether there will be any knock on areas which are expecting parent to be NULL outside the tree but let's see...

[lawnjelly]

Ok I've now got a tentative fix in my version of this PR (#74659), see the Node3D data.parent section of the description.

It will be up to @akien-mga how he wants to fix this in this PR.

@capnm If you want to test my PR that would be useful, as the two PRs are intended to be essentially the same (bar the renaming of the function for the out-of-tree version).

[capnm]

Bad joke, I take it back.

[lawnjelly]

Let's make a deal, you drop the idea of branch_transform and help akien fix this PR and I'll try my best to test it :)

I can't alter @akien-mga 's PR, only provide a possible fix as I have done above.
Testing the other version doesn't need to imply you endorse it BTW - just make a note that you prefer the existing function name.

Just to reiterate .. I have no particular preference for one name over the other -

Using the existing name is simpler technically, but on the downside:

• It can lead to confusion because get_global_transform() no longer returns results in single global / world space coordinate system when out-of-tree - essentially it no longer does what the function implies - it does not return "global space"
• It previously flagged an error and in many cases this would be a genuine programming error (getting global_transform() outside the tree will not return world space), and this will likely lead to a new class of user bugs

[akien-mga]

I can't alter @akien-mga 's PR, only provide a possible fix as I have done above.

Technically you can, all maintainers can check out a PR branch locally, amend it, and force push to the PR author's remote (unless the PR author specifically disallowed it when opening the PR).

I kind of lost oversight on this PR, I'm happy to go with any consensual solution :)