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 resizing wrongly after "change type" in editor #91804

Merged
merged 1 commit into from May 15, 2024

Conversation

RedMser
Copy link
Contributor

@RedMser RedMser commented May 10, 2024

Fixes #78779

This bug has been bothering me for a while now, so I decided to tackle it once and for all. Yes it's a hacky fix, but since nobody has stepped up with a proper solution so far, I'd argue this is better than keeping this bug around for even longer.

Similar (six year old!) hacks are also found right below where I did my change, for making other node types work properly with this editor feature. So maybe this method is flawed in some way?

Note that this does NOT fix the undo issues that come with this (so undoing a "change type" may still cause the control to end up wrongly sized). There are similar issues with the editor showing outdated sizes, or undo/redo not reverting this, so it's likely a more fundamental issue at play.

Background

See this very helpful comment for some details.

The PR #78009 made it so that Control updates its size cache in a lot more places, including when entering the tree (NOTIFICATION_THEME_CHANGED).

As I understand it, the editor's "replace by" feature (used by "change type") works by first calling Node::replace_by and then re-applying same-named properties. However, since replace_by also adds the node to the scene tree, the size is recomputed through _size_changed().

I'm not exactly sure why it ends up using the parent viewport's size in this case, instead of the project settings size, but this is the best fix I could come up with.
Tried changing Control::get_parent_anchorable_rect logic a bunch, but it doesn't seem to be the culprit from my understanding. Don't want to remove/delay the problematic _size_changed() call since it seems to have fixed a bunch of bugs.

@RedMser RedMser requested a review from a team as a code owner May 10, 2024 17:00
@AThousandShips AThousandShips added this to the 4.3 milestone May 10, 2024
@AThousandShips AThousandShips added cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels May 10, 2024
editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented May 14, 2024

This is funny.

godot/scene/gui/control.cpp

Lines 663 to 667 in 1d47561

if (scene_root_parent && get_viewport() == scene_root_parent->get_viewport()) {
parent_rect.size = Size2(GLOBAL_GET("display/window/size/viewport_width"), GLOBAL_GET("display/window/size/viewport_height"));
} else {
parent_rect = get_viewport()->get_visible_rect();
}

The condition fails, because edited_scene_root is null, because it's currently being replaced by another node. A fix could be setting the properties after the root was set as edited scene, but not sure how doable is that and it's probably not safe.

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.

It's a hack, but at least it's safe.

@akien-mga akien-mga merged commit 80b9f5b into godotengine:master May 15, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release topic:editor topic:gui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Control size changes after changing its type to a subtype
4 participants