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

Rework oriented containers #64724

Merged
merged 1 commit into from
Sep 2, 2022
Merged

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Aug 22, 2022

This PR makes BoxContainer, SplitContainer and FlowContainer non-abstract, so you can change the orientation on the fly using a vertical property:
godot windows tools 64_pYlwvVURZs

The property is not available in the specialized classes and trying to change it from code in these classes results in an error:
image
image

Related to godotengine/godot-proposals#3558
Helps #36040

@KoBeWi KoBeWi added this to the 4.x milestone Aug 22, 2022
@KoBeWi KoBeWi requested review from a team as code owners August 22, 2022 11:16
@YuriSizov
Copy link
Contributor

YuriSizov commented Aug 22, 2022

Love the icons 😄

Personally, I like the approach here, with keeping the variations available and making them non-mutable. But I suspect not everyone will like that.

@Mickeon
Copy link
Contributor

Mickeon commented Aug 22, 2022

I love it! It keeps the dimensions appropriately when flipping coordinates, it's very clever. Probably the more expected behaviour that can be easily overridden by flipping x and y back in script.

@YeldhamDev
Copy link
Member

I don't get it. Why keep the H/VBoxContainer nodes with this change?

@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 22, 2022

Because H/VBoxContainer have more clear usage than some "BoxContainer". Or at least that was the reasoning why unifying it was originally rejected.

@MewPurPur
Copy link
Contributor

MewPurPur commented Aug 22, 2022

To me it's useful to be able to look in the SceneTree dock and see the orientation and how containers differ, rather than a soup of diagonal containers that doesn't tell much. There is no way to change the icon based on a property atm, the technology isn't there yet

Edit: But to be honest, I would only use BoxContainer if displaying a different icon based on orientation ever becomes possible.

@YeldhamDev
Copy link
Member

Because H/VBoxContainer have more clear usage than some "BoxContainer". Or at least that was the reasoning why unifying it was originally rejected.

Then I just don't see the point of this PR. If anything, it would just make things confusing.

@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 22, 2022

The PR doesn't change much. Users can still use H/VBoxContainers, advanced users can use BoxContainer. The only difference is that BoxContainer is no longer grayed-out and you can click it. Maybe the intended usage could be better clarified in the docs, but other than that it's fine IMO.

@@ -73,7 +79,7 @@ class HBoxContainer : public BoxContainer {

public:
HBoxContainer() :
BoxContainer(false) {}
BoxContainer(false) { is_fixed = true; }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
BoxContainer(false) { is_fixed = true; }
BoxContainer(false),
is_fixed(true) {}

Same for others.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does not work, I get C2614 illegal member initialization: 'is_fixed' is not a base or member error :/

Copy link
Member

Choose a reason for hiding this comment

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

Ah that's weird. I guess it's fine as is then.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good to me, up to others in @godotengine/gui-nodes to approve the concept.

@KoBeWi KoBeWi force-pushed the HVBoxContainer branch 2 times, most recently from 2b6134e to 1e8aa99 Compare August 24, 2022 15:39
@akien-mga akien-mga modified the milestones: 4.x, 4.0 Sep 2, 2022
@akien-mga akien-mga merged commit 5a136ee into godotengine:master Sep 2, 2022
@akien-mga
Copy link
Member

Thanks!

@KoBeWi KoBeWi deleted the HVBoxContainer branch September 2, 2022 09:13
@sairam4123
Copy link

What about other H/V classes?

4d49 added a commit to 4d49/object-inspector that referenced this pull request Sep 8, 2022
@KoBeWi
Copy link
Member Author

KoBeWi commented Sep 8, 2022

They are not container.

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.

8 participants