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 AspectRatioContainer class #43807

Merged
merged 1 commit into from Nov 26, 2020
Merged

Add AspectRatioContainer class #43807

merged 1 commit into from Nov 26, 2020

Conversation

Xrayez
Copy link
Contributor

@Xrayez Xrayez commented Nov 24, 2020

Closes godotengine/godot-proposals#69.
Based on work by @UgisBrekis in #32169, co-authored-by: @UgisBrekis (this will be reflected in commit history).

image

Main differences with #32169:

  1. Alignment is changed to enum type rather than scalar, to achieve consistency with HBox/VBox containers. This actually made the implementation more complex, but this is the consensus I've inferred from New container type: Aspect Ratio Container godot-proposals#69 as stated by @groud there Added new container type: AspectRatioContainer #32169 (comment):

    Ah indeed you are right and I was wrong here. But in that case, I do think this option could be removed from the H/VBoxContainers.
    Edit: though I am not sure there is a simple way to achieve the same result without this option. The fact that those containers do not shrink automatically to the smallest size might makes things complex

    Also, if we want it to be added, it will have to be consistent with the HBox and VBox containers, so it will have to be a enum. Consistency is more important here.

    And I actually lean towards solution proposed by @UgisBrekis here. Initially, I've even removed those aligntment_x and alignment_y properties altogether, but the default alignment behavior was quite confusing, and I couldn't figure out how to do the alignment properly without those options, so I think those properties are indeed needed (even if they're just enum properties), else it would be quite confusing to beginners to figure this out (even if there's a way to achieve desired alignment without those properties). I think this kind of reasoning also applies to existing HBox/VBox containers, to make usage intuitive out of the box.

  2. alignment_x/y renamed to alignment_horizontal/vertical, because they are grouped in the inspector anyway.

  3. Generated and written documentation, based on description in New container type: Aspect Ratio Container godot-proposals#69.

  4. Various code/convention/consistency fixes and minor typos.

There's no editor icon, but perhaps someone could design one (or perhaps there's existing one which I left unnoticed).

@Xrayez Xrayez requested a review from a team as a code owner November 24, 2020 00:07
@akien-mga akien-mga added this to the 4.0 milestone Nov 24, 2020
Copy link
Member

@groud groud left a comment

Choose a reason for hiding this comment

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

My comments on the original PR are still valid here. I still think those alignment options are useless.
I'd be happy proven wrong, but I see no real life use case for those.

For now, I would make the centered behavior the default, and if anyone comes up with a real life use case, we could add it again.

@Xrayez
Copy link
Contributor Author

Xrayez commented Nov 24, 2020

Use case: creating thumbnails

AspectRatioContainer as elements of GridContainer

Ratio: 2
Stretch Mode: Cover
GridContainer/Custom Constants/VSeparation: 50
GridContainer/Custom Constants/HSeparation: 100

Alignment Horizontal: Center (default)

image

In this case, the frame aligned with AspectRatioContainer goes beyond origin, which is undesired in most cases due to potential clipping.

Alignment Horizontal: Begin

image

OK, this would be fine even with ScrollContainer on the rightmost side because I can dynamically manage the needed number of columns to fit frames dynamically now.

I hope you're happy now. 🙂

Test project:
thumbnails.zip

@Xrayez Xrayez requested review from groud and removed request for a team November 24, 2020 14:11
@groud
Copy link
Member

groud commented Nov 24, 2020

Ok, I don't think this example is the best one, but I guess I have the idea. I think I see a use case in the covered mode. If you want a thumbnail inside an AspectRatioContainer, with clipping enabled, you might want to be able to select the part of the thumbnail you want to show, not only the center. So it makes sense to keep this configurable, I agree.

I believe the only thing missing is to squash the commits together (please keep @UgisBrekis as co-author though).

I hope you're happy now. slightly_smiling_face

Making me happy is not important, it's making me convinced that is. 😉

Copy link
Member

@groud groud left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@Xrayez
Copy link
Contributor Author

Xrayez commented Nov 24, 2020

I have updated the PR to include a new icon:

image

This is a modification of Container.svg icon, IMO it should be good enough than no icon, if someone wants to change this later, feel free. 🙂

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.

It's not clear from the documentation how the container handles multiple children, as it was written in the perspective of a single child node it seems.

doc/classes/AspectRatioContainer.xml Outdated Show resolved Hide resolved
doc/classes/AspectRatioContainer.xml Outdated Show resolved Hide resolved
doc/classes/AspectRatioContainer.xml Outdated Show resolved Hide resolved
doc/classes/AspectRatioContainer.xml Outdated Show resolved Hide resolved
doc/classes/AspectRatioContainer.xml Outdated Show resolved Hide resolved
The width of child is automatically adjusted based on the height of the container.
</constant>
<constant name="STRETCH_FIT" value="2" enum="StretchMode">
The width and height of child is automatically adjusted to make the rect fit inside the rect of the container while keeping the aspect ratio.
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
The width and height of child is automatically adjusted to make the rect fit inside the rect of the container while keeping the aspect ratio.
The width and height of child controls is automatically adjusted to make their rect fit inside the rect of the container while keeping the aspect ratio.

Not sure if "rect" is clear enough here (and would likely be hard to localize), but "boundary rectangle" is a bit heavy too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would be "bounding rectangle". To avoid verbosity, I rephrased this to:

The bounding rectangle of child controls is automatically adjusted to fit inside the container while keeping the aspect ratio.

doc/classes/AspectRatioContainer.xml Outdated Show resolved Hide resolved
doc/classes/AspectRatioContainer.xml Outdated Show resolved Hide resolved
doc/classes/AspectRatioContainer.xml Outdated Show resolved Hide resolved
doc/classes/AspectRatioContainer.xml Outdated Show resolved Hide resolved
@aaronfranke
Copy link
Member

For the icon, since aspect ratios are often wide, perhaps the icon should be wide?

AspectRatioContainer AspectRatioContainerHorizontal.zip

However, I think it would be better to make a new icon, maybe with diagonal arrows or something?

Co-authored-by: Ugis Brekis <ugis.brekis@productmadness.com>
@Xrayez
Copy link
Contributor Author

Xrayez commented Nov 25, 2020

For the icon, since aspect ratios are often wide, perhaps the icon should be wide?

While dealing with aspect ratios, what comes to my mind first is: A4 paper, smartphones, tables, more like 3:4 ratio in that sense, not widescreen 4k displays.

In fact, the 3:4 ratio would only make the GUI more compact, even if it's only visual.

However, I think it would be better to make a new icon, maybe with diagonal arrows or something?

Sure, that's why I mentioned that it would be probably best if someone could enhance the existing version after this PR, or come up with a completely new one. 🙂

@akien-mga akien-mga merged commit a8c2cc9 into godotengine:master Nov 26, 2020
@akien-mga
Copy link
Member

Thanks!

@Xrayez Xrayez deleted the aspect-ratio-container branch November 26, 2020 11:20
@dreamsComeTrue
Copy link
Contributor

Please, make it to the 3.x branch as well @akien-mga :) It's urgently needed there.
Great work!

@Xrayez
Copy link
Contributor Author

Xrayez commented Dec 14, 2020

I can backport this to 3.2, upon approval of course.

This feature has been listed on goostengine/goost#7, so it would make more sense if this could be made available in Godot.

Also note that the original author missed the feature freeze in 3.2: godotengine/godot-proposals#69 (comment).

@AnEnigmaticBug
Copy link

I would really love to have AspectRatioContainer in Godot 3.2. Since Godot 4 is still months away, it makes sense to add it. Due to its absence, people have been using workarounds. Each one containing needlessly duplicated code and (potentially) bugs.

@boukew99
Copy link

How is this as an icon?
AspectRatioContainer
It looks a bit like a frame with the inner rectangle being golden ratio (10 x 6).

@Calinou
Copy link
Member

Calinou commented Jul 10, 2021

How is this as an icon?
AspectRatioContainer
It looks a bit like a frame with the inner rectangle being golden ratio (10 x 6).

These are the current icons:

image

@Xrayez What do you think about the icon proposed above?

@aaronfranke
Copy link
Member

@boukew99 Do you have an SVG version of this icon?

@boukew99
Copy link

boukew99 commented Jul 10, 2021

@boukew99 Do you have an SVG version of this icon?

no

@Xrayez
Copy link
Contributor Author

Xrayez commented Jul 10, 2021

The problem is that 16x16 icons don't look nice if an icon has too many details, and that makes it problematic to represent golden ratio (if this was the original goal), especially when coordinates have to be snapped to pixels/integers so that they don't look blurry in the editor. It's difficult to say whether proposed icon would look good or bad in the editor, this is why SVG source is required to test this. I'm fine with the current icon, and the proposed icon looks fine at first glance as well, but might need modifications.

But I'm just a regular contributor, lets see what members say about this.

@boukew99 Do you have an SVG version of this icon?
no

See Editor icons documentation if you'd like to learn how to create SVG icons for the editor.

@boukew99
Copy link

The problem is that 16x16 icons don't look nice if an icon has too many details, and that makes it problematic

I don't know about all that, but made some svg's as documented. Made a couple variations so that you have options. Note that the icons are not optimized yet and I used a slightly transparent color for the inner rectangle to distinguish it (except for 4), don't know if this is a problem.
AspectRatioContainerIcons.zip

@boukew99
Copy link

afbeelding
done with class_name

@boukew99
Copy link

afbeelding
Indicate golden ratio with negative space, altough the thick borders do not seem to fit very well.

@boukew99
Copy link

afbeelding
A bit like those black aspect ratio bars

@boukew99
Copy link

afbeelding
Accidently made NinePathRect with the rule of thirds :0

@boukew99
Copy link

afbeelding
Last one, don't know what else to try

@Zireael07
Copy link
Contributor

The first or the last would be the best imho.

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.

New container type: Aspect Ratio Container
10 participants