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

Add FlowContainer #56104

Merged
merged 1 commit into from Jan 18, 2022
Merged

Conversation

Geometror
Copy link
Member

@Geometror Geometror commented Dec 20, 2021

This PR adds a H/VFlowContainer node, which arranges and wraps control nodes like line of text. This container type can be very useful for menu bands, palettes, etc., so it has some internal use cases (e.g. the color picker presets for which I had to use a GridContainer which had some problems with resizing requiring a rather hacky solution).
Implements godotengine/godot-proposals#2893
Might provide a solution/alternative in some cases to godotengine/godot-proposals#2907

grafik

Features:

  • Horizontal and vertical flow container nodes
  • Respects all Size Flags as well as the stretch ratio

Demonstration:

Animation (1)

EDIT: New improved version:

Animation3

Note: This PR adds a separate HFlowContainer and VFlowContainer node for consistency. Whether to merge these container variants is another topic and requires additional discussion.

@Geometror Geometror requested review from a team as code owners December 20, 2021 16:24
@Calinou Calinou added this to the 4.0 milestone Dec 20, 2021
@Calinou
Copy link
Member

Calinou commented Dec 20, 2021

Note: This PR adds a separate HFlowContainer and VFlowContainer node for consistency. Whether to merge these container variants is another topic and requires additional discussion.

For the record, this is being discussed in godotengine/godot-proposals#3558.

@bruvzg
Copy link
Member

bruvzg commented Dec 20, 2021

Is it working with RTL layout? It's probably should have a check for bool rtl = is_layout_rtl(); and mirror control positions horizontally, like this:

Screenshot 2021-12-20 at 18 41 26

Screenshot 2021-12-20 at 18 48 52

@Geometror
Copy link
Member Author

RTL layout now working:

Animation (1) RTL

doc/classes/FlowContainer.xml Outdated Show resolved Hide resolved
@likeich
Copy link
Contributor

likeich commented Dec 21, 2021

This would be really useful for a project I'm working on. The project is an android launcher, I could use this to order apps like a word cloud. I've wanted to do this for a while but didn't because of how difficult it would be with the current control nodes, this would be great for that.

Just my 2 cents.

@Zireael07
Copy link
Contributor

This would massively simplify one of my extant projects (a graphical roguelike). Think Diablo 3/NWN 1/Baldur's Gate inventory, with a grid of slots...

@groud
Copy link
Member

groud commented Dec 21, 2021

A FlowContainer is definitely something we have been wanting for a long time. However, I think the current implementation won't be very usable in practice. While it works as an independent node as you display in your video, I am pretty sure that, as soon as your container will be put in another container, it will break. Mainly because it won't be able to compute a correct minimum size.

Supporting floating layouts is a more complex task than it seems, as they require computing a minimum width that depends on a given height (or the other way around, if you adjust the width the minimum height can change). This is something that is not yet supported at all by Godot's layout system. It should be done someday but it's a significant amount of work.

@Geometror
Copy link
Member Author

Geometror commented Dec 21, 2021

Removed an overlooked print_line debug loop.
@groud Although I think there might a few things to add or change in the future, they seem to work fine when placed in other containers or even when other containers are placed inside them (only tested with a VBoxContainer in the following example):
Tested with a small application which should resemble some kind of text-editor:
grafik
Animation (3)B
EDIT: Slightly improved GIF

@DarkKilauea
Copy link
Contributor

Note: This PR adds a separate HFlowContainer and VFlowContainer node for consistency. Whether to merge these container variants is another topic and requires additional discussion.

Although it shouldn't block the PR from being merged, I think we should drop the H/V variants and just have FlowContainer since the only real difference between them is a boolean that determines the layout direction. I understand the need for the prefixes with H/VBoxContainer since BoxContainer isn't very descriptive of the function, but FlowContainer does a better job here and isn't clarified by the addition of a prefix.

@YuriSizov
Copy link
Contributor

I understand the need for the prefixes with H/VBoxContainer since BoxContainer isn't very descriptive of the function

That's not really the reason for their existence. It does make the tree much clearer without the need to go into each node's properties. I think that consistency here is for the better, until we decide to drop H/V prefixes everywhere (there was a proposal to do so, maybe even a PR).

@akien-mga akien-mga requested a review from a team January 5, 2022 09:52
scene/gui/flow_container.h Outdated Show resolved Hide resolved
scene/gui/flow_container.h Outdated Show resolved Hide resolved
scene/gui/flow_container.cpp Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented Jan 5, 2022

Toggling visibility of a child in FlowContainer results in a crash:

CrashHandlerException: Program crashed
Engine version: Godot Engine v4.0.dev.custom_build (2c7fcdd7f94307a8dbf9436edc3294aaa6f87a88)
Dumping the backtrace. Please include this when reporting the bug on https://github.com/godotengine/godot/issues
[0] CowData<Rect2>::get (C:\Users\Tomek\Desktop\Godot\godot\core\templates\cowdata.h:163)
[1] CowData<Rect2>::get (C:\Users\Tomek\Desktop\Godot\godot\core\templates\cowdata.h:163)
[2] Vector<Rect2>::operator[] (C:\Users\Tomek\Desktop\Godot\godot\core\templates\vector.h:90)
[3] FlowContainer::_resort (C:\Users\Tomek\Desktop\Godot\godot\scene\gui\flow_container.cpp:127)
[4] FlowContainer::_notification (C:\Users\Tomek\Desktop\Godot\godot\scene\gui\flow_container.cpp:209)
[5] FlowContainer::_notificationv (C:\Users\Tomek\Desktop\Godot\godot\scene\gui\flow_container.h:37)
[6] HFlowContainer::_notificationv (C:\Users\Tomek\Desktop\Godot\godot\scene\gui\flow_container.h:70)
[7] Object::notification (C:\Users\Tomek\Desktop\Godot\godot\core\object\object.cpp:848)
[8] Container::_sort_children (C:\Users\Tomek\Desktop\Godot\godot\scene\gui\container.cpp:94)
[9] call_with_variant_args_helper<Container> (C:\Users\Tomek\Desktop\Godot\godot\core\variant\binder_common.h:231)
[10] call_with_variant_args<Container> (C:\Users\Tomek\Desktop\Godot\godot\core\variant\binder_common.h:341)
[11] CallableCustomMethodPointer<Container>::call (C:\Users\Tomek\Desktop\Godot\godot\core\object\callable_method_pointer.h:97)
[12] Callable::call (C:\Users\Tomek\Desktop\Godot\godot\core\variant\callable.cpp:51)
[13] MessageQueue::_call_function (C:\Users\Tomek\Desktop\Godot\godot\core\object\message_queue.cpp:259)
[14] MessageQueue::flush (C:\Users\Tomek\Desktop\Godot\godot\core\object\message_queue.cpp:306)
[15] SceneTree::physics_process (C:\Users\Tomek\Desktop\Godot\godot\scene\main\scene_tree.cpp:424)
[16] Main::iteration (C:\Users\Tomek\Desktop\Godot\godot\main\main.cpp:2657)
[17] OS_Windows::run (C:\Users\Tomek\Desktop\Godot\godot\platform\windows\os_windows.cpp:667)
[18] widechar_main (C:\Users\Tomek\Desktop\Godot\godot\platform\windows\godot_windows.cpp:163)
[19] _main (C:\Users\Tomek\Desktop\Godot\godot\platform\windows\godot_windows.cpp:185)
[20] main (C:\Users\Tomek\Desktop\Godot\godot\platform\windows\godot_windows.cpp:199)
[21] __scrt_common_main_seh (d:\a01\_work\2\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
[22] BaseThreadInitThunk
-- END OF BACKTRACE --

If a child is invisible, it won't be added to child_rect Vector and it goes out of bounds.

scene/gui/flow_container.cpp Outdated Show resolved Hide resolved
scene/gui/flow_container.cpp Outdated Show resolved Hide resolved
scene/gui/flow_container.cpp Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented Jan 5, 2022

The size flags could be handled better.

This is HFlowContainer:
image
The only flag that has effect is vertical Expand.

For the OK button, Horizontal Fill should actually stretch it to all available space instead of using only minsize. Shrink Center and Shrink End could change the horizontal alignment. And actually, if they affected vertical alignment too, the line_alignment property wouldn't be necessary.

To better illustrate what I have in mind:
No flags:
image

Horizontal Fill
image

Horizontal Shrink Center
image

Horizontal Shrink End
image

Vertical Fill
image

Vertical Shrink Center (same as CENTER line alignment)
image

Vertical Shrink End (same as BOTTOM line alignment)
image

Both Fills:
image

The Expand flag would be unused. You can't really push controls in a FlowContainer, no?

Looking at the code, it shouldn't be difficult to implement. Although @pycbouh knows more about flags, so he could comment whether what I propose makes sense.

@Geometror Geometror force-pushed the add-flow-layout-container branch 2 times, most recently from 416be9e to cd551e8 Compare January 5, 2022 16:18
@YuriSizov
Copy link
Contributor

The Expand flag would be unused. You can't really push controls in a FlowContainer, no?

Wouldn't it be useful to enable this behavior explicitly with expand so that you can do this with not just the last child in a row/column, but also with intermediate ones?

In the same row you have "OK" other children could be positioned in the same way, in theory, but without an explicit expand we wouldn't be able to tell them to take the remaining space from the row. Unless shrink center and shrink end do that implicitly, but then you still might want to use stretch ratios and do something fancy there with multiple items, no? And in that case, even shrink begin (no flags) would want to be able to "expand" to reclaim a piece of that space.

@Geometror
Copy link
Member Author

Partially rewritten, so it can respect all size flags (as well as the stretch ratio) and the line_alignment property is no longer needed (as suggested by @KoBeWi and @pycbouh).

It seems to work as expected in all scenarios I tested (also with RTL layout), but further testing is always welcome :)

Animation3

Animation2

@KoBeWi
Copy link
Member

KoBeWi commented Jan 6, 2022

I tested the flags and they seem fine, except the "perpendicular" size shouldn't need expand. E.g. in HBoxContainer, if your control has Fill enabled, it takes whole height. But in HFlowContainer you need to enable Expand. It shouldn't be like that, as it's inconsistent. Verticall Fill in HFlowContainer should make the control take all available height without Expand. The "parallel" behavior (i.e. horizontal flags in HFlowContainer) is correct.

@Geometror
Copy link
Member Author

@KoBeWi Changed it accordingly.

@KoBeWi
Copy link
Member

KoBeWi commented Jan 7, 2022

Good, but now Shrink flags don't work 🙃 (I mean vertical shrink in HFlowContainer)

EDIT:
Also after another look, the "perpendicular" Expand flag might have some use actually. I noticed that the size flags align the controls inside line. With Expand they could be aligned inside whole container, e.g.
image
in this case button with Expand could expand the line, because it has space.
But that's just an idea that might be too complicated to implement. The current behavior is ok.

@Geometror
Copy link
Member Author

Fixed, now it should finally work as expected :)
Regarding the vertical expand flag, I would propose that we see if there is actual demand for that because it would indeed blow up/complicate the code and cause some layouting problems (I think) since the container changes its "perpendicular" size on demand.

scene/gui/flow_container.cpp Outdated Show resolved Hide resolved
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested different layouts. Size flags, stretch ratio, expanding etc. all work as expected.

@groud HFlowContainer is like a Label with autowrap enabled, but lays controls instead of text. It uses all available width and if it doesn't have enough, it will grow its minimum size. VFlowContainer is analogous, but vertically. The container system is robust enough to support this without problems. So unless you can find a (non-corner) case where this really breaks, the PR is good to go.

@Scony
Copy link
Contributor

Scony commented Jan 10, 2022

@Geometror will any of these containers enable one to layout things from the end (bottom-right) to overcome this hack: https://twitter.com/pawel_lampe/status/1373370799319617537 ?

@bruvzg
Copy link
Member

bruvzg commented Jan 10, 2022

@Geometror will any of these containers enable one to layout things from the end (bottom-right) to overcome this hack: https://twitter.com/pawel_lampe/status/1373370799319617537 ?

You can do it with the existing GridContainer, by setting layout direction to right-to-left (but it's available only in 4.0).

@Scony
Copy link
Contributor

Scony commented Jan 10, 2022

@Geometror will any of these containers enable one to layout things from the end (bottom-right) to overcome this hack: https://twitter.com/pawel_lampe/status/1373370799319617537 ?

You can do it with the existing GridContainer, by setting layout direction to right-to-left (but it's available only in 4.0).

yeah, but I don't know how many columns I need beforehand. Thus FlowContainer will be perfect - as long as it allows one to set layout direction.

@KoBeWi
Copy link
Member

KoBeWi commented Jan 10, 2022

FlowContainer from this PR also supports layout direction, but there's no bottom-to-top option.

@akien-mga akien-mga merged commit 030638e into godotengine:master Jan 18, 2022
@akien-mga
Copy link
Member

Thanks!

@groud
Copy link
Member

groud commented Jan 19, 2022

We discussed in length this PR on Rocket.Chat yesterday, as I wanted to figure out why this FlowContainer seems to work correctly, while, theoretically, it should not. I'll express my doubts here so that they are documented, in case we face issues later on.

To compute its children position and size, a Container relies on its children size flags and their minimum size returned by get_minimum_size(). In the case of this FlowContainer implementation, it's minimum size is computed out of these lines:

	minimum.width = MAX(minimum.width, size.width);
	minimum.height = cached_size;

where cached_size is computed within the _resort function.

In this _resort function, called when the FlowContainer is resized, we have int current_container_size = vertical ? get_rect().size.y : get_rect().size.x;, which basically relies of the FlowConatiner actual size. This value is then used to compute the FlowContainer's children position and size, but also to compute cached_size.

All of this to explain that, to compute the minimum size of a FlowContainer (returned by get_minimum_size()), we rely one way or another on its actual size. This is very risky, as we might end up with unexpected behaviors, as in means a parent Container would set the FlowContainer's size depending on the current FlowContainer's size. This is basically feedback loop which can lead to very unexpected behaviors.

For example, something like this:

Size2 MyControl::get_minimum_size() {
    return size + Size2(1,1);
}

would cause MyControl to grow to infinity as soon as it is put into a Container. Thus relying on the actual size to compute a minimum size is quite a bad thing in general.

In the case of this PR, I don't know why such behavior does not happen, and why the size stays stable. I suppose it might come from the fact that only one component (out of x and y) is impacted, causing the values to stabilize. Or something else, I don't know.

Also, as a side note, we also have a similar behavior with the Label node with the "auto wrap" mode, and it works there too. I don't know if we are going to have problems there too, but we should likely keep an eye on issues that could arise.

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.

None yet