Add render transform to Control nodes

[timoschwarzer]

This is an implementation of my proposal outlined in this discussion: https://github.com/godotengine/godot-proposals/issues/7053#issuecomment-1883816733

tl;dr: Currently it's hard and complicated to animate Control nodes without breaking the layout, especially if the Control nodes are in deeply nested layout structures which is often the case. This PR adds new properties to Control nodes that specify a render transform that does not affect and is not affected by layouting (similar to CSS' transform). These make UI animations much less pain to work with

• render_offset: translation
• render_scale: scale
• render_rotation, rotation
• render_transform_pivot, pivot for scaling and rotating

render_offset and render_transform_pivot are relative to the size of the Control node by default (I think it's a very common usecase to rotate around and scale from the center) but it can be toggled to use absolute pixel values instead.

Screencast from 2024-01-11 17-10-13.webm

Considerations

Input is currently not affected by the render transform. That means if you e.g. apply a render offset to a button, the button is still clickable at its original position but drawn somewhere else. This might need some discussion whether that's desirable or not.

[fire]

I remember Node2D has properties.

Vector2 | position | Vector2(0, 0) float | rotation | 0.0 Vector2 | scale | Vector2(1, 1)

How are these different?

Edit:

Will review the proposal.

[Mickeon]

One could argue that both render_rotation and render_scale are not that useful since (unless I'm severely mistaken) Controls already ignore both rotation and scale within containers and when moving anchors/margins.

[timoschwarzer]

@Mickeon this is currently considered a bug: https://github.com/godotengine/godot/issues/19068

That's why I added rotation and scale render transforms separately although they have the same result currently :)

[Mickeon]

I've gotten so used to the bug it may as well be a feature to me. 5 years ever since it has been reported, too. This "bug" does have its advantages in the same exact way as render_offset does in the PR. Hmm...

[timoschwarzer]

@Mickeon yeah, that's why I added render_rotation. So if the bug with rotation gets fixed eventually, people who used it for animation can switch to render_rotation and people who want rotated controls to actually use more space can finally do that. Same for scale and render_scale.

[djrain]

How this would affect children of the animated node, if at all? In our games we're usually animating whole control-based scenes or Containers, like a playing card or something, frequently containing both Controls and Node2D, so the whole thing needs to be animated as a unit. I currently achieve that by applying an animation transform to the node but only after any parent Container does its layout.

[timoschwarzer]

@djrain the current implementation sets the CanvasItem transform of the Control node, so it affects all children as well.

[timoschwarzer]

@AThousandShips thanks for the review :raised_hands: applied everything and fixed code style issues

[timoschwarzer]

@AThousandShips now it should have everything except the px suffix

[Mickeon]

Two things:

• You should add these lines (and ideally roughly document them) in Control.xml otherwise that Linux build will never compile
• When the PR is in "Draft" state it means it is not ready for review. I don't know if this is intentional.

[timoschwarzer]

@Mickeon thanks for noticing, I was aware of it! I had a small fun project using the changes of this PR going on until today hence I didn't write the docs yet and didn't remove the WIP status. I'll get this PR ready in the next days hopefully.

[timoschwarzer]

@Mickeon I added the docs and removed the WIP status so the PR should be ready for review now :slightly_smiling_face:

[Mickeon]

That's good, I... hmm... This is not a review but some thoughts:

• This PR could've used a separate proposal to further discuss potential solutions to the issue.
  • I probably should've recommended this much sooner, but everyone has their own things to worry about.
• The PR as is adds a LOT of properties to all Controls, which is most certainly hefty. It does introduce overhead, yet a grand majority of Control nodes in your standard application are not going to make use of them, or at least not all the time.
  • Because of it, in my opinion, it's worth being careful if this PR is even considered as is. Of course as you can see everyone is desiring something like this, but having it for every Control may be excessive. There may be multiple solutions to the same issue worth evaluating.

[timoschwarzer]

@Mickeon I can only speak for myself, but I've had more use for the newly added properties in contrast to existing position, rotation and scale properties. In addition to that, the point of this PR is to make all Controls easily animatable without messing with layouting. I don't see why only some Controls should have the render transform properties and some shouldn't (where would you draw the line here?).

I didn't measure it but I can't imagine the overhead being even noticable. The final transform is not calculated every frame but is cached iirc.

[conde2]

First of all, amazing pull request, indeed very useful.

I would only suggest to check if you're not setting the same value in the setters. To avoid a calling notify and a queue draw.

[markdibarry]

Also, rotation already has limited use with controls since clip_contents is not compatible.

[filkata]

Hoping that this is still going to get merged.

[Calinou]

@filkata Please don't bump issues without contributing significant new information. Use the :+1: reaction button on the first post instead.

4.3 is in feature freeze, so any new feature PRs can only be merged in 4.4 at the earliest.

[fire]

The general conversation from https://github.com/godotengine/godot-proposals/issues/7053 seems to be the maintainers are divided on adding this.

When there's a clear agreement pull requests get merged quickly, but with disagreement enhancements get stuck.

[Mickeon]

I do really appreciate that @timoschwarzer has been keeping this PR rebased and up-to-date as much as possible. I'm not part of the team of interest, but I am one of many Godot users.My worry is the same as it used to be from the start. That is, nodes in general are not exactly light in memory and the additional overhead may prove problematic, given that most Control nodes do not need this.

So the question to me always remains: is the trade-off worth it? Regardless, the feature is obviously very, very desired.

[timoschwarzer]

@Mickeon would it help to benchmark the actual impact this change would have, especially in UI heavy projects such as the Godot editor itself? I think I could do that.

[Mickeon]

I can't say benchmarking would do a lot for the people of interest, because there's still the question of whether Control nodes should be capable of this in the first place. But it may confirm or deny my own suspicious about the "unnecessary" overhead. I do repeat that there may be other ways to accomplish this that could be more satisfactory.

[timoschwarzer]

I will be busy for two weeks now but will try to get some meaningful data then.

I do repeat that there may be other ways to accomplish this that could be more satisfactory

I would of course like to see more sophisticated ways to accomplish animations for Control nodes but so far the other solutions I have seen came with limitations and aren't as flexible while I do see and have had use cases to animate both Control and Container nodes. When you say "more satisfactory", do you tie that directly to "not having render transform properties on all Control nodes"?

[timoschwarzer]

@Mickeon I finally did some benchmarks:

Benchmark

Sample 1: Master branch at 1fc82087658066935bed9e1350d62e334c7e0309 Sample 2: Same as Sample 1 plus this commit

For each sample, I launched the editor into an empty project, let everything load and then closed the editor again. Then I launch the same editor executable with a profiler, let it open and close it again. Each sample was profiled three times, the results per sample were roughly the same.

System:

Godot v4.4.dev (48bcad089) - Arch Linux #1 ZEN SMP PREEMPT_DYNAMIC Thu, 12 Sep 2024 17:17:51 +0000 on Wayland - X11 display driver, Multi-window, 2 monitors - Vulkan (Forward+) - dedicated Intel(R) Arc(tm) A770 Graphics (DG2) - Intel(R) Core(TM) i9-9900K CPU @ 3.60GHz (16 threads)

Compiled with

scons platform=linuxbsd target=editor production=yes linker=mold lto=none debug_symbols=yes

Profiled with:

valgrind --tool=massif ./godot.linuxbsd.editor.x86_64 -e --path /path/to/empty/godot/project

Results

Sample 1 (master branch): Peak Heap allocation: 574.13380 MiB - 574.17510 MiB

Sample 2 (this PR): Peak Heap allocation: 575.23056 MiB - 575.23404 MiB

Difference: 1.05894 MiB - 1.09676 MiB → This PR causes the editor to use approximately 0.18% more heap memory.

[KoBeWi]

I think this is the only way to reliably animate Control nodes inside Container with AnimationPlayer when their size is undetermined. Which is nice, but I think such strict requirement is not that common. There are multiple workarounds and if all fails, you can use Tweens.

That said, I think there is a way to have this feature without much overhead (currently, aside from memory, it also makes transforming more complex). The render_* stuff can be moved to a separate struct, which is null by default. Once any render property is set for the first time, allocate the struct. This limits the additional memory usage to single pointer for most Controls and allows to opt-out from the transform operations when the struct is null. Though not sure if doing this is a good idea 🤔

Other than that, since this is purely visual, should these properties be serialized in the scene? Especially with the input problem you mentioned? The new properties should have their purpose documented in Control's class description (i.e. that they are meant only for animation). If the usage is optional like I described above, and it's properly documented, I think it might be fine to have this feature.

EDIT: Though looking at the proposal again, I think this can be a new Container too. The only argument against that was that Control workflow involves lots of nesting, but it's fine in this case, because you'd use this container only for this specific case. Also in a scene with 10 levels of nesting, one extra node is irrelevant.

EDIT2: Then again, such container is quite easy to do in GDScript, as even noted in the proposal.

[timoschwarzer]

Warning: personal opinion and experiences. :slightly_smiling_face:

I think this is the only way to reliably animate Control nodes inside Container with AnimationPlayer when their size is undetermined. Which is nice, but I think such strict requirement is not that common.

I must completely disagree with your statement about this not being a common requirement. In every project so far that involves UI, a feature to apply a render transform to Control nodes of different kinds would have made things much easier to create. And it already helped a lot since I use a fork with this PR applied. Yes, I could have used Tweens for many of the things that I animated, but next to not being able to react to transform changes in parent nodes, Tweens just cannot hold up to the iteration speed and quality that AnimationPlayer gives. Being able to change animation easing curves and previewing them with one click - or even using Bezier curves - is a huge reduction in effort that I need to spend to get the same results in comparison to using Tweens. Looking at the reactions to this PR and the proposal, I don't seem to be alone with this opinion.

That said, I think there is a way to have this feature without much overhead (currently, aside from memory, it also makes transforming more complex).

0.18% more memory usage in the most UI heavy Godot project that I know of (the Editor) still does not sound like much overhead to me, but a solution like you pointed out sounds like a good compromise, if it's really a concern.

Also in a scene with 10 levels of nesting, one extra node is irrelevant.

I would agree immediately if Godot had some kind of slot mechanism that lets you define where children should be inserted for instanced scenes. Too often have I been at a point where I got screwed by this to be able to agree here right now. I have had dozens of such cases where this deep nesting of controls prevented modularization of logical Control "components" into separate scenes. I cannot tell immediately if this would be a problem if render transformation is achieved using Containers.

[KoBeWi]

I would agree immediately if Godot had some kind of slot mechanism that lets you define where children should be inserted for instanced scenes.

Soon: #84018

[torcado194]

Just to give my two cents: I think this feature is invaluable, to the point that I had already implemented this myself locally (though just for offset, not rotation). I've been using this PR for several weeks and have found no issues with it. I very much prefer having these properties on Control, regardless of the memory overhead. I think a dedicated node for render offset is a fine alternative, but I am also of the opinion that I would rather reduce the necessary nesting in the scene tree, at least for something as general as a visible offset.

My only feedback is about making the "relative to size" toggles default disabled, to keep them in-line with how transforms work everywhere else (that being in absolute pixels), and because positioning in absolute units is much more common in my experience. I agree that rotating around the center is the most common case for the rotation pivot, but at the very least I don't think that decision should effect the transform offset (and should probably also include similar changes to the basic pivot offset unit for consistency).

[Mickeon]

For the ones interested in this feature, see and give feedback to the following alternative as well:

• https://github.com/godotengine/godot/pull/98955

[dog-on-moon]

I'd like to keep pushing for this PR a bit, so for anyone reading/catching up, here's my thoughts to try and pull together the current state of things:

1. The TransformContainer PR is a fine alternative, but my concerns are that it is currently less effective for developing UI than this one (see https://github.com/godotengine/godot/pull/98955#issuecomment-2465908667)
2. I'm unsure how to run or setup appropriate benchmarks for Godot, so if there is anyone interested in testing specifically the editor startup time before and after this PR, that would help validate the performance impact of this change as well!

Another major thing standing in the way of this PR is adjusting how the properties are shown in the inspector to make them a bit cleaner. In my opinion, the nicest way to do this would be to format the options to have parity with the existing Transform category for Control:

(ninja edit: adding the "Lock/Unlock Component Ratio" button on the render_scale vector would be a good bonus, similar to Control's scale property)

And to the extent of supporting parity, I'm tempted to favor removing "offset_relative_to_size" and "transform_pivot_relative_to_size" as well -- primarily because "offset_relative_to_size" can already be false in most cases, and we already expect developers to manually set the Control's pivot offset to size / 2 in its property documentation for scaling about its center. It's a kind of complex option that I would prefer to be reimplemented via scripting or done manually in most cases.