Fix global transform being wrong on entering tree

[groud]

• Fixes Tilemap Collision not colliding with player in 4.2 #85680
• Bugsquad edit: Fixes get_local_mouse_position does not work under certain circumstances #85705

So, this is a tricky problem to explain. Basically, in the TileMap code, I used to do something like this (it's simplified so you get the idea):

case NOTIFICATION_TRANSFORM_CHANGED: {
   ...
    Transform2D gl_transform = tile_map_node->get_global_transform();
	ps->body_set_state(body, PhysicsServer2D::BODY_STATE_TRANSFORM, gl_transform * xform);
   ...
}

After a long investigation on the TileMap code, I realized that the gl_transform provided there was wrong. Basically, when entering the tree, get_global_transform will provide wrong data on the first NOTIFICATION_TRANSFORM_CHANGED received (this notification is received on entering the tree, after ENTER_TREE).

What this PR does, is making sure that, on entering the tree, the CanvasItem global transform is marked as invalid right away. I am not 100% sure his is the most efficient / correct way to solve the issue, but it works.
I added it to NOTIFICATION_PARENTED as it is the first notification to be triggered (at the CanvasItem level) when adding a CanvasItem to the tree. Also, it calls _notify_transform right after, so not marking the global_transform as invalied seemed weird to me. I kind of guessed that when re-parented, the node's global_transform should always we set as invalid anyway.

Another solution might be to set the flag in the constructor. As I guess that the fact the NOTIFICATION_EXIT_TREE notification sets the state as invalid would probably be enough (basically, the problem seems to be the global_transform not being marked as invalid by default). I decided to add it to NOTIFICATION_PARENTED as it seemed to make more sense.

Edit: after some investigation, it seems like this notification is not always triggered, so I updated the PR to update things in NOTIFICATION_ENTER_TREE instead.

As a side note, something like this worked normally in GDScript:

			add_child(tilemaps)
			print(tilemaps.global_transform)

I suspect that there's something in the add_child chain of events that does trigger the invalidation of the global_transform later on, but not sure where.

[Sauermann]

What this PR does, is making sure that, on entering the tree, the CanvasItem global transform is marked as invalid right away

What about nested nodes

- node_a
 - node_b
   - node_c
- node_d

Now conceptually do

node_a.remove_child(node_b)
node_d.add_child(node_b)

node_c should not have received a NOTIFICATION_PARENTED.
Could such a situation happen in the TileMap code?

basically, the problem seems to be the global_transform not being marked as invalid by default

I would consider this indeed a problem.

[groud]

What about nested nodes

I think that, in that case, it should work correctly, as the NOTIFICATION_EXIT_TREE does mark the global transform as invalid too.

[groud]

Updated the PR, moving things to NOTIFICATION_ENTER_TREE

[akien-mga]

Related changes in recent times:

• Fix invalid 2D global position when read outside tree #75509
• Fix global transform validity for Node2D and Control #80105
• Remove CanvasItem::_invalidate_global_transform #80320

[groud]

Ah thanks. The problem might come from #80105 then, where _set_global_invalid(true); were removed from NOTIFICATION_ENTER_TREE.
CC @Sauermann

[Sauermann]

It appears, that in #80105 I was mistaken by my assumption, that I could move to the invalidation call from ENTER_TREE to PARENTED. Now I understand the problems of this change.

Before #80105, _set_global_invalid was called during ENTER_TREE right before _enter_canvas(); perhaps it would be best to move it again to that location.

#80105 got cherry-picked to 4.1.2, should this fix also make it into the next 4.1 release?

[groud]

Before #80105, _set_global_invalid was called during ENTER_TREE right before _enter_canvas(); perhaps it would be best to move it again to that location.

I don't know if it has any implication but I guess it should have no significant impact to move it back there (from what I read in the code). I'll update the PR to move the statement where it was before then.

Edit: done.

[kleonc]

I'm not sure if there's really a bug within the CanvasItem notifications.

If I understand it right, the issue with the mentioned TileMap's code is that it assumes that NOTIFICATION_TRANSFORM_CHANGED is received only when inside the tree?
If such notification would be received when outside the tree then the obtained global transform passed to the physics server would be not taking into account the transform of the future-parent-in-the-tree of course. And the thing is such notification can be received when outside the tree so the above can happen.

To me it seems rather like incorrect logic/handling of notifications on the TileMap side. Like it should likely be ensuring it's inside the tree when handling NOTIFICATION_TRANSFORM_CHANGED, and do some update on entering the tree. In fact it could do something like:

case NOTIFICATION_ENTER_TREE: {
   ...
   set_notify_transform(true);
   update_physics_stuff();
   ...
}
case NOTIFICATION_EXIT_TREE: {
   ...
   set_notify_transform(false);
   ...
}
case NOTIFICATION_TRANSFORM_CHANGED: {
   ...
   update_physics_stuff();
   ...
}

To clarify, it's very likely that before the changes making CanvasItem's global transform work outside the tree (PRs from #86841 (comment) + probably some earlier ones) the NOTIFICATION_TRANSFORM_CHANGED could have been received only when inside the tree. Hence before such changes the TileMap's code might have worked just fine. But currently such assumption is incorrect.

---

If there's indeed some bug with the CanvasItem notifications that I'm not currently getting then for sure we should add test case(s) for such specific situation, to avoid potential regressions in the future (#80105 added some tests).

[groud]

If I understand it right, the issue with the mentioned TileMap's code is that it assumes that NOTIFICATION_TRANSFORM_CHANGED is received only when inside the tree?

The PR description is a bit outdated, but after investigating, the problem is not present only there. The global transform is wrong even in NOTIFICATION_ENTER_TREE. This is an issue, as there are several places in the code that rely on get_global_transform in this notification (not only TileMap). TBH, I wonder if this wasn't already reported before somewhere, it probably causes other issues elsewhere.

Edit: I tried adding some print_linein CollisionObject2D, but there it seems like the global transform ends up fine on the master branch in NOTIFICATION_ENTER_TREE. I think it might be due to NOTIFICATION_TRANSFORM_CHANGED being called before, due to CollisionObject2D calling set_notify_transform(true); in its constructor.

To me it seems rather like incorrect logic/handling of notifications on the TileMap side. Like it should likely be ensuring it's inside the tree when handling NOTIFICATION_TRANSFORM_CHANGED, and do some update on entering the tree. In fact it could do something like:

Well, the TileMap code seems a bit weird I agree, but I tested that thoroughly: the global transform is 100% wrong when entering the tree. I tested it while removing all physics stuff, just keeping what was strictly necessary, and the global transform was still wrong when entering the tree.

This PR fixes it.

[kleonc]

Well, the TileMap code seems a bit weird I agree, but I tested that thoroughly: the global transform is 100% wrong when entering the tree. I tested it while removing all physics stuff, just keeping what was strictly necessary, and the global transform was still wrong when entering the tree.

Ah, indeed - even if the TileMap is needlessly updating physics on NOTIFICATION_TRANSFORM_CHANGED when outside the tree, it should still receive an additional NOTIFICATION_TRANSFORM_CHANGED after entering the tree as its global transform would change (and thus it should work properly).

TBH, I wonder if this wasn't already reported before somewhere, it probably causes other issues elsewhere.

I suspect #85705 could have the same cause? 🤔

[Sauermann]

The bug, that this PR solves is not restricted to TileMaps.
I have created a unit test for this bug in #87270 and have verified, that this PR is sufficient to make that unit test pass.

[Sauermann]

I have tested the following MRPs:

• get_local_mouse_position does not work under certain circumstances #85705
• Tilemap Collision not colliding with player in 4.2 #85680 (comment)

This PR (rebased on 107f296) fixes both of them.

[YuriSizov]

Thanks!

[YuriSizov]

Removing 4.1 cherry-pick as it wasn't confirmed for that branch (in fact, one of the closed issues implies it works correctly in 4.1.3), so to avoid the unexpected, let's keep things as is.

[YuriSizov]

Cherry-picked for 4.2.2.