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

Implement a TransformContainer Node #7053

Open
kolunmi opened this issue Jun 10, 2023 · 21 comments
Open

Implement a TransformContainer Node #7053

kolunmi opened this issue Jun 10, 2023 · 21 comments

Comments

@kolunmi
Copy link

kolunmi commented Jun 10, 2023

Describe the project you are working on

A project with a gui

Describe the problem or limitation you are having in your project

Hi, a little while ago I made a proposal which described the same issue at hand: #6863

Describe the feature / enhancement and how it helps to overcome the problem or limitation

In my previous issue, I proposed translating a Control node relative to its anchored position using a new property. I've realized this can actually be achieved more elegantly using a custom container which simply transforms its children according to some exported properties: offset, scale, and rotation.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

This is pretty easy to achieve in gdscript:

@tool
extends Container

class_name TransformContainer

@export var size_as_unit := false:
	set(n):
		size_as_unit = n
		queue_sort()

@export_category("Child Transform")
@export var transform_offset := Vector2.ZERO:
	set(n):
		transform_offset = n
		queue_sort()

@export var transform_pivot_offset := Vector2.ZERO:
	set(n):
		transform_pivot_offset = n
		queue_sort()

@export var transform_rotation := 0.0:
	set(n):
		transform_rotation = n
		queue_sort()

@export var transform_scale := Vector2.ONE:
	set(n):
		transform_scale = n
		queue_sort()

func _notification(what):
	if what == NOTIFICATION_SORT_CHILDREN:
		for c in get_children():
			if not c is Control:
				continue
			fit_child_in_rect(c, Rect2(Vector2(), size))
			c.position = transform_offset * c.size if size_as_unit else transform_offset
			c.pivot_offset = transform_pivot_offset * c.size if size_as_unit else transform_pivot_offset
			c.rotation = transform_rotation
			c.scale = transform_scale

func _get_minimum_size():
	var ms = Vector2.ZERO
	
	for c in get_children():
		if not c is Control or not c.visible or c.top_level:
			continue
		var minsize = c.get_combined_minimum_size()
		ms.x = max(ms.x, minsize.x)
		ms.y = max(ms.y, minsize.y)
	
	return ms

If this enhancement will not be used often, can it be worked around with a few lines of script?

Though, as shown above, a gdscript implementation is possible, there may be performance concerns when animating. Also that you must add @tool to the top of every inheriting class for the container to work as expected in the editor is a minor annoyance.

Is there a reason why this should be core and not an add-on in the asset library?

I believe this container is an absolutely crucial addition to the lineup and would personally use it everywhere. Thank you very much for reading!

@KoBeWi
Copy link
Member

KoBeWi commented Jun 10, 2023

Somewhat similar to #4994 I think.

@Kakiroi
Copy link

Kakiroi commented Nov 13, 2023

I've used it and it's very useful. The fact that it retains the minimum size makes it stable to use it on complicated scene. Though I wish this was a default behavior for all control nodes.

@kolunmi
Copy link
Author

kolunmi commented Nov 13, 2023

@Kakiroi Thank you! I wish it was the default behavior too. Hopefully we can get something like this in the future. I will say, I'm surprised there aren't more people requesting this.

@timoschwarzer
Copy link

timoschwarzer commented Jan 8, 2024

Unless I'm missing some obvious way to achieve UI animations, this proposal and the supplied GDScript implementation make UI animations so much more pleasant to work with while retaining all of Godot's layout system. I'd like to see some builtin way to apply a render transform to any Control node that doesn't affect layouting for UI animation purposes.

@Calinou
Copy link
Member

Calinou commented Jan 8, 2024

I definitely think this is useful, but I don't know whether this would be best implemented as a node or as a property (as proposed in #6863). UI scenes are already prone to large amounts of nesting, so adding even more containers feels like a poor solution to me.

@timoschwarzer
Copy link

I agree, I guess render_offset, render_rotation and render_scale properties directly on Control would be a good start. Controls already have scale which according to the docs seems to be there mainly for animation purposes, but it's in the Layout section in the inspector despite it seemingly not affecting any layouting (?).

To summarize the intent: There should be properties that define the transform before layouting, then layouting happens by Anchors/Containers and finally there should be properties that define a transform that is applied on top of the result from the layouting step.

@Calinou
Copy link
Member

Calinou commented Jan 9, 2024

Controls already have scale which according to the docs seems to be there mainly for animation purposes, but it's in the Layout section in the inspector despite it seemingly not affecting any layouting (?).

Rotation and scale currently don't affect layout, but this is considered a bug. Making the existing rotation and scale affect layout would likely break some things in people's projects though, as the current behavior may be relied upon.

@YuriSizov
Copy link
Contributor

YuriSizov commented Jan 9, 2024

UI scenes are already prone to large amounts of nesting, so adding even more containers feels like a poor solution to me.

I would say, adding another set of properties which on the surface are related to layouts and positioning would be an even worse idea. At least in a container they would be, well, contained. As properties they can become confusing to people who are not yet familiar with the system or haven't considered the need to animate controls.

In other words, it sounds nice when you are familiar with the problem, but if you are not, it will just look awkward that Godot has several seemingly conflicting properties for positioning and transforms.

Edit: That said, I'm not sold on the container approach either. I think there may be a more elegant way to provide animation capabilities.

@KoBeWi
Copy link
Member

KoBeWi commented Jan 9, 2024

Could #3123 be a solution?
Though this proposal only mentions anchors, so not sure why animating them is a problem.

@timoschwarzer
Copy link

timoschwarzer commented Jan 9, 2024

@YuriSizov I'd put that as the argument for exactly the opposite: Since this request is specifically not about layouting, but a render transform that is independent from the layout system, I think additional properties that are explicitly labeled as such ("render_offset" etc) would be a good solution.
I like to compare this problem with the transform property of CSS. You can layout things any way you want with any of the available containers and other layout options, but then add a transform property and animate that. The advantage here is that you have a property that guarantees you two things:

  1. if you reset it to an identity transform, your element is guaranteed to be in the place where the layout constraints put it and
  2. It doesn't have permanent side effects on the element itself or even other elements. I can put in any transform I want, but once it's set back to identity the element is back at its original position and it won't affect other Controls ever.

Point 2 is among others one of the reason why the issue @KoBeWi linked does not suffice in my opinion. The proposal only solves a symptom of the author's problem (prevent layouting so he can animate layout properties) while what they actually want is a transform after layouting happened. (At least that's what I get from the example videos. They have a VBox and want to highlight a single element by nudging it to the left)

You can animate anchors, but they have problems that become at least annoying to work with:

  • Anchors are relative to the Control rect, so there is no easy way of animating something e.g. 100px to the left without some calculations.
  • Setting Anchor values often has permanent side effects. For example you can move something to the right by increasing the left Anchor, but this will eventually increase the right Anchor as well. So when you set the left Anchor back to it's original value, your Control might be bigger than before.
  • Anchors are used for layouting (that's what they are for after all) and thus don't have a default value to reset to to get the result of the layouting step. So for each animation you would need to keep track of the original value.

@timoschwarzer
Copy link

I've implemented a proof of concept of my proposal:

Screencast.from.2024-01-11.17-10-13.webm

You can find the WIP pull request here: godotengine/godot#87081

If you have feedback for that PR specifically I'd appreciate comments there :)

@YuriSizov
Copy link
Contributor

Yeah, this is going to be confusing as hell to users when you use anchors and not containers. Especially because you basically use the same names, but also just conceptually.

Since this request is specifically not about layouting, but a render transform

That was kind of the main aspect of my point. This is not obvious to a user that there is some specific distinction. And in case of anchors there is no distinction at all. Anchors also don't have a problem with animations, only containers do. So our solution needs to be specific to containers, and not a general one.

@KoBeWi
Copy link
Member

KoBeWi commented Jan 11, 2024

Here's the same animation made in vanilla Godot:

godot.windows.editor.dev.x86_64_ADaVyu3Sh9.mp4

Animation.tscn.txt

sort_disabled would allow to get rid of the Control nodes in the middle.

@timoschwarzer
Copy link

timoschwarzer commented Jan 11, 2024

@KoBeWi assuming we would have sort_disabled active and you insert a new control between the others, you would need to redo all the animations for the following controls? Or if you are using a container with an unknown size (thinking of flow containers with auto wrap...)?

@YuriSizov thank you for the clarification. Although I don't agree with anchors not being affected by this problem because setting one side of an anchor can set the anchor of the other side as well and wont set it back. So currently you are often forced to animate both sides simultaneously.
I see that the naming could cause some confusion (although I don't agree it being confusing "as hell", because they are specifically separated in two different groups in the inspector and with some descriptions I think it could be made very clear what each of them does) and I'm happy for alternative suggestions.

@KoBeWi
Copy link
Member

KoBeWi commented Jan 12, 2024

you insert a new control between the others, you would need to redo all the animations for the following controls?

You are right, the solution with middle Controls has the advantage that you can move them around and the animations will still work correctly. The disadvantage is that you have more Controls in the scene tree.

@YuriSizov
Copy link
Contributor

YuriSizov commented Jan 12, 2024

Although I don't agree with anchors not being affected by this problem because setting one side of an anchor can set the anchor of the other side as well and wont set it back.

These are two different problems, though. Problem with animating in containers is that containers override children's transform. The only workaround is to inject something into container logic, like an extra control from KoBeWi's example. This problem doesn't exist with anchors. Difficulties with anchors don't need the same solution as difficulties with containers.

So currently you are often forced to animate both sides simultaneously.

That doesn't seem like a flaw to me, but if there is a use case to animate just one side and adjust the other automatically, this can be done with some locking mechanism, so anchors are always adjusted together.

@timoschwarzer
Copy link

@YuriSizov It might all be possible in the current state but the current workflow and developer experience doesn't feel very great to me at least. Another thing is that it's considered a bug that scale and rotation don't affect layouting currently. So once that's fixed, render scale and render rotation become relevant for layouts using anchors as well.

@KoBeWi with extra Control nodes in the middle, don't you also lose the ability to use containers in general? Because in our example if the size of the ColorRect changes, the size of the parent Control node doesn't change without manually setting it, right?

@KoBeWi
Copy link
Member

KoBeWi commented Jan 12, 2024

Because in our example if the size of the ColorRect changes, the size of the parent Control node doesn't change without manually setting it, right?

Yeah, but why would it need to? The ColorRects have full rect anchors and when Controls change size, the ColorRects will follow it. The opposite behavior is not useful. When you resize the ColorRect as part of animation, the Control does not need to change.
Though the problem that may arise is that you need to set minimum size of the Control, not the ColorRect. It requires a bit different workflow, but it's not unreasonable.

@YuriSizov
Copy link
Contributor

It might all be possible in the current state but the current workflow and developer experience doesn't feel very great to me at least.

Sure, but this doesn't invalidate or counter anything of what I've said before. Again, for the example that you've provided before with anchors needing to be in sync a better solution is to just add a parameter to Control which makes sure that the corresponding anchor is automatically adjusted with the one that you change. No need for a second set of transform properties to have that effect.

Another thing is that it's considered a bug that scale and rotation don't affect layouting currently. So once that's fixed, render scale and render rotation become relevant for layouts using anchors as well.

I don't think it's a bug. Scale and rotation in general don't affect UI layouts in containers (because, again, this is only relevant for containers). In fact, scale and rotation are mostly useful for animation. So I would say that the way forward with those properties in children of containers is to not reset them and use them for drawing without them affecting container logic.

@timoschwarzer
Copy link

@KoBeWi

When you resize the ColorRect as part of animation, the Control does not need to change.

Sorry for the confusion, I didn't mean resizing Controls as part of an animation but having Controls of variable size at runtime, for example Labels or Buttons with internationalized text. The text length differs between the languages, breaking all the animations because they use absolute position values, or did I miss something?

@YuriSizov

Again, for the example that you've provided before with anchors needing to be in sync a better solution is to just add a parameter to Control which makes sure that the corresponding anchor is automatically adjusted with the one that you change.

As far as I can see this wouldn't make the workflow less finicky. Would I need to set that parameter at the start of an animation every time and set it back at the end? Or disable it when I want to change anchors in the editor and then set it back for my animations to not break?

@KoBeWi
Copy link
Member

KoBeWi commented Jan 12, 2024

Controls of variable size at runtime, for example Labels or Buttons with internationalized text

Ok, this is where it would break. I guess the only workaround would be using Tweens, which are a different animation workflow.

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

No branches or pull requests

6 participants