Skip to content

Conversation

@StarryWorm
Copy link
Contributor

Before this PR, whenever Control::set_position or Control::set_size was called, they would "freeze" the container's minimum size by fixing its offsets. If the Container was made smaller by resizing/deleting a child node (e.g., an HBoxContainer with a child deleted) after set_position or set_size had been called, it would not shrink to match the new expected size.

This PR introduces an offset recalculation in Control::_update_minimum_size before _size_changed() is called, which fixes the issue.

Before PR

node_2d.tscn.-.control.shrink.fix.-.Godot.Engine.2025-11-07.17-27-07.mp4

After PR

node_2d.tscn.-.control.shrink.fix.-.Godot.Engine.2025-11-07.17-29-01.mp4

Note: I don't know why the recordings are so messed up but they show the behavior and fix, so good enough for me

@StarryWorm StarryWorm requested a review from a team as a code owner November 7, 2025 22:31
@YeldhamDev YeldhamDev added this to the 4.6 milestone Nov 7, 2025
@kevinlam508
Copy link
Contributor

kevinlam508 commented Nov 9, 2025

This change will cause a regression when using anchors to have a control set to a relative size.

  • A control could be set up to always take up half the screen like so:
image
  • The control could coincidentally have it's min size be exactly half screen size
    • Control could be a container and have children that happen to cause the min size to become screen size
    • Game could be in windowed mode and shrunk such that the screen size becomes the min size
    • An easy way to see this for testing would be to set the custom min size to half screen size
  • If the control min size then shrinks, the control will no longer be half screen size, like it should have been by setting the anchors.

Keeping something a relative size with this change would require additional scripting to assign min size when sizes change when this is what anchors are designed for.

I tried tackling this issue and the similar #100626 before, but ultimately figured that the desired behavior is possible without a change. To always keep a control at its min size is possible as long as the anchors are handled correctly. If the user keeps the anchored points (anchors + offsets) the same on an axis, then the control will always be its min size on that axis. So move while maintaining specific anchors, a user could use set_begin and set_end instead of set_position since changing set_position to maintain anchor distances had issues.

Maybe a set_anchored_position that only moves the anchor offsets based on the existing offsets rather than min size could be added for convenience. Or adding putting the auto shrink behavior behind a bool property, so it doesn't complicate the normal use for anchors.

@StarryWorm
Copy link
Contributor Author

This change will cause a regression when using anchors to have a control set to a relative size.

If the control min size then shrinks, the control will no longer be half screen size, like it should have been by setting the anchors.

Please test before reporting regressions. This uses _compute_offsets(), which does not modify the anchors. Unless I am misunderstanding what you are saying, this PR does not cause this issue. See videos.

node_2d.tscn.-.control.shrink.fix.-.Godot.Engine.2025-11-10.12-16-08.mp4
node_2d.tscn.-.control.shrink.fix.-.Godot.Engine.2025-11-10.12-18-47.mp4

So move while maintaining specific anchors, a user could use set_begin and set_end instead of set_position since changing set_position to maintain anchor distances had issues.

Maybe a set_anchored_position that only moves the anchor offsets based on the existing offsets rather than min size could be added for convenience. Or adding putting the auto shrink behavior behind a bool property, so it doesn't complicate the normal use for anchors.

This is simply not an option. Moving the node in the editor uses set_position so it cannot be causing issues that "lock" the minimum size to what it was at the time of moving. Similarly, users should be able to expect that set_position does not have this issue when they use it in code.

@kevinlam508
Copy link
Contributor

I did test it, but I should have expected the confusion due to the exact steps I had to take to confirm the issue. I brought up the concern because of how difficult it'd be to realized what's going on if the stars aligned for someone unaware of this change.

  • I meant anchors and offsets when I said anchors are changed since together they form an "anchor rect" used for sizing
  • Both dimensions have to have min size equal to the current size to trigger the issue since the change checks equality on the whole rect rather than any axis independently
  • The first video couldn't have had the vbox container reach the required vertical min size and I couldn't see the child item custom min size to see if that could have been missed for the repro.

Before:

shrink.before.mp4

After:

shrink.after.mp4

Here's the MRP project:
shrink-test.zip

Just open the project, select "relative child" and shrink its custom min size. With the change, offsets get changed despite expecting "relative child" to stay half the size of the parent.
Be careful about switching between having the changes and not. The offset changes can get saved to the scene which change the test. The "relative child" settings should be as below:
image

The MRP is vastly simplified, but it'd be possible to accidentally set up this scenario if "relative child" was a container with dynamic children or the parent changes size.

Like I said, if a specialized function to set position isn't wanted, then I'm fine with the change with it put behind an always_min_size bool property (possibly split by axis) so that sizing by anchors + offsets doesn't have an edge case. Splitting by axis would also give more precise control since right now both axes have to match to shrink one.

@StarryWorm
Copy link
Contributor Author

I will look into seeing if that can be prevented with a code fix, otherwise I would also be in favor of introducing the by-axis setting.
Thanks for the detailed report and MRP!

Edge cases suck

@StarryWorm
Copy link
Contributor Author

StarryWorm commented Nov 10, 2025

Managed to remove that edge case by adding a check for
data.stored_layout_mode != LayoutMode::LAYOUT_MODE_ANCHORS

I still don't fully understand the conditions that could trigger this issue, so I fixed the one you pointed out, but I didn't know how else to try reaching that state. Please try the edge cases you can think of and let me know if the fix doesn't work for them.

or the parent changes size

This - if I understand it correctly - I couldn't "fix", but I ran it on master and the issue exists there already too. So I would be tempted to say it is a separate issue (namely, resizing the parent doesn't invalidate the child's cache, which should happen if the child is using anchor mode).
That could be the focus of a future bugfix PR, but is not a regression introduced by this change nor within the scope of this change.

~

Backtested the updated PR on all previous tests I ran and it still performs as expected.

@kevinlam508
Copy link
Contributor

kevinlam508 commented Nov 11, 2025

Non-anchor MRP:
shrink-test-2.zip

  1. Duplicate "Child" so that there's 4 total
  2. Delete a child
  3. Before this change, "PanelContainer" will keep it's size. After, "PanelContainer" will shrink.

Although, this one can be worked around by setting the custom min size of "PanelContainer" to a desired size.

The core of the edge cases is:

  1. Coincidentally make size the same as min size
  2. Cause a min size shrink
  3. Some control will shrink when it should keep its size from a different setting

And there's plenty of ways for step 1 to happen by chance aside from the anchor + offset side effects from set_position which may not necessarily want to be at min size.

  • The parent size change example was something like (although this is no longer possible by excluding anchor mode):
    1. Parent is size (300, 100), child has anchors like before and min size (100, 100)
    2. Reduce parent size to (200, 100), now child is in "shrink to min size" mode
    3. Change child min size
  • Should be possible to cause unintended shrinking with theme changes or theme override changes since separation or stylebox settings affect min size. This case would be difficult to script around without decently complex size correction. I don't have a test project, but one high level example could be:
    1. Have two themes with different separation values for HBoxContainer
    2. Have the HBoxContainer at the exact size for the theme that makes it larger
    3. Switch to the smaller theme

I have a hunch that there's other cases I can't think of because it's a kind of generic requirement.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GraphNode doesn't shrink automatically

3 participants