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 TransformContainer #98955

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Nov 8, 2024

Closes godotengine/godot-proposals#7053
Supersedes #87081

godot.windows.editor.dev.x86_64_MA8PK0TCS4.mp4
godot.windows.editor.dev.x86_64_3QZberG1sF.mp4

@KoBeWi KoBeWi added this to the 4.x milestone Nov 8, 2024
@KoBeWi KoBeWi requested review from a team as code owners November 8, 2024 11:53
@KoBeWi KoBeWi force-pushed the containformers branch 3 times, most recently from edc6dc9 to 7737930 Compare November 8, 2024 12:00
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Not much of a fan of prefixing all of these properties with "transform_", I'll say that much. I know why that's the case, as some of these properties would otherwise conflict with Control's own.

But if it has to be done, perhaps one could argue visual or render would be a more intuitive, and perhaps less redundant prefix.

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 8, 2024

But if it has to be done, perhaps one could argue visual or render would be a more intuitive, and perhaps less redundant prefix.

transform prefix is more consistent with the class' name.
TransformContainer with visual_offset etc. would be odd.
idk

@groud
Copy link
Member

groud commented Nov 8, 2024

Not much of a fan of prefixing all of these properties with "transform_", I'll say that much. I know why that's the case, as some of these properties would otherwise conflict with Control's own.

The prefix could be removed from the inspector by using a property group I'd say.

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 8, 2024

Then the container would have all properties in a group.

@michal-gora
Copy link

Would adding a scaling "pivot"/origin point make sense?

@arkology
Copy link
Contributor

arkology commented Nov 8, 2024

It doesn't affect input, right? So I should agree that calling this container just TransformContainter may be confusing for those who will expect it to affect everything.
(But for me this naming is ok because I know background. And calling it RenderTransformContainter may be definitely too much)

@mk56-spn
Copy link

mk56-spn commented Nov 8, 2024

Just to clarify the main difference between this and the other implementation ( #87081) is this one being a container itself instead of a drop down in pre existing control nodes?

if so id be curious as to why this is preferred, sure the other option adds a little more "bloat" to preexisting nodes. but it also seems a tad more convenient then having yet another control node for nesting control nodes in.

I assume the main advantage of this approach is allowing one to apply said transform to various nested controls more easily?

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 8, 2024

sure the other option adds a little more "bloat" to preexisting nodes

Yes, it adds bloat to all Control nodes. There is thousands of them in the editor. It's extra data per each node and the extra transform is processed regardless if it was used. The new container allows using transform only in the nodes that actually need it, and most often there won't be many of them.

@Mickeon
Copy link
Contributor

Mickeon commented Nov 8, 2024

The prefix could be removed from the inspector by using a property group I'd say.

Then the container would have all properties in a group.

Control itself is like this. It's not unprecedented.

@Kakiroi
Copy link

Kakiroi commented Nov 8, 2024

Maybe rotation pivot and scale pivot should be separate for finer control?

@passivestar
Copy link
Contributor

Nice to see relative pivot working here!

Some problems I found:

  1. Flickering when resizing
1.mp4
  1. Scale-Rotation order. It shears the control which is a cool effect but it would be nice if there was a way to avoid that. Perhaps a dropdown to select the order of operations? Would also be nice to be able to offset in a different order to move the thing in "orthographic projection"
2.mp4
  1. Can't see the effect in the running game no matter how hard I try with a nested ColorRect. Possibly related to 1?
3.mp4

3.1 Managed to see the effect using a PanelContainer instead of a ColorRect. However it snaps back to untransformed when the window is resized

4.mp4
  1. I understand this is a purely visual effect but would go hard if it supported input too. Unless documented this will probably be reported as a bug at some point
5.mp4
  1. Scale ratio lock is missing unlike in the regular Control transform, which is a very useful feature

P.S To be honest after testing this I don't really understand how this is different from the regular Control transform, and why this needed a new node instead of having the Control node's transform improved. But I didn't read through the whole conversation in the proposal. On the other hand maybe if you need to read the full conversation in a github proposal to understand the feature then maybe it won't be obvious to users why this exists and when to use it over the regular node transforms

@mk56-spn
Copy link

mk56-spn commented Nov 8, 2024

P.S To be honest after testing this I don't really understand how this is different from the regular Control transform, and why this needed a new node instead of having the Control node's transform improved. But I didn't read through the whole conversation in the proposal. On the other hand maybe if you need to read the full conversation in a github proposal to understand the feature then maybe it won't be obvious to users why this exists and when to use it over the regular node transforms

If it works how I understand it does then this transform doesn't mess with dynamic container resizing for example. So if you have a horizontal fill container with 4 colorects inside it ( as demonstrated above ) the sizing of the container remains the same whilst you animate those colour rects . This was possible before with enough container nesting but it was rather painful at times.

@ettiSurreal
Copy link
Contributor

Icon probably needs change. TransformContainer

I'm not exactly a designer but have my proposal:
TransformContainer (1)

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 8, 2024

Pushed some changes:

  • changed properties to use visual_ prefix
  • grouped them
  • new icon
    image

@torcado194
Copy link

torcado194 commented Nov 8, 2024

Great work, thanks for putting in the time for this!

I have similar thoughts on this as i do with the alternative PR
I still prefer reducing the necessary nesting of the scene tree as opposed to the number of properties in a node. Having to add an extra MarginContainer for offsetting is already an annoying part of the workflow for what I believe to be a very common aspect of UI: positioning items. timoschwarzer demonstrated that adding these properties to Control has negligible impact on memory overhead, and even KoBeWi proposed a change to optimize it further in that thread.
Regardless, I prefer either solution way above none at all, this difference is annoying but minor to me overall.

My stronger criticisms are similar to the ones I had in the other PR:
I think the "Relative to size" toggles should be default disabled, to keep them in-line with how transforms work everywhere else. A case could be made to have the Pivot Relative be default enabled, since rotating about the center is a common usecase. But I do not think the Offset Relative should be default enabled. The base transform and positioning properties are based on absolute units, I see no reason that it should be different here. This will just end up being another annoying part of the workflow, manually disabling this check every time I want to move a UI element a few pixels. (others seem to agree)

I agree with @passivestar that the skewing behavior is unintuitive and that it should be under an option, or simply removed. A user can reproduce the skewing effect by nesting TransformContainers if they desire. This adds yet another difference from the normal position/rotation behavior, which I believe to be more intuitive.
A common usecase for me is to animate button hover effects, the current implementation prevents me from e.g. shifting the button up and rotating it without making it skew undesirably.

As for passivestar's 4th point, I do not think this (or the alternative PR) should affect input. The idea is that this is a purely visual change detached from the UI processing, akin to CSS's transform property. Again using a button hover effect as an example, keeping the input area unchanged prevents the very common issue of a button jittering up and down as it goes in and out of hover when the cursor is on the edge. Having an alternative solution for this may be nice, but not as a part of this PR.

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 8, 2024

Pushed more changes. Additional property hints and some fixes for multiple children.

I ran into more problems though.

godot.windows.editor.dev.x86_64_fcK9MTKjgq.mp4

Like this label which is shrunk to the center. It has pivot of 0.5, 0.5, but it's centered around its own center (as if it was anchored to the top). Probably not expected behavior.

Flickering when resizing
Can't see the effect in the running game no matter how hard I try with a nested ColorRect. Possibly related to 1?

They are indeed the same issue. For some reason the Control will reset its transform after sorting. Another case when this happens is when you add a new Control to a transformed TransformContainer. The transform will not apply immediately. Not sure how to solve that yet.

I understand this is a purely visual effect but would go hard if it supported input too. Unless documented this will probably be reported as a bug at some point

Added a mention in the doc.

Perhaps a dropdown to select the order of operations? Would also be nice to be able to offset in a different order to move the thing in "orthographic projection"

I'm not familiar with transforms, if you can suggest list of available options and their effect I can add them.

I agree with passivestar that the skewing behavior is unintuitive and that it should be under an option, or simply removed.

The skewing behavior is not intended. It's some transform magic gone wrong.

@passivestar
Copy link
Contributor

I'm not familiar with transforms, if you can suggest list of available options and their effect I can add them.

if you see skewing as a bug and intend to fix it you can just ignore that comment because it's not that relevant without skewing

@dog-on-moon
Copy link
Contributor

dog-on-moon commented Nov 8, 2024

I concur with @torcado194 that the implementation of a separate node for offsetting render transforms is annoying from a developer perspective, except significantly moreso than MarginContainers.

Consider a common case with say, 4 buttons setup in a VBoxContainer. To implement any interesting animations on them (hover effects, slide-in on appear, etc) each of those buttons now requires a separate TransformContainer in-between, which conflicts with any script anticipating Button children in the VBoxContainer. Adding these containers and patching scripts to support them makes iteration very slow -- this situation is OK for MarginContainers (they wrap over the VBoxContainer instead), but Containers are a headache to attach between existing, dynamic elements.

Also, compare TransformContainer to the current approach of implementing UI animations in Godot: add a parent Control in-between the VBoxContainer and the Button to serve as layout, and then manipulate the transform of the Button instead. TransformContainer directly mimics this process and does not address most of its problems.

I agree that a separate container like this is indeed the most memory-efficient way to introduce UI animations. However, I worry that it stems from undervaluing how critical supporting convenient UI animations is for producing robust, responsive game UI. Since it has been shown that the overhead in the editor scene is negligible (0.18% more heap memory in the editor scene, according to the other PR), I find it preferable if it is implemented as properties (to me, the convenience greatly outweighs the seemingly minimal costs).

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 9, 2024

Also, compare TransformContainer to the current approach of implementing UI animations in Godot: add a parent Control in-between the VBoxContainer and the Button to serve as layout, and then manipulate the transform of the Button instead.

The main benefit is that TransformContainer has no problems with sizing that may come with using Controls. The transform is applied on top of existing layout, so if the container resizes, it does not break animations that may e.g. use absolute position values.

Since it has been shown that the overhead in the editor scene is negligible (0.18% more heap memory in the editor scene, according to the other PR)

That's only memory usage, but the other performance impact was not measured. Since all Controls have to process extra transform, it may impact startup time.


With that said, this PR is just another alternative for the same problem. Aside from #87081, there is also #95632. In the end we can merge only one of them and which one is still up to be decided.

@popcar2
Copy link

popcar2 commented Nov 13, 2024

I was messing around with this PR and found weird behavior: Attempting to tween anything in a panel's Stylebox causes it to ignore the visual offset. In this video, if I tween the shadow size of the panel, it doesn't move up with the TextureRect

2024-11-13.18-12-20.mp4

MRP: transform_container_test.zip


Anyways, I agree that having render transforms on Controls themselves would be more convenient as someone who uses UI a lot. If performance overhead is the main blocker for that, it would be great if someone can run more benchmarks to see if it actually is an issue. That said, I don't mind this PR going through rather than the other ones given enough reason.

Also, I think offset relative to size and pivot relative to size should be disabled by default. They're useful features, but almost everything including regular transforms work with pixels, so it could be confusing.

@fire
Copy link
Member

fire commented Nov 13, 2024

Clarification between the different layout property systems would be the leading blocker to my approval of adding render transforms on Controls.

Performance overhead is not the leading blocker.

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.

Implement a TransformContainer Node