Skip to content

Fix unexpected control position change when left/top offsets not match pos_cache (reverted) #106141

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

Merged
merged 1 commit into from
May 9, 2025

Conversation

L2750558108
Copy link
Contributor

@L2750558108 L2750558108 commented May 7, 2025

In #104357, in order to prevent the size of the offsets from being broken during offsets recalculate in set_position, the rect which use size calculated in real time via the offsets (previously use size_cache) is used in the offsets' recalculating

But this doesn't take into account the position of the rect. If the current pos_cache does not match (edge[0], edge[1]) calculated via offsets (e.g. the control's size is propped up by the minimum size when the grow_direction is not Right/Bottom), the position of the control may be offset to the top left of the expected position. This pr solve this

Fixes : #105771

@L2750558108 L2750558108 requested a review from a team as a code owner May 7, 2025 10:19
@KoBeWi KoBeWi added this to the 4.5 milestone May 7, 2025
@L2750558108 L2750558108 changed the title Fix unexpected control position changed when left/top offsets not match pos_cache Fix unexpected control position change when left/top offsets not match pos_cache May 7, 2025
@L2750558108 L2750558108 force-pushed the fix-control-position branch from d4dfd67 to 49c7966 Compare May 7, 2025 10:47
@KoBeWi
Copy link
Member

KoBeWi commented May 7, 2025

It fixes the regressions, but it might introduce a new one, just like the previous fixes.
Ideally we should have tests at least for each issue in the chain.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Well, I verified that it does not bring back any of the related issues. Here are reproduction scenes for each of them:
Sizebugs.zip
(I did not test each one in their bugged state, only with this PR)

While there is a risk this PR will create just another regression, the alternative is reverting all previous fixes (or picking the least bad of the issues). Merging the fix while we are still in dev stage is fine; potential new bugs are likely to get detected and fixed before stable (and assuming we don't re-introduce old bugs, there is only a limited number of things that can break, right? 🤞).

@akien-mga akien-mga changed the title Fix unexpected control position change when left/top offsets not match pos_cache Fix unexpected control position change when left/top offsets not match pos_cache May 8, 2025
@akien-mga
Copy link
Member

YOLO I guess :D

We really need to work on a test suite for Control positions as you suggested, as we've had probably a dozen such regressions over time already.

But yeah, let's give this one a spin, and if we found more regressions which require piling more hacks on top, there's always the option of reverting the whole chain of PRs to the last "reliable with known issues" state.

@Repiteo Repiteo merged commit a752868 into godotengine:master May 9, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented May 9, 2025

Thanks!

@akien-mga akien-mga changed the title Fix unexpected control position change when left/top offsets not match pos_cache Fix unexpected control position change when left/top offsets not match pos_cache (reverted) May 16, 2025
Repiteo added a commit that referenced this pull request May 16, 2025
Revert #104357 and #106141 to fix Control RTL position
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.

Control's global position set to wrong value
4 participants