Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Control.global_position represent the same point as position, not global_transform.origin #101719

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kleonc
Copy link
Member

@kleonc kleonc commented Jan 17, 2025

Note that the issues/discrepancies occur only for Controls with non-default rotation/scale and pivot_offset.

After rethinking #95798 (comment) (please read for a detailed explanation of the issue), I think the current behavior is wrong.

Currently (after #89502) Control.global_position is being equivalent to Control.global_transform.origin.

However, AFAICT it makes more sense for Control.global_position to be instead representing the same exact point as Control.position, just in a different coordinate system. And Control.position is not equivalent to Control.transform.origin, because the transform incorporates rotation/scale around custom pivot, hence the final translation/origin part of such transform is not only the position. These imply that Control.global_position would be not equivalent to Control.global_transform.origin:

If:

  • Control.global_position and Control.position represent the same point.
  • Control.global_transform.origin and Control.transform.origin represent the same point.
  • Control.position is not equivalent to Control.transform.origin.

then:

  • Control.global_position is not equivalent to Control.global_transform.origin.

So this PR implements the alternative mentioned in #95798 (comment), now both Control.global_position and Control.position represent the same point (but in different spaces (global/parent)). Also, as a consequence, Control.global_position is no longer equivalent to Control.global_transform.origin.

Regarding compatibility breaking:

  • As mentioned in Inconsistent behavior when rotating control nodes #95798 (comment), pre-4.3 the behavior between Control.set_global_position and Control.get_global_position was mixed/broken.
  • In 4.3.stable it was "fixed" in Fix Control::set_global_position for rotated/scaled transforms #89502, making Control.{set|get}_global_position consistently referring to Control.global_transform.origin (aka the same point as Control.transform.origin (but in different coordinate system), which is not necessarily the same point as Control.position).
  • This PR completely changes the behavior compared to 4.3.stable, so it definitely is / can be considered compat breaking (hence I'm marking it as such). But I consider this to be a bug fix, this PR seems to be the correct approach to me, the current behavior should be changed. I'd say the faster the better, less versions with Control.global_position and Control.position representing different points. So I suggest merging for 4.4, whether it should / should not be cherry-picked for 4.3.1 I'm not sure.

Fixes #95798.
Fixes #101692.


To-do (hence a draft):

  • Improve docs.

@kleonc kleonc added this to the 4.4 milestone Jan 17, 2025
@kleonc kleonc requested a review from a team as a code owner January 17, 2025 21:05
@kleonc kleonc marked this pull request as draft January 17, 2025 21:05
@kleonc kleonc force-pushed the control_global_position_is_same_as_position branch from c0cd3a9 to 9d8911e Compare January 17, 2025 21:07
@kleonc kleonc force-pushed the control_global_position_is_same_as_position branch from 9d8911e to d19e218 Compare January 17, 2025 23:26
@kleonc
Copy link
Member Author

kleonc commented Jan 17, 2025

Apparently data.parent_canvas_item is not set when outside the tree, made the tests fail.
But it was already being used in Control.set_global_position, meaning it wasn't working correctly before this PR when outside the tree, there was just no test case catching this.
I've added relevant test case, and fixed the behavior (without changing when data.parent_canvas_item is being set to avoid potential regressions).

@fire
Copy link
Member

fire commented Jan 18, 2025

@TokageItLab What are your thoughts on this since we have kept fixing rotation and global transform issues in 3D?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants