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

Script Create Dialog is enormous #80218

Closed
KoBeWi opened this issue Aug 3, 2023 · 7 comments · Fixed by #78744
Closed

Script Create Dialog is enormous #80218

KoBeWi opened this issue Aug 3, 2023 · 7 comments · Fixed by #78744

Comments

@KoBeWi
Copy link
Member

KoBeWi commented Aug 3, 2023

Godot version

4.2 0606ba7

System information

Windows 10.0.19045 - Vulkan (Forward+) - dedicated NVIDIA GeForce GTX 1060 (NVIDIA; 30.0.15.1403) - Intel(R) Core(TM) i7-7700HQ CPU @ 2.80GHz (8 Threads)

Issue description

When you create a new script, the dialog has a height of a few screens. Likely caused by autowrapped Labels. The bug happens even when a new Label appears in already shrunk dialog:

6ffenSASQc.mp4

Steps to reproduce

  1. Create script

Minimal reproduction project

N/A

@akien-mga
Copy link
Member

Seems to be a regression from #77280 according to @bruvzg, CC @Rindbee.

@bruvzg
Copy link
Member

bruvzg commented Aug 3, 2023

Seems to be a regression from #77280 according to @bruvzg, CC @Rindbee.

The #77280 seems to be fine by itself. The issue is most likely in the container control fitting code and the way how it's handling min. size (assuming it's constant, while it's never true, and min. height always depends on width or vice versa).

@bruvzg
Copy link
Member

bruvzg commented Aug 3, 2023

So the root of the issue: container is calculating its minimum size and sorting controls BEFORE setting control size, and using get_combined_minimum_size of the child controls for this purpose. At this point, child controls have zero size, and the calculated minimum size of the child controls will have huge height (since text is broken into a single character per line to fit control zero width). But I'm not sure what would be the correct order of operation here.

Size2i size = c->get_combined_minimum_size();

@YuriSizov
Copy link
Contributor

Container needs to calculate its min size to set controls' sizes and positions, that's a given. Our current approach is to do it enough times so it corrects itself out. Somehow that recent PR broke it completely for this case.

I don't think we can devise a good solution quickly, so for the sake of usability I'd revert the PR for now.

@Rindbee
Copy link
Contributor

Rindbee commented Aug 3, 2023

Seems to be the same case I'm worried about in #74052 (comment).

@Rindbee
Copy link
Contributor

Rindbee commented Aug 5, 2023

This issue is due to the window with wrap_controls enabled, which is similar to a container, but it does not correct itself, because its size can only be automatically expanded, not automatically shrunk.

godot/scene/gui/control.cpp

Lines 1610 to 1614 in 16a9356

Window *parent_window = invalidate->get_parent_window();
if (parent_window && parent_window->is_wrapping_controls()) {
parent_window->child_controls_changed();
break; // Stop on a window as well.
}

The code removed in #77280 seems to prevent scaling up too quickly. But it causes #77268.

MRP.zip

@YuriSizov
Copy link
Contributor

Indeed, this happens because windows can't shrink even if their content's minimum size has been reduced. I don't think it would be reasonable to expect windows to shrink either. They are resizable by the window manager and users, it would be unexpected if they could resize themselves at will. So that part of the logic is kind of stuck.


We once again reach the limits of our sizing logic.

With most controls their minimum size is fixed and doesn't change sporadically. So we can ask for their minimum size and then use that information to fit it into a container or a window. But for controls with smart layouts, such as flow containers or autowrapped labels, this is not the case. Their required size would change depending on where we want to fit them. So we need to somehow separate what is their minimum size reported to their parents, and what is their required size to fit into their owning container or window.

In other words, when we inquire about the minimum size through the entire GUI hierarchy, these controls need to report only what is fixed about their minimum size. That would be their custom minimum size property, the margins that their styleboxes contribute, and perhaps some minimum indivisible size that is required for their content.

As an example, for a flow container that can be the combined minimum size of its direct children, so we allocate the size that is required to fit any single one of its children, but not all of them at the same time. For a wrapped label it would depend on the wrapping strategy. If we wrap at words, it can be the size of a single word, otherwise it can be the size of a single character. For non-wrapping labels this is irrelevant and we can just use the entire size of the shaped text.

That's what controls would report to their parents when inquired about their minimum size. However, when it comes time to actually put them in place, we would need actually shape/resize/wrap them to fit the space allocated by the container, at which point the reported size information should be precise and include all of the content. If our smart controls are wrapping based on the allocated width, then this, in turn, will most likely affect the height of the container, which it would then propagate above. But it shouldn't affect container's width, so it shouldn't create a case of infinite resizing.

It may take a few frames or loops to come to balance, which is probably not always going to be ideal. Also, that may still be unstable if your layout has both horizontal and vertical wrapping. But I'm not sure there is a reasonable way to resolve this, as by definition it would affect itself infinitely if width affects height, and height in turn affects width. So for most reasonable configurations what I describe might just work.


However, this is a complex task that cannot be implemented without a lot of testing, and risk. For this specific issue the solution can probably be ad-hoc and simple: just add a minimum size to these message labels. There is no real reason to support shrinking them into nothingness. Besides, in most of the dialogs the rest of the layout restricts the minimum size already.

Given we have #78744 that unifies all affected windows, we can base the fix on it and it would be this:

diff --git a/editor/gui/editor_validation_panel.cpp b/editor/gui/editor_validation_panel.cpp
index ba2f2fd4a9..af15010b78 100644
--- a/editor/gui/editor_validation_panel.cpp
+++ b/editor/gui/editor_validation_panel.cpp
@@ -30,6 +30,7 @@

 #include "editor_validation_panel.h"

+#include "editor/editor_scale.h"
 #include "scene/gui/box_container.h"
 #include "scene/gui/button.h"
 #include "scene/gui/label.h"
@@ -63,6 +64,7 @@ void EditorValidationPanel::add_line(int p_id, const String &p_valid_message) {

        Label *label = memnew(Label);
        message_container->add_child(label);
+       label->set_custom_minimum_size(Size2(200 * EDSCALE, 0));
        label->set_autowrap_mode(TextServer::AUTOWRAP_WORD_SMART);

        valid_messages[p_id] = p_valid_message;

@KoBeWi If you want, I think this can be safely included in the aforementioned PR.

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

Successfully merging a pull request may close this issue.

6 participants