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

Added support for flipping BoxContainer children #35791

Conversation

pouleyKetchoupp
Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp commented Jan 31, 2020

This change adds a flip_order property which allows inverting the container's children positions without changing the order they are rendered.

Flip Order off Flip Order on

Bugsquad edit: This closes godotengine/godot-proposals#804.

This change adds a flip_order property which allows inverting the container's
children positions without changing the order they are rendered.
@groud
Copy link
Member

groud commented Jan 31, 2020

I don't see why this cannot be done by moving child to the first place instead. As it can be worked around with literally one line of code I don't think it is worth adding this option.

@Zireael07
Copy link
Contributor

It's about inverting all of the children, not just one.

@groud
Copy link
Member

groud commented Feb 1, 2020

It's about inverting all of the children, not just one.

Yeah, as I said it is pointless. You can simply, when adding children to the node, just move them to the correct spot :

add_child(node)
move_child(node, 0)

The only point I see adding this is something we discussed at Godot sprint. It might be useful when you have right-to-left languages, but the solution might involve automatically changing the order depending on the locale. I don't see any other use case that cannot be worked around by the snippet above.

@pouleyKetchoupp
Copy link
Contributor Author

If you change the order of the children, you change the order they are rendered. So the case I'm showing in the second screenshot (top icon rendered in front) is something you can't achieve without my change, as far as I know.

@groud
Copy link
Member

groud commented Feb 1, 2020

Thing is this use case with overlapping objects is pretty rare, you almost never need that. For 99% of cases the rendering order does not matter as BoxContainers are meant to put objects next to each other at their minimum size, without them overlapping. So I still don't think it is worth adding that only to cover this use case.

@pouleyKetchoupp
Copy link
Contributor Author

A case where you have a stack of tokens or cards is actually very common. So this change is pretty useful and it does something you can't achieve otherwise.

The fact it was originally built for a specific purpose shouldn't limit it from achieving more use cases.

@groud
Copy link
Member

groud commented Feb 1, 2020

A case where you have a stack of tokens or cards is actually very common.

Sorry but not really, there are card games, but usually you need to drag an animate the cards, something which is out of the scope of a BoxContainer. Worst case, you can still implement your own container if you want to, it's pretty easy to do.

The fact it was originally built for a specific purpose shouldn't limit it from achieving more use cases.

It's not how we accept new features. It has to be proven useful for a significant amount of use case, or be hard to workaround. This proposal is none of those.

@pouleyKetchoupp
Copy link
Contributor Author

Is this feature useful in a significant amount of use cases?

  • It can be used for any stacks of overlapping sprites, from piles of coins to cards. It can be used as is, with no animation.
  • As you mentioned, it's also useful for localization in certain languages.
  • Even for non-overlapping cases, it's a one button click and script property shortcut for sorting lists in a certain order.

Is it hard to workaround?

  • In order to achieve it by script, even with custom containers, a user would have to re-implement the whole logic for placing children according to their current size (it takes ~200 lines of C++ code). Lots of users don't have the immediate skills to do this.

Given this change is only a few lines of engine code and given the reasons above, I don't see why it would be so strongly rejected even when reactions seem positive.

@groud
Copy link
Member

groud commented Feb 1, 2020

Those are still too specific cases. Adding feature for rare use cases is not worth the maintainance work.

You don't have to copy the whole BoxContainer system to make it works. You can even use rendering index if you want.

The problem is that there are plenty of really simple ways to solve your problem, while the problem you have is for a use case for which BoxContainer is not meant to be used. The GUI system is not meant for overlappings elements, this is something it was not designed for at all.

So for me the equation is simple, we have:

  • a rare use case (only the second one you gave is interesting, but you solution is not how we would implement it, since we want it to be automatic depending on the locale)
  • something that can be worked around in plenty of ways with really few lines of code.
    Thus it's not worth the trouble of supporting this new feature as built-in.

@pouleyKetchoupp
Copy link
Contributor Author

Can you be more specific about the solution involving rendering index? I know Node2D has a z-index property, but Control doesn't so I don't know how that would work.

@groud
Copy link
Member

groud commented Feb 1, 2020

Oh sorry, I thought Control nodes had it too. Probably it could be made into Control nodes too.

@pouleyKetchoupp
Copy link
Contributor Author

Ok, I can see where the confusion came from. Yes, it means until we have a way to change the rendering order of Control nodes, I don't see a simple solution to handle the use case presented in the screenshots.

@groud
Copy link
Member

groud commented Feb 1, 2020

I don't see a simple solution to handle the use case presented in the screenshots.

Here you go, 7 lines of code:

extends Container

func _notification(what):
	if (what==NOTIFICATION_SORT_CHILDREN):
		var i = 0
		for c in get_children():
			fit_child_in_rect(c, Rect2( Vector2(0, -20 * i), c.texture.get_size()))
			i += 1

To put on a base Container node

@Calinou
Copy link
Member

Calinou commented Feb 1, 2020

Oh sorry, I thought Control nodes had it too. Probably it could be made into Control nodes too.

This was discussed in #7692 a while ago.

@pouleyKetchoupp
Copy link
Contributor Author

@groud Ok, I agree. It's possible to re-implement by script a simple version of BoxContainer that doesn't have any of its other functionalities.

I'm closing this PR since it's not desired. I'll keep using it on my personal repo because it's very convenient to have it built-in :)

It's still possible to re-open this PR in the future if, let's say, we need a simple way to allow ui flipping for localization purpose or left/right handed input for 3.2, while the more complex feature is implemented for 4.0.

@groud
Copy link
Member

groud commented Feb 2, 2020

You can keep the PR open, I am not the only one to decide ^^

@pouleyKetchoupp
Copy link
Contributor Author

Ok then :)

@aaronfranke
Copy link
Member

I don't see the value in this feature. Overlapping items in containers is not the intended usage. I don't think it's worth adding this functionality to BoxContainer if it's only useful for using BoxContainers for purposes it wasn't intended for, such as card games.

It would make more sense for the handling of cards and other gameplay-related elements to be done by user code. Also, if you need to control the Z-index, use Node2D instead of Control, since Control nodes have their Z-index determined by having children on top (and the intention is for you to not have siblings overlap each other).

@donut-on-ice
Copy link

There are cases where you want UI elements in a box to overlap. If this was not intended then maybe the option to have negative values for separators in vboxes and hboxes should be removed.

This being said, I find the options usefull (both the rendered order flip and the overlaping UI elements) since the easiest alternative is to read the box container code and implement your own drawing logic, which is not an easy task for a developer with light programing background

@volzhs
Copy link
Contributor

volzhs commented May 26, 2020

If it is useful, it needs to be with Node or Control, not only with BoxContainer.

@donut-on-ice
Copy link

I am not sure if it would be useful or make sense in any other setting than box container

@Calinou Calinou added this to the 4.0 milestone Jan 5, 2021
@YuriSizov YuriSizov modified the milestones: 4.0, 4.x Feb 12, 2023
@aaronfranke
Copy link
Member

Closing because this PR has conflicts and the original author is no longer interested in working on it. However, if anyone wants this feature, you are welcome to salvage it and work on it further.

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.

Add an "Inverse Sort" property to BoxContainer
9 participants