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

Fix Control rect coordinate system inconsistency #66688

Conversation

Sauermann
Copy link
Contributor

@Sauermann Sauermann commented Oct 1, 2022

Fix get_rect, get_global_rect and get_screen_rect to take Controls scale into account.

supersedes & extracted from #37765 authored by @ThakeeNathees (no response in a year)
Also implemented comments and additionally simplified get_screen_rectand get_screen_position.

related to #66602

Current behavior is, that the returned values are in different coordinate systems.

  • position in canvas/screen
  • size in local space

I would expect, that a function returns values in the same coordinate system.
MRP: BugGlobalRectScale.zip

Updated 2022-10-22: Fix merge conflict.

@Sauermann Sauermann requested review from a team as code owners October 1, 2022 02:58
@KoBeWi KoBeWi added this to the 4.0 milestone Oct 1, 2022
@Sauermann Sauermann force-pushed the fix-control-get-global-rect-coordinates branch 3 times, most recently from 1b8f532 to f0a48d4 Compare October 6, 2022 11:09
@kleonc
Copy link
Member

kleonc commented Oct 6, 2022

Note that with this implementation of get_rect the returned size is still not necessarily in the parent's (or containing node's) coordinate space. Control's local to parent transform (result of get_transform()) is composed of (right to left):
translation(position) * translation(pivot_offset) * rotation * scale * translation(-pivot_offset).

Meaning return Rect2(get_position(), get_scale() * get_size()) is correctly in the parent space only if: (rotation is zero) and ((pivot_offset is zero) or (scale is default)).

It can be changed to:

Transform2D t = get_transform();
return Rect2(t.get_origin, t.get_scale() * get_size())

which should be in the parent space when rotation is zero.

For non-zero rotation the position obtained from get_transform() will be correctly in the parent space but the size calculated like that won't be meaningful (since get_size() is in the Control's local space and t.get_scale() is in the parent's space and these are oriented differently). Similar is true for other similar methods like get_global_rect or get_screen_rect. So it's probably worth adding notes to these that the resulting size is not meaningful in case of non-default rotation.

Another thing which might be worth noting is that what Control::get_rect returns is unaffected by the gui/common/snap_controls_to_pixels project setting and hence it can differ a little from the actual rendering result in case this setting is turned on (same for global/screen).

Also another thing is there's Sprite.get_rect method which returns local rect, so there's naming discrepancy. I wrote plenty why the local rect is important and that's what it should return in #65112. So I think we should rename Sprite.get_rect -> Sprite.get_local_rect and probably add Control.get_local_rect returning Rect2(Vector2(0, 0), get_size()).

@Sauermann
Copy link
Contributor Author

@kleonc thanks for your detailed analysis. I will go through your notes and adjust my PR accordingly.

Regarding your suggestion Control::get_local_rect, it should be noted, that there is already Control::get_anchorable_rect, which returns exactly this.

@Sauermann Sauermann force-pushed the fix-control-get-global-rect-coordinates branch from f0a48d4 to 1aa8f7e Compare October 6, 2022 23:17
@Sauermann
Copy link
Contributor Author

Sauermann commented Oct 6, 2022

I have updated the PR with the comments.
I found of note, that if the node itself or any parent CanvasItem between the node and the canvas have a non default rotation or skew, the resulting size for get_global_rect and get_screen_rect is likely not meaningful.

@Sauermann Sauermann force-pushed the fix-control-get-global-rect-coordinates branch from 1aa8f7e to b6e1744 Compare October 6, 2022 23:23
@Sauermann Sauermann force-pushed the fix-control-get-global-rect-coordinates branch from b6e1744 to bb211cf Compare October 21, 2022 23:33
Fix get_rect, get_global_rect and get_screen_rect to take Controls scale into
account.
Simplify get_screen_position and get_screen_rect
Copy link
Member

@groud groud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved in PR review meeting.

@akien-mga akien-mga merged commit f8d80b4 into godotengine:master Jan 26, 2023
@akien-mga
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

5 participants