New expand options of TextureRect can cause infinite resizing in certain containers due to unstable implementation

[phil-hudson]

Godot version

4.0 RC1

System information

Intel Mac OS

Issue description

When using an aspect ratio container, with a texture rect as a child. Setting the aspect ratio container to anything other than 'fit' is crashing the editor

Steps to reproduce

1. change the aspect ratio container mode from 'fit' to 'cover'
2. crashes editor

Minimal reproduction project

aspectratiobug.zip

[timothyqiu]

GDB Backtrace

Thread 1 "godot.linuxbsd." received signal SIGSEGV, Segmentation fault.
0x0000555569bb727d in Callable::get_object (this=0x7fffed958810) at core/variant/callable.cpp:132
132                     return ObjectDB::get_instance(custom->get_object());
(gdb) bt
#0  0x0000555569bb727d in Callable::get_object (this=0x7fffed958810) at core/variant/callable.cpp:132
#1  0x000055556a3bde92 in MessageQueue::statistics (this=0x6070000f47e0) at core/object/message_queue.cpp:154
#2  0x000055556a3bd5f7 in MessageQueue::push_callablep (this=0x6070000f47e0, p_callable=..., p_args=0x0, p_argcount=0, p_show_error=false) at core/object/message_queue.cpp:121
#3  0x0000555562d0a0d7 in MessageQueue::push_callable<>(Callable const&) (this=0x6070000f47e0, p_callable=...) at ./core/object/message_queue.h:101
#4  0x0000555564fb08d3 in CanvasItem::queue_redraw (this=0x61f00055b290) at scene/main/canvas_item.cpp:358
#5  0x0000555565357cd3 in Button::set_text (this=0x61f00055b290, p_text=...) at scene/gui/button.cpp:451
#6  0x0000555562ed5c58 in EditorLog::LogFilter::set_message_count (this=0x604005f7ff60, p_count=2) at editor/editor_log.h:112
#7  0x0000555562ecd591 in EditorLog::_process_message (this=0x61e000d47090, p_msg=..., p_type=EditorLog::MSG_TYPE_STD) at editor/editor_log.cpp:223
#8  0x0000555562ecd7d4 in EditorLog::add_message (this=0x61e000d47090, p_msg=..., p_type=EditorLog::MSG_TYPE_STD) at editor/editor_log.cpp:235
#9  0x0000555562f7e18d in EditorNode::_print_handler (p_this=0x61f000064a90, p_string=..., p_error=false, p_rich=false) at editor/editor_node.cpp:6503
#10 0x000055556a49d45e in __print_line (p_string=...) at core/string/print_string.cpp:81
#11 0x000055555d8497a4 in print_line (v=...) at ./core/string/print_string.h:65
#12 0x000055556a3bd585 in MessageQueue::push_callablep (this=0x6070000f47e0, p_callable=..., p_args=0x0, p_argcount=0, p_show_error=false) at core/object/message_queue.cpp:120
#13 0x0000555562d0a0d7 in MessageQueue::push_callable<>(Callable const&) (this=0x6070000f47e0, p_callable=...) at ./core/object/message_queue.h:101
#14 0x0000555564fb08d3 in CanvasItem::queue_redraw (this=0x61f000556c90) at scene/main/canvas_item.cpp:358
#15 0x0000555565812266 in RichTextLabel::_add_item (this=0x61f000556c90, p_item=0x607001f7e040, p_enter=false, p_ensure_newline=false) at scene/gui/rich_text_label.cpp:2979
#16 0x0000555565811658 in RichTextLabel::add_text (this=0x61f000556c90, p_text=...) at scene/gui/rich_text_label.cpp:2924
#17 0x0000555562ece827 in EditorLog::_add_log_line (this=0x61e000d47090, p_message=..., p_replace_previous=false) at editor/editor_log.cpp:327
#18 0x0000555562ecd476 in EditorLog::_process_message (this=0x61e000d47090, p_msg=..., p_type=EditorLog::MSG_TYPE_STD) at editor/editor_log.cpp:219
#19 0x0000555562ecd7d4 in EditorLog::add_message (this=0x61e000d47090, p_msg=..., p_type=EditorLog::MSG_TYPE_STD) at editor/editor_log.cpp:235
#20 0x0000555562f7e18d in EditorNode::_print_handler (p_this=0x61f000064a90, p_string=..., p_error=false, p_rich=false) at editor/editor_node.cpp:6503
#21 0x000055556a49d45e in __print_line (p_string=...) at core/string/print_string.cpp:81
#22 0x000055555d8497a4 in print_line (v=...) at ./core/string/print_string.h:65
#23 0x000055556a3bd585 in MessageQueue::push_callablep (this=0x6070000f47e0, p_callable=..., p_args=0x0, p_argcount=0, p_show_error=false) at core/object/message_queue.cpp:120
#24 0x0000555562d0a0d7 in MessageQueue::push_callable<>(Callable const&) (this=0x6070000f47e0, p_callable=...) at ./core/object/message_queue.h:101
#25 0x000055556545c14a in Container::queue_sort (this=0x61e0002eb090) at scene/gui/container.cpp:143
#26 0x000055556545a586 in Container::_child_minsize_changed (this=0x61e0002eb090) at scene/gui/container.cpp:40
#27 0x00005555654633ec in call_with_variant_args_helper<Container>(Container*, void (Container::*)(), Variant const**, Callable::CallError&, IndexSequence<>) (p_instance=0x61e0002eb090, p_method=NULL, p_args=0x0, r_error=...)
    at ./core/variant/binder_common.h:293
#28 0x0000555565462b29 in call_with_variant_args<Container> (p_instance=0x61e0002eb090, p_method=(void (Container::*)(Container * const)) 0x55556545a562 <Container::_child_minsize_changed()>, p_args=0x0, p_argcount=0, r_error=...)
    at ./core/variant/binder_common.h:407
#29 0x0000555565461dfa in CallableCustomMethodPointer<Container>::call (this=0x608001864bb0, p_arguments=0x0, p_argcount=0, r_return_value=..., r_call_error=...) at ./core/object/callable_method_pointer.h:104
#30 0x0000555569bb5cd0 in Callable::callp (this=0x60d003e9d968, p_arguments=0x0, p_argcount=0, r_return_value=..., r_call_error=...) at core/variant/callable.cpp:50
#31 0x000055556a3d4e2f in Object::emit_signalp (this=0x61e0002ebc90, p_name=..., p_args=0x0, p_argcount=0) at core/object/object.cpp:1046
#32 0x000055555e92a5fb in Object::emit_signal<>(StringName const&) (this=0x61e0002ebc90, p_name=...) at ./core/object/object.h:869
#33 0x000055556547868b in Control::_update_minimum_size (this=0x61e0002ebc90) at scene/gui/control.cpp:1539
#34 0x000055555ea65bdc in call_with_variant_args_helper<__UnexistingClass>(__UnexistingClass*, void (__UnexistingClass::*)(), Variant const**, Callable::CallError&, IndexSequence<>) (p_instance=0x61e0002ebc90, p_method=NULL,
    p_args=0x7fffffffd0f0, r_error=...) at ./core/variant/binder_common.h:293
#35 0x000055555ea61adb in call_with_variant_args_dv<__UnexistingClass> (p_instance=0x61e0002ebc90, p_method=(void (__UnexistingClass::*)(__UnexistingClass * const)) 0x555565478414 <Control::_update_minimum_size()>, p_args=0x0,
    p_argcount=0, r_error=..., default_values=...) at ./core/variant/binder_common.h:440
--Type <RET> for more, q to quit, c to continue without paging--
#36 0x000055555ea584c4 in MethodBindT<>::call(Object*, Variant const**, int, Callable::CallError&) const (this=0x60c0000d69d0, p_object=0x61e0002ebc90, p_args=0x0, p_arg_count=0, r_error=...) at ./core/object/method_bind.h:320
#37 0x000055556a3d000c in Object::callp (this=0x61e0002ebc90, p_method=..., p_args=0x0, p_argcount=0, r_error=...) at core/object/object.cpp:733
#38 0x0000555569bb635f in Callable::callp (this=0x7fffedd587a0, p_arguments=0x0, p_argcount=0, r_return_value=..., r_call_error=...) at core/variant/callable.cpp:62
#39 0x000055556a3c0054 in MessageQueue::_call_function (this=0x6070000f47e0, p_callable=..., p_args=0x7fffedd587b8, p_argcount=0, p_show_error=false) at core/object/message_queue.cpp:229
#40 0x000055556a3c08be in MessageQueue::flush (this=0x6070000f47e0) at core/object/message_queue.cpp:275
#41 0x000055556514b8da in SceneTree::process (this=0x619000301190, p_time=0.0069444444444444449) at scene/main/scene_tree.cpp:461
#42 0x000055555d9911eb in Main::iteration () at main/main.cpp:3131
#43 0x000055555d8432ea in OS_LinuxBSD::run (this=0x7fffffffdf30) at platform/linuxbsd/os_linuxbsd.cpp:880
#44 0x000055555d82ebd2 in main (argc=4, argv=0x7fffffffe538) at platform/linuxbsd/godot_linuxbsd.cpp:73

[robertomorrison0]

To add: this only happens if the TextureRect's expand mode is "Fit Width Proportional"

[chutchinson]

I spent some time researching this bug, and it appears to be caused in part due to container sorting logic (specifically the AspectRatioContainer, but I haven't investigated other containers yet). Essentially, after changing the mode to Cover, the children are sorted / resized. This causes the container's minimum size to change, which causes the sorting to happen again, except this time the children are sized larger than before, which affects the container's minimum size (infinite loop), and this continues until the MessageQueue buffer is full, explaining the stack trace above. This leads me to believe the issue is with a signal/notification, because the message queue doesn't appear to be fully processing messages (and freeing up queue space).

There may be a separate bug with the message queue causing an access violation, which may only manifest when the queue is full. I am investigating that too.

[akien-mga]

The MessageQueue crash is likely #63298

But the infinite loop in container logic should be fixed independently of that one.

[KoBeWi]

Printing the child_size gives interesting results 🙃

godot.windows.editor.dev.x86_64_u7ZHEDWLNr.mp4

EDIT:
I don't know how to solve it other than detecting a loop and force-stopping it.

[YuriSizov]

If the issue is caused by the TextureRect, then from the discussion on RC it seems that TextureRect is wrongly using its current size to compute its min size. This shouldn't be done.

[KoBeWi]

Then what should it use? The whole point of expand modes was to have the TextureRect fit the texture properly based on size.

[YuriSizov]

@KoBeWi It should use the current size to compute the size of the texture. But it should not use it to compute the min size of the control, otherwise it can lead to infinite growth and recalculation, as is observed. The texture size should be used for the min size.

Or perhaps you want to use the custom_minimum_size? I'm not sure I understand the idea of the expand modes now that I look at the code. Min size should never depend on the current size (with FlowContainer being a happy little accident that "just works")

[YuriSizov]

Okay, so after looking into the feature, I think that the original problem should've been solved differently. This implementation tries to trick the container into keeping a ratio by manipulating the min size, but that leads to other issues, as min size has to be deterministic and it isn't. This feature doesn't work with all containers because of that, and it also glitches when the control is used standalone, with anchors.

The solution to the problem should've been to introduce a new sizing flags for containers (or probably a modifier like Expand), so that we tell the container the desired behavior instead of trying to trick it with what is in essence a hack. That new flag would tell the container to take the min size of the child and use it as a ratio when filling the space, so if the container needs to make the height bigger, it also has to make the width bigger. Then on the TextureRect's side you need to give it a min size with a correct ratio matching your texture, which is a bit of a setup, but will work reliably.

That's the proper solution, IMO. As for the current one, I'm not sure how to salvage it, because it relies on an unstable logic. The safest option would probably be to remove the newly added expand options and only keep the first two, keep size and ignore size. The rest should be converted to one of those. This is effectively the 3.x behavior, as it was before the original PR. Then we can work on a better solution, in 4.1 I guess. At the very least, that would disable the currently misbehaving options, and if we can ultimately salvage them, we can then add them back later.

[Zireael07]

Anyway, something needs to be done fast because a crash/infinite loop isn't a good thing to have in 4.0 stable :/

[KoBeWi]

he safest option would probably be to remove the newly added expand options and only keep the first two, keep size and ignore size.

If we are going for a temporary fix for the crash, I have a better solution: #73396

No reason to remove a functionality if it works fine in every case except one.

[YuriSizov]

No reason to remove a functionality if it works fine in every case except one.

It's not just one case, and the fact that it works at all is incidental. The implementation is simply incorrect, and we will not be able to remove it after 4.0.

I have a better solution: #73396

I don't think adding hacks to fix existing hacks is a good solution. This also means anyone making a custom container would need to replicate your hack if they run into a similar issue. This is just backwards.

If this is a temporary solution, what is your proposal for a permanent solution?

[KoBeWi]

I don't have a permanent solution for now. We could even go with your sizing flag solution (but it can't require setting minimum size), in worst case we will end up with a few legacy enum values for expand_mode and some compatibility code.

Expand flags are too important for TextureRect to remove them after they finally got implemented and made the class usable in containers.

[YuriSizov]

but it can't require setting minimum size

Why is that a limitations? Setting up min sizes is a normal practice for containers. You would need to supply the ratio somehow, without relying on the current size to avoid cascading issues. And you can't rely on node's specific properties for that.

Expand flags are too important for TextureRect to remove them after they finally got implemented and made the class usable in containers.

A size flag would be easy to implement. I don't think committing to a problematic implementation just to preserve our current status quo is valid. I understand that you need this, but we should make it waterproof. I'm sorry that the PR hasn't been reviewed properly in the first place, so we could avoid it.

[KoBeWi]

Why is that a limitations?

Because it makes the TextureRect too rigid. If I use any expand mode other than "Keep Size", I don't care about texture size and just want the TextureRect to fit in whatever I put it into. Before Expand Modes I had to resort to all sorts of hacks to make it stretch properly in BoxContainers.

Implementing a ratio that depends on custom minimum size is totally pointless, because that's exactly how TextureRect was before - you had to use custom minimum size. And the container may change size or the size can sometimes be arbitrary, which makes it very cumbersome to work with.

As an example, you might want to have a HBoxContainer with text and icon. The icon is bigger than font, so you enable the "Expand" (as it was in 3.x) and set a minimum size, so the icon fits. But then you decide to change the font size, or whatever detail that affects the icon size. Suddenly it doesn't fit anymore and you have to manually set a new minimum size that will make the icon stretch properly. After doing it 10+ times, it becomes kind of annoying. It's even worse if the icon isn't square, because you need to calculate the minimum size that will make the icon keep aspect ratio.

[YuriSizov]

But what I propose doesn't require you to do any of that. You give it a small minimum size and leave the rest to the container. It will preserve the aspect ratio while scaling the texture up and down, just like it would with any other control. If you really want to remove this step of setting the min size manually, we could add a flag to TextureRect to have a min size be a small ratio based on the texture size (say, values between 0 and 1). That would still be deterministic and work with any container.

So, Keep Size, Ignore Size, Use Size as Ratio for the expand mode and min size of TextureRect, and a Fit to Size Ratio flag for the container logic.

Before Expand Modes I had to resort to all sorts of hacks to make it stretch properly in BoxContainers.

That's my problem with the implementation, you are still resorting to a hack, it's just hidden in the engine now. I'm surprised it works at all, because what happens right now is, and this is required for your hack to work, you can change the size below the min size returned by get_minimum_size(), which simply breaks any validation that the node has. And it is also why it glitches if you use one of the fit modes with an anchored TextureRect and try to resize it. Try it, it goes nuts in the editor 🙃

[KoBeWi]

Hm, that could work 🤔

Still, removing the current flags is breaking compatibility. And if we were to do it anyway, I wonder if we could as well implement this feature now 🙃

[chutchinson]

I am interested in implementing the approach from @YuriSizov, including any compatibility shims / fixups. I'm a new contributor, but I think I can manage this implementation (including docs, compatibility/migration tooling, etc.). I think I will give it a try. :)

[YuriSizov]

@chutchinson Hey, thanks for offering! It depends on the timeline right now. If we only remove the current options right now, and work on the solution later, I'd be happy to help you with this. But if we want to have the implementation replaced in the next few days (or even today), it's probably better if one of the maintainers (well, me, I guess) works on it. I'll let you know the plan.

[akien-mga]

The crash is fixed by #73396, so repurposing this issue for the underlying bug (feel free to edit the title if I didn't infer it correctly).

[YuriSizov]

@chutchinson If you want to work on a PR for the updated implementation, feel free. If you drop by the contributors chat, I can help you to figure out the necessary steps.

[YuriSizov]

In the end it might be impossible to implement proportional sizing for controls without the min size hackery, similar to how it's done in TextureRect. The main problem is that there is no mechanism to enforce any size restrictions aside from through the min size. So if we need to somehow tell the owning container that hey, if I grow horizontally I also must grow vertically, we just can't do that.

The container sizing option that I proposed here only moves the hack from the child node, like TextureRect, to the parent container. But then, if that container is also a child of another container, then we can only communicate this new limit via the min size. So we end up in the same place we are currently, with unstable logic that just kind of works.

Say we have a BoxContainer. It uses children min sizes to allocate itself a min size. If it's anchored and you resize it to be bigger, or if it's a child of another container that resizes it to be bigger, then it can use the remaining size to expand the children that want that. So in HBox we are free to expand horizontally to take as much space as there is. But if these children are configured to resize proportionally, then they will demand more vertical space as well. Something that our HBox doesn't necessarily have.

Which is not a problem in an anchored control, we can just grow in any direction. But if our HBox is a child of another container, say VBox, it needs to request more space from that VBox, and the only mechanism that Godot has for that is min size. (And then VBox might need to request space from its parent, and so on.)

---

Long story short, I'm putting a pin in this one. Feel free to come up with alternative ideas.

So far I can imagine some path with introducing an extra concept, besides the min size, that containers can use for their sizing logic. That could allow us to divorce the container logic and general min size limitations a node might have. So a control node can have some "requested size" which the container will use for allocating some space (all the way up the chain), and min size would be only used to actually limit the minimal size.

[Rindbee]

The crash is fixed by #73396, so repurposing this issue for the underlying bug (feel free to edit the title if I didn't infer it correctly).

The crash doesn't seem to be fully fixed, it still crashes if there are other container nodes (e.g. BoxContainer) between the AspectRatioContainer and the TextureRect.

These two functions are in conflict with each other.

1. The container tries to rely on the minimum size of the child control to determine the display space of the child control (stretch_mode = STRETCH_COVER, );
2. And the size of the child control depends on the size of the display space, which affects its own minimum size (expand_mode = EXPAND_FIT_*).

The condition of the infinite loop is met. Similar to #75713 at this point.