Ensure that controls update all their sizing information when required

[YuriSizov]

This PR reverts #77651 and supersedes #77825.

It still fixes #74052, the original issue from #77651. It also fixes regressions from that PR:

• Fixes 4.1 SubViewport.size is always (2, 2)? #77815.
• Fixes Container size not updating correctly when using call_deferred #77885.
• Fixes Script name tooltips are oversized #77787 (though I couldn't reproduce it even with the current master, so please double-check).
• Fixes 4.1.beta1, mouse-over slider shows offset handle highlight #78118 (tested by Sauermann)
• Fixes Atlas preview jumps after first scroll event #78091

I decided to take the approach of reverting and refixing the issue because I wasn't confident in the original solution. I had my doubts initially, but it looked sensible enough and fixed the issue at hand, so I approved and merged it. However, given all the new issues that popped up, I thought it would be better for me to investigate the problem further and try a safer approach.

My idea here is that instead of changing the order in which the data is invalidated, we should instead make sure that all the necessary routines are called when we think that the size might've changed. This happens on tree entry, but also on global changes, such as themes and translations. Previously we would only do some of the updates in these situations, but I'm not sure it was appropriate to do it like that.

I have a suspicion, that for the most part it worked thanks to a very nasty hidden side-effect of get_combined_minimum_size(), which could in turn trigger a complete invalidation of the minimum size information. Yes, from an innocent getter. This wasn't originally the case, however, this was bolted on later, in #19871 (a follow up to #19334). I don't think this was a proper solution, but once again it solved the problems at hand at the time. I removed that side-effect, and with all the other changes haven't noticed any obvious regressions, testing the editor and the test scenes from all the linked issues.

This brings things closer to the original state of this commit: 005b69c#diff-2b7ba034670859ced66e442b9ec59f7351febc3b4352e535bc85da804bcca266

---

I was faced with one regression of my own, where opening a scene in the editor in the background caused size calculations to misbehave once you switched to it. Turns out that when we request an update with _update_minimum_size() we set a guard flag to prevent doing it multiple times. However, when the node is out of the scene tree, this method hits an early return. Which means that the guard flag is never reset and the method cannot be triggered again either.

For some reason, this only happened in editor and only under those specific circumstances (instanced scenes weren't affected, for example). Changing the size of the control in the editor would also cause it to finally update, somehow. So it's still a bit of a mystery, but the logic seems flawed without this fix.

Edit: Well, I guess for this situation to happen, the call to update_minimum_size must happen while the nodes are inside of the tree, and then the call to _update_minimum_size (with the underscore, the actual routine) needs to happen while the nodes are out of the tree. Which is... interesting? I guess as scenes are opened in the editor, the nodes are briefly added to the tree and then removed?

---

All in all, a lot of the original issue comes down to the old problem of cascading updates in labels (see #73809). Basically, when we do shaping of a text label, we use its current size. But shaping itself affects the minimum size, which in turn further affects the current size. This means that shaping is an unstable and unreliable operation whenever we have wrapping enabled (and thus cannot enforce any minimum size based on the contents).

This probably affects more stuff than labels, but somehow the entire GUI system manages to work to this day, and this PR seems to address the particular issue with auto-wrapping by doing just enough updates to make things right.

[akien-mga]

CC @Rindbee

[Rindbee]

The reason for this series of issues is Label.

1. Issue a NOTIFICATION_RESIZED notification (_size_changed());

   godot/scene/gui/control.cpp

   Line 1736 in e188d61

   notification(NOTIFICATION_RESIZED);

2. Mark data.minimum_size_valid as false;

   godot/scene/gui/control.cpp

   Line 1589 in e188d61

   invalidate->data.minimum_size_valid = false;

Then calling get_combined_minimum_size() may immediately update data.minimum_size_cache. And the update of data.last_minimum_size is deferred in _update_minimum_size(). This mechanism has a long history and is widely used. #77651 broke this mechanism, both data.minimum_size_cache and data.last_minimum_size are deferred.

Under this mechanism, the potential problem is that data.minimum_size_cache has been updated multiple times and then returns to data.last_minimum_size, so the following code will not be executed.

godot/scene/gui/control.cpp

Lines 1609 to 1613 in e188d61

	if (minsize != data.last_minimum_size) {
	data.last_minimum_size = minsize;
	_size_changed();
	emit_signal(SceneStringNames::get_singleton()->minimum_size_changed);
	}

Instead, the parent container was previously using an outdated a middle minimum size (not the first/final one) of child label to calculate its own minimum size.

So for labels, when data.minimum_size_cache returns to data.last_minimum_size, if data.minimum_size_cache has been updated, a minimum_size_changed signal may need to be emitted to notify the parent container to update the minimum size.

I'm not sure if this PR can avoid the above situation. A flag (data.minimum_size_cache_update_ahead?) can be added to indicate whether data.minimum_size_cache ever changed between update_minimum_size() and _update_minimum_size().

[Rindbee]

Suggested change

	if (minsize != data.last_minimum_size) {
	if (minsize != data.last_minimum_size || data.minimum_size_cache_update_ahead ) {
	data.minimum_size_cache_update_ahead = false;

[Rindbee]

Suggested change

	data.minimum_size_cache = minsize;
	if (data.minimum_size_cache != minsize) {
	data.minimum_size_cache_update_ahead = true;
	data.minimum_size_cache = minsize;
	}

[YuriSizov]

I'm not sure if this PR can avoid the above situation.

Can you provide a test case for that situation? It doesn't matter how many times the data.minimum_size_cache is invalidated, because it will get updated the moment it is requested next. And its value is the source of truth and it is to be trusted. So parent containers always receive the actual and correct information about children min sizes.

As for the trigger for the min size changed signal, it is the responsibility of each control to call update_minimum_size whenever it is expected that the min size was updated. And when that happens we will check what the current source of truth holds (or computes on the fly) and compare it with the stored value of data.last_minimum_size. This is the only place where data.last_minimum_size can be set, so I don't see how there can be any issue with that comparison. It will be reliably hit as long as the control calls update_minimum_size when appropriate.

---

To reiterate:

• The data.minimum_size_cache property is just a cache and we can flush and recalculate it as often as we want. It's not very fast to do it on the fly all the time, so we cache it, but whenever we suspect that the min size could've changed we must call update_minimum_size(), which ensures that it will be updated on the next fetch.

• The minimum_size_changed signal is guarded by another cached value, data.last_minimum_size. This value is also updated when we call update_minimum_size() (though not immediately). By this time the data.minimum_size_cache property has been invalidated and is guaranteed to return an actual value, which we check against the last known value and then emit the signal if the last known value is different.

I don't see a flaw in this system. Especially now that there is no side-effect from calling get_combined_minimum_size().

[Rindbee]

Can you provide a test case for that situation?

I can't provide a test case for this PR. But I'm sure #74052 has this kind of problem. See #74052 (comment)

The first two columns of the output, after the first signal is emitted, the values become unequal, and finally return to the equal state, but the signal is not emitted.

This PR does not change the mechanism. So I feel like this underlying problem is not resolved.

[YuriSizov]

The underlying problem is that auto-wrapping makes label sizing unstable, and we cannot fix that with a patch like that. It also affects other controls that behave similarly, like the FlowContainer.

When a control is designed to be elastic and adapt to whatever outer size restrictions there are (and does not have any internal restrictions), its size cannot be resolved in one step like we can do with all other controls. In this case the Label cannot enforce any minimum size because it has wrapping enabled, so its minimum width is 0. This means that, until the actual size is enforced by the parent container, the Label will shape its text as if the actual width was 0, increasing the height to fit all the text according to its wrapping rules.

Then later, when the actual size is established by the container logic, the Label will shape the text closer to what is expected and will reset its minimum size requirements to a reasonable height. And after that the container needs to do the sorting of children again.

So it always takes several steps. The current GUI system is designed to propagate minimum size changes upwards and reach a state of stability in one such pass. But with wrapping controls there is this back and forth where parent containers affect the minimum size of their children, which changes where the wrap point is, which affects the actual size, and thus a second pass is needed. And as a result we get various bugs — where the second pass doesn't happen or where we get cascading changes snowballing and never reaching a stable state.

I wasn't trying to fix the underlying problem, because it's not a simple fix. Fixing it would require us to redesign some things about our GUI system. At this point it's unsafe to try and do that for 4.1. What I was trying to do was to make sure we react to resizing in a more straightforward and predictable manner, without partial or hidden logic, and it happened to fix the issue without the same regressions your solution had.

[Rindbee]

The underlying problem is that auto-wrapping makes label sizing unstable

This is a more underlying reason.

godot/scene/gui/control.cpp

Line 1607 in a5a2367

Size2 minsize = get_combined_minimum_size();

The above code, viewed from another perspective, is equivalent to the following code:

	get_combined_minimum_size(); // Treat it as an update request.
	Size2 minsize = data.minimum_size_cache;

So the work of _update_minimum_size() can be regarded as synchronizing data.minimum_size_cache to data.last_minimum_size.

And the following code is somewhat equivalent to executing part of the _update_minimum_size() code immediately (just not emitting the minimum_size_changed signal):

update_minimum_size();
_size_changed();

If it happens to be equal when comparing, the minimum_size_changed signal will not be emitted regardless of whether data.minimum_size_cache has changed (more than twice, value0 -> value1 -> ... -> value0) since the last comparison.

godot/scene/gui/control.cpp

Line 1609 in e188d61

if (minsize != data.last_minimum_size) {

[YuriSizov]

And the following code is somewhat equivalent to executing part of the _update_minimum_size() code immediately (just not emitting the minimum_size_changed signal)

The difference here is that calling _size_changed() explicitly alongside update_minimum_size() means that we will double-check if the actual size has changed, and then, the next time the MessageQueue is flushed, we will check if the min size has changed, and if it did, check the actual size again. In the case of Label this also means that we would likely request a reshaping immediately, and then once again if the min size change, which seems to bring the balance to the whole thing.

But anyway, I'm sorry, I'm not sure if we're discussing semantics at this point or if you're pointing at some flaw? Can you elaborate if there is something that you believe I'm missing? So far everything seems to work without changing the overall logic.

[KoBeWi]

I didn't spot regressions from these changes and the PR fixes multiple issues, so it's fine.

[akien-mga]

Thanks!

I suggest moving the remaining discussion on @Rindbee's points to a follow-up issue or PR. They didn't seem blocking for merging this regression fix.

[sosasees]

¿will Godot with this pull request merged
update Control sizes when changing the window size project settings?

in Godot 4.0 i have to repress the anchor preset on the canvas's topmost Control after changing the window size project setting to preview how the new window size would really look like.

(this is the only reason i added FullRectControl to my responsive UI plugin version 1.1.
if the question hears yes, i can remove it in the add-on's possible version 2.0)

update

i tried this in the Godot 4.1 beta 2 web editor — the question hears no.

test project: test-2023-06-15.zip
(2.3 KiByte download, 3.0 KiByte unzipped)

[YuriSizov]

¿will Godot with this pull request merged update Control sizes when changing the window size project settings?

in Godot 4.0 i have to repress the anchor preset on the canvas's topmost Control after changing the window size project setting to preview how the new window size would really look like.

No, this is unrelated to what you describe. Do you have a bug report about this problem?

[sosasees]

Do you have a bug report about this problem?

no, i didn't open an issue about this.

i think this should be a feature proposal — i didn't make that either. i am not sure when i will open that issue on godot-proposals, as i must rest.

---

update: i now made the feature proposal, 5 days after this comment.