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

New container type: Aspect Ratio Container #69

Closed
UgisBrekis opened this issue Sep 14, 2019 · 21 comments · Fixed by godotengine/godot#43807
Closed

New container type: Aspect Ratio Container #69

UgisBrekis opened this issue Sep 14, 2019 · 21 comments · Fixed by godotengine/godot#43807
Milestone

Comments

@UgisBrekis
Copy link

Describe the project you are working on:
Aspect Ratio Container is equivalent of Unity's Aspect Ratio Fitter- it resizes child Control nodes based on two parameters: ratio and stretch mode.

Ratio: The aspect ratio to enforce on child Control nodes. This is the width divided by the height.
Stretch Mode:

  • Width controls height: The height of child Control nodes is automatically adjusted based on the width of the container.
  • Height controls width: The width of child Control nodes is automatically adjusted based on the height of the container.
  • Fit: The width and height of child Control nodes are automatically adjusted to make the rect fit inside the rect of the container while keeping the aspect ratio.
  • Fill: The width and height of child Control nodes are automatically adjusted to make the rect cover the entire area of the container while keeping the aspect ratio

Additionally there are Alignment X and Alignment Y properties which influence the position of child Control nodes.

Describe how this feature / enhancement will help your project:
Even though Godot has a great set of Control nodes, there is no way to achieve the same outcome without scripting a custom container node, which creates unnecessary barrier for the user. Aspect Ratio Container solves the problem where container size is dynamic and content size needs to adjust accordingly without loosing proportions. Somewhat similar feature is seen on TextureRect, however it is limited to a texture only and it doesn't offer alignment options.

Show a mock up screenshots/video or a flow diagram explaining how your proposal will work:

https://youtu.be/PZlKiujqHIY

Describe implementation detail for your proposal (in code), if possible:
if (p_what == NOTIFICATION_SORT_CHILDREN) {

	Size2 size = get_size();
	for (int i = 0; i < get_child_count(); i++) {

		Control *c = Object::cast_to<Control>(get_child(i));
		if (!c)
			continue;
		if (c->is_set_as_toplevel())
			continue;

		Vector2 offset = Vector2();
		Size2 child_minsize = c->get_combined_minimum_size();
		Size2 child_size = Size2(ratio, 1.0);
		float scale_factor = 1.0;

		switch (stretch_mode) {
			case WIDTH_CONTROLS_HEIGHT: {
				scale_factor = size.x / child_size.x;
			} break;

			case HEIGHT_CONTROLS_WIDTH: {
				scale_factor = size.y / child_size.y;
			} break;

			case FIT: {
				scale_factor = MIN(size.x / child_size.x, size.y / child_size.y);
			} break;

			case FILL: {
				scale_factor = MAX(size.x / child_size.x, size.y / child_size.y);
			} break;
		}

		child_size *= scale_factor;
		child_size.x = MAX(child_size.x, child_minsize.x);
		child_size.y = MAX(child_size.y, child_minsize.y);

		offset = (size - child_size) * Vector2(alignment_x, alignment_y);

		fit_child_in_rect(c, Rect2(offset, child_size));
	}
}

If this enhancement will not be used often, can it be worked around with a few lines of script?:
This is not a highly complex feature, so it can be worked around with a few lines of code, however the aim for it is to complement existing set of Control nodes in order to remove the necessity to write any code.

Is there a reason why this should be core and not an add-on in the asset library?:
It is one of the core UI building blocks, it is too small to be distributed as a plugin and it greatly complements the existing Container nodes.

I have already implemented the feature on my own fork: https://github.com/UgisBrekis/godot/tree/AspectRatioContainer

@willnationsdev
Copy link
Contributor

willnationsdev commented Sep 15, 2019

This looks like a cool addition to me. I could already imagine using this for cutscene editing purposes, moving ViewportContainers around inside of these by animating the alignments and making sure that the cutscenes feel more cinematic by adjusting the aspect ratio more conveniently.

@UgisBrekis If people don't like the idea of adding it to the engine then hit me up and we can add it to godot-next where something like this would be right at home.

@girng
Copy link

girng commented Sep 16, 2019

If this can't be done with the layout presets, margins, etc, etc already... I think it's a good idea and useful for UI development. Nice video showing it in action, and a very well-written post.

edit: Not sure on the quantity of use-cases though as compared to if it's find as an addon node. However, I do believe once it's added, it will have its use-cases since more godot devs can utilize it for UI dev?

@groud
Copy link
Member

groud commented Sep 16, 2019

The anchors and margins workflow is meant to cover this exact use case.
Nevermind, the video fixed my understanding ^^

@CptPotato
Copy link

Small suggestion: You can add an option to the container that will simply stretch the contents if the stretch-factor falls below a certain percentage. I'm using this for scaling my game's content to the window size: https://youtu.be/3pbmiNMSTv4
It's a bit of a specific use-case, though.

@Xrayez
Copy link
Contributor

Xrayez commented Jan 29, 2020

@UgisBrekis I'm currently writing a plugin/prototype for #397. I want to restrict the vector editing area to represent a square, but I want containers to let it grow as needed while remaining its aspect ratio. Do you think that it would be easy to achieve with your implementation? See current state at #397 (comment).

@Castlemark
Copy link

Castlemark commented Jan 30, 2020

Looks very useful for some scenarios (mainly UI), if I could suggest something, it would be to add the option to clip the contents of the children that are outside the control's rect (similar to how the Keep Aspect Covered stretch mode in TextureRect works).

texturerect

@Xrayez
Copy link
Contributor

Xrayez commented Sep 9, 2020

What's the status? Almost a year passed!

I think this could be a nice addition, too bad no consensus was reached in godotengine/godot#32169.

I'm working on a C++ module (Goost) which can benefit from this kind of thing in Godot 3.2, and it seems like you've already implemented this in C++ in godotengine/godot#32169 (which was made on Godot 3.2 alpha), so perhaps consider contributing if still interested?

I'm not sure if @groud already plans to add such a node in 4.0, or perhaps @groud changed his mind and this should be implemented as a plugin/module to prevent Godot from bloating? Adding new features creates unnecessary maintenance burden on Godot core developers, so I don't know, it would still be nice to see this feature implemented in Godot.

What do you think?

@groud
Copy link
Member

groud commented Sep 9, 2020

I'm not sure if @groud already plans to add such a node in 4.0, or perhaps @groud changed his mind and this should be implemented as a plugin/module to prevent Godot from bloating? Adding new features creates unnecessary maintenance burden on Godot core developers, so I don't know, it would still be nice to see this feature implemented in Godot.

No, this node is a good idea. There was only on small disagreement in the PR implementation.
But since the author did not make the asked modifications, I might try to get his code and salvage it. :)

@groud groud self-assigned this Sep 9, 2020
@UgisBrekis
Copy link
Author

Sorry everyone for abandoning this thing- back then I was really looking forward to getting this into 3.2, but missed the deadline. If that’s ok @groud I’d love to actually finish this myself, it would be a good excercise at contributing to Godot source code.

@groud
Copy link
Member

groud commented Sep 11, 2020

@UgisBrekis Sure, no problem. :) Don't hesitate to ping me when the PR is ready to be reviewed again.
(If there was any doubt about it, if I had to rebase the code myself, I would indeed have kept you as commit author).

@Xrayez
Copy link
Contributor

Xrayez commented Sep 19, 2020

But since the author did not make the asked modifications, I might try to get his code and salvage it. :)

@groud It's good to hear that we're going in some direction with this proposal, but "might" conveys a different meaning than "will", in my opinion (English is far from my native language). I'd remove myself from Assignees if I'm not actually willing to implement something, else this creates a wrong intention, a false promise that something will be indeed implemented, but then it blocks the decision making of other parties if this isn't actually worked on. I'd like to see how long it would actually take for this proposal to be implemented, else could literally take another year. I'm sorry to vent like this but I have to, because it seems like we don't have enough core developers to keep up with the review/development pace.

I’d love to actually finish this myself, it would be a good exercise at contributing to Godot source code.

@UgisBrekis yeah, though there's been quite a lot of core changes since Godot 3.2 in the master branch, nobody has written a migration guide yet either, but you could take a look at the existing codebase for examples.


I mean, if we're going with the direction of removing features from Godot, like with the recently deprecated InterpolatedCamera: godotengine/godot#42113 (already removed in Godot 4.0), and I suspect the reason behind that was exactly the fact that such functionality is difficult to make general-purpose enough to suite most use cases, similar reasoning could be applied to the proposed AspectRatioContainer, and the fact that the proposed implementation godotengine/godot#32169 received a "small disagreement" only adds to the "not general purpose enough". I might be exaggerating but this is the way I see the situation.

@groud I know you're into/specialize in UI development in Godot, so I'm just trying to reveal possible prejudice/favoritism when you say that this is useful (...to be included in core?), but there is some objective ground which just goes against that. I mean, as with the InterpolatedCamera example, isn't that useful too? Then why remove? I'm actually quite surprised to hear that you find something useful, because all I've been hearing from you is "specific, specific, specific", again sorry but I'm just pointing to your typical behavior here.

The amount of code it takes to re-implement InterpolatedCamera via script would be almost the same as with AspectRatioContainer, and takes only a "handful" lines of code for both.

Yet still, as I said, I'd find this useful for my own use cases.

I'm confused, in what direction we're going again? #575 😕

@Calinou
Copy link
Member

Calinou commented Sep 19, 2020

@Xrayez For the InterpolatedCamera3D case, I'm in the process of porting it as an add-on (with more features): https://github.com/Calinou/godot-interpolated-camera3d

This way, people can receive updates and fixes to the InterpolatedCamera3D node without having to wait for new Godot releases.

@groud
Copy link
Member

groud commented Sep 20, 2020

@groud It's good to hear that we're going in some direction with this proposal, but "might" conveys a different meaning than "will", in my opinion (English is far from my native language). I'd remove myself from Assignees if I'm not actually willing to implement something, else this creates a wrong intention, a false promise that something will be indeed implemented, but then it blocks the decision making of other parties if this isn't actually worked on. I'd like to see how long it would actually take for this proposal to be implemented, else could literally take another year. I'm sorry to vent like this but I have to, because it seems like we don't have enough core developers to keep up with the review/development pace.

Godot's development is made on a best effort basis. I have things to do I my current life that may prevent me to implement something, whether it's my motivation or the time to do so. That's why I used "might", but put myself as an assignee as a reminder to implement it. Like on any proposal/issue with an assignee, anyone is free to ask if they can work on a feature they want to be implemented. Having an "assignee" is by no mean a guarantee that a job is going to be done.

As I am going to be a paid contributor, this my change for my personal case, as my job is literally going to be implementing stuff for Godot. However, we will have indeed to discuss with other core contributors what are going to be the jobs I should focus on first, so any task like this one could not be decided as a priority.

@groud I know you're into/specialize in UI development in Godot, so I'm just trying to reveal possible prejudice/favoritism when you say that this is useful (...to be included in core?), but there is some objective ground which just goes against that. I mean, as with the InterpolatedCamera example, isn't that useful too? Then why remove? I'm actually quite surprised to hear that you find something useful, because all I've been hearing from you is "specific, specific, specific", again sorry but I'm just pointing to your typical behavior here.

I don't really see your point... When I give my opinion on something I give it case by case. I might indeed favor proposals I agree with, but that's all. I, regarding my own experience, consider the pros and cons, then give my opinion. it's indeed subjective as I have my own opinion on what should Godot look like, and what it should not look like.

I can't tell about the InterpolatedCamera example, I suppose it's being removed because you can replace its behavior using a Tween? thus reducing the bloat? But those are only suppositions, I don't know.

Regarding this proposal, it's simple, easy to understand, has a single, well defined scope while still covering what seems to me a common use case. So I don't see any blocking point there. But if you do, feel free to express them.

But to end on this point, you should drop trying to understand "the absolute philosophy" behind Godot's development, there's none. The only real philosophy behind it is that it is an open source project made by several individuals thinking by themselves, who disagrees all the time but spend countless hours discussing to end up to an agreement. Most decisions are made one after the other, and with our blurry roadmap, we know more or less what to do "in general", while sometimes going the other way when we think it's worth it.

Consequently, each decision has to be made case by case, that's why we created this repository to discuss things topic per topic. However, if writing a proposal, risking being refused, is still a problem, I would highly suggest first asking on IRC at #godotengine-devel, pinging the persons you know are "in charge" of the proposal you're about to make. That's what I do all the times when I have ideas about a new feature. It avoided me a lot of work, as many of my ideas got refused there before even opening a detailed proposal.

@Xrayez
Copy link
Contributor

Xrayez commented Sep 20, 2020

@groud thanks for your honesty, you confirmed my observations regarding what you've said, but I'm sorry to hear that.

@Xrayez
Copy link
Contributor

Xrayez commented Nov 22, 2020

Godot's development is made on a best effort basis. I have things to do I my current life that may prevent me to implement something, whether it's my motivation or the time to do so. That's why I used "might", but put myself as an assignee as a reminder to implement it. Like on any proposal/issue with an assignee, anyone is free to ask if they can work on a feature they want to be implemented.

@groud any updates on this? I'm asking because you're still under Assignees, and it's been 3 months since the last interaction. If you don't, I'd like to work on this (or any other contributor for that matter, perhaps the author of this proposal would still like to continue).

Of course it would be better if you could describe the way this being implemented, so we can eliminate those small disagreements, I hope you understand what I'm trying to say if I'm not being clear enough, if not then I'm sorry.

Having an "assignee" is by no mean a guarantee that a job is going to be done.

I don't see the point then. If the only reason why you use Assignee feature is to remind yourself to do it, there are actually better tools for this: Calendars, To-do apps etc, you could probably even use the "Projects" workflow:

image

(but it seems like it's disabled in godotengine)

Currently all this does is preventing other contributors from working on a feature, because allegedly someone is already assigned to do it. That's why I've previously suggested you to remove yourself from assignees if you don't intend to implement this in the near future.

@groud
Copy link
Member

groud commented Nov 23, 2020

@groud any updates on this? I'm asking because you're still under Assignees, and it's been 3 months since the last interaction. If you don't, I'd like to work on this (or any other contributor for that matter, perhaps the author of this proposal would still like to continue).

No updates for now, it's a not a priority work for now. You can work on it.

Of course it would be better if you could describe the way this being implemented, so we can eliminate those small disagreements, I hope you understand what I'm trying to say if I'm not being clear enough, if not then I'm sorry.

See the PR itself, I asked there about the removal of a unneeded property. Otherwise the original PR seemed good.

@Xrayez
Copy link
Contributor

Xrayez commented Nov 23, 2020

No updates for now, it's a not a priority work for now. You can work on it.

@groud I can start working on this once you remove yourself from Assignees.

I'm not a member in Godot so I cannot self-assign myself. Even if I could, you would have to un-assign yourself in any case.

You've assigned yourself and don't put any work on this (especially being hired recently), what goals do you pursue?

Otherwise then I'm sorry but I see this kind of behavior unprofessional.

My definition of "professional" is:

  1. If I start something, I finish it. In this case, the work starts from the moment of assigning yourself to do the job.
  2. If I know I cannot finish the job, I explicitly say so. That way, the work is done in timely manner by other people that may be more knowledgeable in a particular area.

The only real philosophy behind it is that it is an open source project made by several individuals thinking by themselves, who disagrees all the time but spend countless hours discussing to end up to an agreement.

Yes, I think I'm experiencing this now. 😛

@groud
Copy link
Member

groud commented Nov 23, 2020

Alright, I don't want to bikeshed with you any longer. You wasted enough of my time.

@groud groud removed their assignment Nov 23, 2020
@akien-mga
Copy link
Member

akien-mga commented Nov 23, 2020

@Xrayez You presume of the meaning of the "assigned" status in the Godot project. We tend to use it to signify who is in charge of the decision making on a specific area, not necessarily who is going to actually do the implementation work. We have hundreds of contributors and can't have everyone in the GitHub org to be assign-able, so only area owners end up being assigned to let them keep track of things that they should be involved in.

Even if @groud assigned himself with the intent of doing the implementation, that doesn't mean that it's locked. And yes, it can happen that core contributors have many issues assigned to them which they're not working on right now. If you're not sure about that, you can ask instead of making assumptions and demands.

Now, your insinuations regarding @groud's work or professionalism are unwelcome. If you have a personal grievance, feel free to bring it to me or other core devs on IRC instead of ranting on a public issue tracker. You're not to be judge of the work quality or perceived professionalism of any contributors.

@Xrayez
Copy link
Contributor

Xrayez commented Nov 23, 2020

Well I think we definitely have cultural differences here...

@Xrayez
Copy link
Contributor

Xrayez commented Nov 26, 2020

Ok this is finally implemented in godotengine/godot#43807, thanks for providing the original implementation @UgisBrekis (added as co-author)!

Also, sorry for looking demanding @groud, I'm just very perfectionistic and sometimes it's difficult for me to accept/understand the workflow of other people (most are lazy, but I'm sure you're not). 😃

We have hundreds of contributors and can't have everyone in the GitHub org to be assign-able

Also, sometimes I feel not accepted by the Godot community at times and feel lack of equality/justice (even if that might not be the case), because there's a clear difference between a random contributor and a Godot member/core developers when it comes to decision making process (as has been previously said, democracy doesn't work here, so I'm trying out the suggested do-ocracy now).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants