Add custom_maximum_size property to Control

[StarryWorm]

• Closes Add a custom_maximum_size to Control godot-proposals#13534
• Supersedes/Alternative to Add a SizeContainer control node godot-proposals#8333, Add autowrap_maximum_width to Label and RichTextLabel godot-proposals#13568 , Implement a new property named ScrollContainer.max_size #94171, Add SizeContainer #112364

Adds a custom_maximum_size property to Control. This property will limit the size of the Control.
This property is Size2, and only applies to dimensions for which it is greater than 0.

• e.g., `custom_maximum_size = (100,0) will limit the width to 100 but will not limit the height.

This property is directly exposed in the Inspector and can be modified freely. The custom_maximum_size property overrides custom_minimum_size if it is smaller.

• e.g., custom_minimum_size = (100, 100) with custom_maximum_size = (200, 50) will result in size = (100, 50) (expandable to (200,50) if needed by its contents)

Furthermore, a new propagate_maximum_size boolean property is added to Control, which forces all children to respect the custom_maximum_size defined on that Control.

• Default value: false.
• This is overridden to true for Container.
  • This is further overridden back to false for ScrollContainer.

Certain Container subtypes may limit the size of their children even further than the maximum size in order to bring the behavior in line with existing minimum size behavior.

• For example, a MarginContainer with a child with size = (50,50) and margins of 5px on every side will have size = (60,60) currently. In this PR, if a MarginContainer has custom_maximum_size = (60, 60) with 5px margins, the children will have maximum_size = (50,50) (when propagate_maximum_size = true)

List of affected `Container` subtypes and details

FoldableContainer: now takes into account the panel theme property.
MarginContainer: now takes into account margin_* properties.
PanelContainer: now takes into account the panel theme property.
ScrollContainer: now takes into account the panel theme property and works with SCROLL_MODE_RESERVE.
TabContainer: now takes into account the panel, the tabbar_background, and the TabBar itself.

The second commit enables ScrollContainer to make use of this new custom_maximum_size by adding a new SCROLL_MODE_MAXIMIZE_FIRST. This new mode makes the ScrollContainer increase its size to fit the contents up to its custom_maximum_size first, and if the contents are larger than custom_maximum_size, it behaves like SCROLL_MODE_AUTO. If no custom_maximum_size is defined for that axis, it will behave like SCROLL_MODE_DISABLED.

The third commit enables Label and RichTextLabel to work with the new custom_maximum_size. This new behavior will override the old behavior of custom_minimum_size if both are defined. Old behavior (when only custom_minimum_size is defined) is preserved.

• For Label, this is relevant when autowrap_mode != AUTOWRAP_OFF or when text_overrun_behavior != OVERRUN_NO_TRIMMING
• For RichTextLabel, this is relevant when fit_content = true and autowrap_mode != AUTOWRAP_OFF

Warning

Merge notes: This PR and #112741 are interlinked. The code section in question is the addition of custom_maximum_size to size computation in Control::_size_changed (this PR), which becomes Control::_compute_position_and_size_with_grow (that PR)

AI disclaimer: GPT Codex 5.3 was used to help me figure out some of the ScrollContainer, Label, and RichTextLabel implementations. All AI-generated content was thoroughly reviewed by me and tested piece-wise. Most of the implementation is human-made.

[KoBeWi]

This looks like a more general way to implement godotengine/godot-proposals#13568 and similar. It will require follow-ups that adjusts behavior of other nodes (similar to what is already done in ScrollContainer), but it's a nice base I guess.

Seems like you need to do some cleanup (#116640 (comment)) before the implementation can be reviewed properly.

[markdibarry]

I tested again after the changes to Label and RichTextLabel, and this does work now in certain specific scenarios. Label seems to ignore the parent's max size, and RichTextLabel still doesn't work at all. For Label, it does appear to work if you set both the parent's max size and each individual descendant Label's max size to the exact same value, but that's not very feasible as a workflow and leaves a lot of undefined and buggy behavior.

[StarryWorm]

That should solve it. It was a stupid bug because I used get_custom_maximum_size() instead of get_combined_maximum_size(). Only the latter uses the parent Control's custom_maximum_size if propagate_maximum_size is true.

[markdibarry]

I don't believe there should be any case where propagate_maximum_size should ever be false, so I don't think there's a need for that new boolean. Either way, I tried with both on and off, and it's still the same behavior.

So that you can test it too, going forward, here's the steps to recreate the scenario mentioned in all the proposals for this feature (I'd make an MRP but since the internal code is still a WIP, it's more reliable this way):

1. Create a new Control scene and add a PanelContainer.
2. Set the PanelContainer's custom_maximum_size width to 150px and set its horizontal container sizing to Shrink Begin
3. Add a VBoxContainer as a child of the PanelContainer
4. Add one or more Labels with auto wrap or text overrun set to the VBoxContainer.

As you type notice that it ignores the PanelContainer's max size.

[StarryWorm]

I don't believe there should be any case where propagate_maximum_size should ever be false, so I don't think there's a need for that new boolean. Either way, I tried with both on and off, and it's still the same behavior.

As stated in the PR description, some Control subtypes need it to be false. This includes ScrollContainer, for example, and is basically every Control that has clip_contents = true by default.
I could very well modify the PR to set propagate_maximum_size = true as the default behavior and override it to false for the affected containers.

Regarding your example, it actually works as intended. If you look at your Label, you will see it still shows the warning for the max width being needed. That is because the VBoxContainer does not have propagate_maximum_size set to true. If you turn that on, it will work as you are expecting it to.

So this isn't a bug, it's a UX problem.
With this, I think it's pretty clear that I should set it to true by default, with false overrides for the specific Container subtypes that need it.

• Edit: This may not be possible in the event that upgrading an existing project containing said subtypes doesn't set it to false at upgrade-time. Will test before changing default behavior.

The propagate flag will still only be for immediate children, but with true by default, it should make the new feature "work as expected" by default. It absolutely cannot be for more than the immediate children (i.e., grandchildren, etc) due to the same reason why it exists. Those subtypes will pretty much break otherwise.

If you want to see it for yourself, just create a ScrollContainer with a custom_maximum_size and propagate_maximum_size = true, and put anything as a child with a custom_minimum_size greater than the scroll's max size (or a box container filled with more children than fit in the ScrollContainer's immediately visible rect). When you run the scene and scroll, it will scroll to nothing instead of the expected contents.

[markdibarry]

I do see now that it does work but only for immediate children, which you're right, is just not a good workflow/UX yet. It absolutely should have all descendants respect the value, and that should be the default. We wouldn't want it to break compatibility (or this PR would probably have to wait until we have a consensus on that), so whatever the defaults would be would need to show no change to existing projects. Considering max size just doesn't exist atm, no value should be the same behavior as without this PR.

I think, because this is a wide-reaching change that will require a lot of testing, it may be good to include a test project so we can see how it behaves with different workflows, especially the ones outlined in the proposals (so they can be closed). The two main ones are the one I had above about multiple Labels in a PanelContainer (closes #13534, #13568), and the second would be the ScrollContainer one (#94171) where content should make it grow to a max size, then create a scrollbar from then on.

I haven't tested the ScrollContainer one, but I imagine it wouldn't work without changes to ScrollContainer as well, but to avoid this getting too out of hand, maybe we could handle that in a follow up PR. I may be wrong though.

[StarryWorm]

Changed the default value of propagate_maximum_size to true for UX.

This is overridden to false for GraphEdit, ItemList, RichTextLabel, ScrollContainer, and Tree. All of these Control subtypes have clip_contents set to true by default, as their children may be bigger than themselves yet still navigated (primarily through scrollbars) within the bounds of the parent.

• This will work with upgrading projects.
• These changes are all in the main commit instead of separate commits to prevent any sort of regression issues if any of the separate commits were reverted for any reason. One commit is responsible for introducing the new custom_maximum_size and ensuring no existing projects break due to it.

@markdibarry please try your scenario again and let me know if it fails. Works as "would be expected" on my machine.

The two main ones are the one I had above about multiple Labels in a PanelContainer (closes #13534, #13568)

Yep, which now should work fully as expected.

the second one would be the ScrollContainer one (#94171) where content should make it grow to a max size, then create a scrollbar from then on.
I haven't tested the ScrollContainer one, but I imagine it wouldn't work without changes to ScrollContainer as well, but to avoid this getting too out of hand, maybe we could handle that in a follow up PR. I may be wrong though.

This is already handled with the new SCROLL_MODE_MAXIMIZE_FIRST 😄

[StarryWorm]

Regarding tests, I will add unit tests to the engine for the scenarios to help guard against regressions in the future, and will provide a test project so people can see for themselves how it works.

[markdibarry]

(chef's kiss) Wunderbar. Tested on those two scenarios and both are working as expected!

Wasn't trying to nitpick, just wanted to make sure it covered both of those two proposals completely. I just know first hand if it doesn't "just work" for 90% of cases out of the box, there's gonna tons of confusion from users and an avalanche of issues created around it. This seems like, at least while we trial run:

1. A user will need to mess with the max size initially for there to be any chance of them messing something up, and can just set it to its defaults to fix it.
2. When they do use it with specific purpose, the defaults work as expected for the majority of cases.
3. For the ones that need special custom behavior, they can modify it.

Now that we can see the main cases covered, we can dig in further for regressions and refine the workflow.

Great work!

[StarryWorm]

Oh, don't worry, I am absolutely delighted you pushed the PR to its limits. Nitpicking is why the PR process exists.

Ideally, given how this is set up, there is 0 chance of regression. All the tests also run fine on my machine, which should cover any potential regressions due to this change. As said, though, I will add as many tests as I can think of, both to prevent regressions caused by this PR and future regressions against this PR.

[StarryWorm]

Added unit tests for everything - in the appropriate commit for all of them.

[KoBeWi]

Looks fine overall. The modified controls (ScrollContainer, (RichText)Label) seem to behave as expected with maximum size, if you fulfill the requirements. I also tried some custom behavior using maximum size and it works too. The implementation seems to mostly mirror minimum size.

Definitely needs more reviews and discussion, as it's a rather major change, but it looks promising from what I tested.

For Label, this is relevant when autowrap_mode != AUTOWRAP_OFF or when text_overrun_behavior != OVERRUN_NO_TRIMMING
For RichTextLabel, this is relevant when fit_content = true and autowrap_mode != AUTOWRAP_OFF

This needs to be documented somewhere. Maybe description of each Control could have a paragraph about how it handles maximum size (only for controls that do handle it somehow).

[StarryWorm]

Regarding documentation, I have added it to the properties for which it is relevant.

I don't think adding a paragraph at the top of the documentation for the classes (ScrollContainer, Label, RichTextLabel) is the right decision.
Currently, this isn't the case for any property on those classes (or most other classes), and I think that's a good standard to follow. Even the minimum size handling for Label was not documented at the class level (or at all, to be fair); it was just a warning.

Also, since we are getting closer to a PR ready for merging, I want to make sure the commits are okay as is. I have it split into three because they all do something "separate": adding a Control feature, a ScrollContainer feature, and a Label/RichTextLabel feature. I know the standard is to keep things in one commit unless there is a very good reason not to. I believe these split commits meet that standard.
Maintainers, what's your opinion? Should I squash them?

[AdriaandeJongh]

Really look forward to this feature and took a bit of time to review today but will review more later:

• propagate_maximum_size is a bit of a tricky boolean and I'm not sure we should introduce it with this PR. It propagates the max size to individual nodes which is a bit weird in the context of BoxContainers:

… and it also propagates to grandchildren and beyond, even if those children are not in a container:

I would argue that the custom maximum size should primarily be set on the thing that has the potential to grow, in the examples above: the Label and not the PanelContainer. And with that approach we'd never need any propagation and thus removing one extra property and another layer of complexity – as well as remove something that may confuse new users (eg. "why aren't my labels growing?! ohh it's because 6 parents up something propagated all the way down here").

• I'd reorder first four properties inside the Layout section of Control to: custom min size, custom max size, (propagate max size here, if there's consensus on keeping it), and then clip contents. (and then layout direction, mode, etc)

• Other than the above, I played around with Label and RichTextLabel and loved it! Precisely what I've been needing for the last two years.

Will play with this a bit more later.

[StarryWorm]

It propagates the max size to individual nodes which is a bit weird in the context of BoxContainers

How is it weird? The contents of a BoxContainer are always smaller or as large as it (i.e., one child is visible). Currently, this is handled by the BoxContainer expanding to fit its contents. If we don't have it propagate its max size to its children, it will not layout as expected. Or am I missing something?
This applies to all Container types with the exception of ScrollContainer.

and it also propagates to grandchildren and beyond, even if those children are not in a container. [...] something that may confuse new users (eg. "why aren't my labels growing?! ohh it's because 6 parents up something propagated all the way down here")

I don't really see a real-world use case for this. I could be missing some, but as far as I understand it, Container types are meant to "contain" all their children and grandchildren, i.e., they should all fit within the Container rect.
If some users are creating some kind of (in my opinion) counter-intuitive layout where that isn't the case, I would frankly put the onus on them. Also, they aren't "new" users at that point.

Now, what could be done is to make Control not propagate by default, and instead have Container override that. I would actually prefer that. After all, Control types aren't meant to "contain", that's what we have Container types for.
For a much better explanation of what I am trying to say, see godotengine/godot-proposals#14222

Not having it propagate for Containers is, in my opinion, the wrong decision.
The moment you add MarginContainers and BoxContainers to a UI sub-element (example: a side panel in a 2D UI), you would need to go through each of their children, and calculate what max size each of them needs (including the margins from their parents and their siblings in BoxContainers) so that your UI sub-element ends up being the right max size and not infringe on the rest of the UI/the gameplay area.
Or you can just give the UI sub-element the constraint it needs to follow, and the rest will arrange itself to match it.

Edit: I strongly believe the user should still be able to toggle propagation on or off for more advanced layouts. This also goes back to Yuri's proposal of merging Control and Container, which is all about giving the user the choice of how to handle things.

Side note: This reminds me that I need to specialize MarginContainer and add in logic for any other Control that can end up with margins to substract that from their child's combined maximum size if propagate is on.

[markdibarry]

@AdriaandeJongh To piggy-back off what StarryWorm was saying, the behavior you're suggesting sounds more confusing to users tbh. I agree that ideally we could avoid the bool if possible, but the intuitive behavior IMO is children of a container with custom_maximum_size should respect the size of their container. I think of it from a UX perspective:

• The most likely scenario is you don't need custom_maximum_size, in that case don't use it! Simple.
• The second most likely (the proposal scenarios) is that you need it so that a Control or Container and children need to not exceed a maximum size. In this case (in the current form), it does it by just setting the custom_maximum_size of the Control or ancestor Container. Simple!
• The least likely is that you need a maximum size for one single Control, and it does have children, and those children need to exceed the maximum size of its container. In that case, it should be able to be overridden on a per-node basis.

What you suggest means more consistency at the cost of the common scenario and uncommon scenario both being complex to use. I guess we'd have to determine which we're more comfortable with. The HBoxContainer example you used seems like an uncommon case in UI (at least vs a VBoxContainer), but you probably wouldn't use a max size to achieve that anyway (unless I'm misunderstanding the example). I don't think there's really a way to know what the user intends to happen when you have a bunch of labels in an HBoxContainer and all of them set to auto-wrap without setting the labels min/max sizes themselves.

Now, what could be done is to make Control not propagate by default, and instead have Container override that. I would actually prefer that.

@StarryWorm Yeah, that makes sense to me.

[StarryWorm]

Changelog:

• propagate_maximum_size is now false by default, overridden to true by Container.
  • This is still overridden to false for ScrollContainer.
  • Control subtypes that had a false override no longer have it (unnecessary).
• Updated documentation to reflect that update_maximum_size calls update_minimum_size internally.
  • Modified two calls in Control that used to be update_minimum_size and update_maximum_size to only update_maximum_size to avoid double calling of update_minimum_size. This does not change behavior.
• Certain Container types now use a new get_inner_combined_maximum_size() to reflect that their content's maximum size should be limited more than just by the Container's combined maximum size.
  • This brings the behavior in line with the reasonable expectation, namely that it works like minimum size works, whereby the parent Container expands to include all its children and additional theme elements.
  • This behavior was not brought to any Control types, as they currently don't have the minimum size behavior that this reflects.

List of affected `Container` subtypes and details

FoldableContainer: now takes into account the panel theme property.
MarginContainer: now takes into account margin_*.
PanelContainer: now takes into account the panel theme property.
ScrollContainer: now takes into account the panel theme property and works with SCROLL_MODE_RESERVE.
TabContainer: now takes into account the panel, the tabbar_background, and the TabBar itself.

[AdriaandeJongh]

Wanted to test this again but the Scene and FileSystem panels are broken so I can't do anything to test this 😅

EDIT: I downloaded the artifact for macOS editor. I don't know whether it's this PR or another PR in master that broke these panels.

[StarryWorm]

That's odd... The Windows editor artifact (which has the same compilation flags that could impact this) doesn't have that issue. The implementation should be completely platform independent too... :/

Clearly a ScrollContainer issue, though, which I think is this PR's fault; one of the builds I made during the development of the feature had that issue, but I resolved it... or so I thought, I guess?

[jelolul]

That's odd... The Windows editor artifact (which has the same compilation flags that could impact this) doesn't have that issue. The implementation should be completely platform independent too... :/

I've actually tested this and I have the same issue on the Windows editor artifact

[StarryWorm]

Wait what? Now I'm very confused...
Could you please tell me how you got to that state?

I did:

• Download windows editor from Checks tab
• Unzip, run it
• Opened existing project -> works
• Created new project -> works
  • Tried with all 3 renderers, compatbility, mobile, and forward+

[AdriaandeJongh]

Wait what? Now I'm very confused... Could you please tell me how you got to that state?

I did:

• Download windows editor from Checks tab
• Unzip, run it
• Opened existing project -> works
• Created new project -> works

This is precisely what I did.

[jelolul]

• Download windows editor from Checks tab
• Unzip, run it
• Opened existing project -> works

These exact same steps, I tested and it happens even when I create a new project.

I'm confident that it broke after the last changes that you made 2 days ago.

[StarryWorm]

I mean, I have a rough idea of what could have caused the issue.
The problem I am having right now is figuring out why it works on my machine and not on yours (plural), even though it's the same artifact. The only way I have to ensure it all works now is to run it through CI and have you two check the artifacts...

[StarryWorm]

Problem identified: the Modern theme is what breaks it. My editor runs on the Classic theme (not a fan of the Modern), and it works with that one.
Should be an easy fix

[StarryWorm]

Fixed, also included the language changes from @markdibarry and @AThousandShips

[AdriaandeJongh]

Suggested change

	Enables whether [Control] based children should have their maximum size limited by this node.
	If [code]true[/code], [member custom_maximum_size] propagates to all child [Control] nodes until a child node sets their own [member custom_maximum_size] or until that child node disables [member propagate_maximum_size].

[AdriaandeJongh]

Suggested change

	[b]Note:[/b] It is recommended to define an autowrapping width via [member Control.custom_maximum_size] when using something other than [constant TextServer.AUTOWRAP_OFF].
	[b]Note:[/b] Labels with autowrapping enabled must have a custom maximum width configured to work correctly, either through the Label's own [member Control.custom_maximum_size] or as a result of a propagated maximum size from a parent Control with [Control.propagate_maximum_size] enabled.

[AdriaandeJongh]

Suggested change

	[b]Note:[/b] It is recommended to define an autowrapping width via [member Control.custom_maximum_size] when using something other than [constant TextServer.AUTOWRAP_OFF] with [member fit_content] set to [code]true[/code].
	[b]Note:[/b] RichTextLabels with autowrapping and [member fit_content] enabled must have a custom maximum width configured to work correctly, either through the RichTextLabel's own [member Control.custom_maximum_size] or as a result of a propagated maximum size from a parent Control with [Control.propagate_maximum_size] enabled.

[AdriaandeJongh]

Suggested change

	[b]Note:[/b] It is recommended to define an autowrapping width via [member Control.custom_maximum_size] when set to [code]true[/code] with [member autowrap_mode] set to something other than [constant TextServer.AUTOWRAP_OFF].
	[b]Note:[/b] RichTextLabels with autowrapping and [member fit_content] enabled must have a custom maximum width configured to work correctly, either through the RichTextLabel's own [member Control.custom_maximum_size] or as a result of a propagated maximum size from a parent Control with [Control.propagate_maximum_size] enabled.

[AdriaandeJongh]

I haven't looked at the code, but the functionality and usability look good overall. The additional properties make the Layout section of Controls a bit of a mess, but another PR can attempt to reorganize and restructure all that.

There's some iffy behavior about the container sizing (size_flags_horizontal and size_flags_vertical) where expand (obviously) can't expand beyond the custom maximum size, and then the position of the Control inside the container is essentially 'undefined' – and I bet different depending on the Container attempting to position it. I guess that's somewhat expected behavior, but it makes the whole layout system a bit less 'clean'. 🤷🏼