godot icon indicating copy to clipboard operation
godot copied to clipboard

Fix Control resizing wrongly after "change type" in editor

Open RedMser opened this issue 1 year ago • 1 comments

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 avatar May 10 '24 17:05 RedMser

This is funny. https://github.com/godotengine/godot/blob/1d47561319938e10cb53d202ceaeca102511a31e/scene/gui/control.cpp#L663-L667 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.

KoBeWi avatar May 14 '24 20:05 KoBeWi

Thanks!

akien-mga avatar May 15 '24 10:05 akien-mga