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 render transform to Control nodes #87081

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

Conversation

timoschwarzer
Copy link
Contributor

@timoschwarzer timoschwarzer commented Jan 11, 2024

This is an implementation of my proposal outlined in this discussion: godotengine/godot-proposals#7053 (comment)

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.

scene/gui/control.h Outdated Show resolved Hide resolved
scene/gui/control.cpp Outdated Show resolved Hide resolved
scene/gui/control.cpp Outdated Show resolved Hide resolved
scene/gui/control.cpp Outdated Show resolved Hide resolved
scene/gui/control.cpp Outdated Show resolved Hide resolved
scene/gui/control.cpp Outdated Show resolved Hide resolved
scene/gui/control.cpp Outdated Show resolved Hide resolved
scene/gui/control.cpp Show resolved Hide resolved
scene/gui/control.cpp Show resolved Hide resolved
@fire
Copy link
Member

fire commented Jan 11, 2024

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
Copy link
Contributor

Mickeon commented Jan 11, 2024

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
Copy link
Contributor Author

@Mickeon this is currently considered a bug: #19068

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

@Mickeon
Copy link
Contributor

Mickeon commented Jan 12, 2024

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
Copy link
Contributor Author

@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
Copy link

djrain commented Jan 12, 2024

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
Copy link
Contributor Author

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

@timoschwarzer timoschwarzer force-pushed the feature/control-render-transform branch from c5123f0 to ff160eb Compare January 12, 2024 19:43
@timoschwarzer
Copy link
Contributor Author

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

scene/gui/control.cpp Show resolved Hide resolved
scene/gui/control.cpp Outdated Show resolved Hide resolved
scene/gui/control.cpp Outdated Show resolved Hide resolved
scene/gui/control.cpp Show resolved Hide resolved
scene/gui/control.cpp Outdated Show resolved Hide resolved
scene/gui/control.cpp Outdated Show resolved Hide resolved
@timoschwarzer timoschwarzer force-pushed the feature/control-render-transform branch from ff160eb to f7ce79c Compare January 12, 2024 20:23
@timoschwarzer
Copy link
Contributor Author

@AThousandShips now it should have everything except the px suffix

@timoschwarzer timoschwarzer force-pushed the feature/control-render-transform branch 2 times, most recently from 7b26c72 to 5eed27e Compare January 12, 2024 21:13
@timoschwarzer timoschwarzer force-pushed the feature/control-render-transform branch from 5eed27e to 891f968 Compare January 21, 2024 11:51
@timoschwarzer timoschwarzer force-pushed the feature/control-render-transform branch 2 times, most recently from d068334 to d8bdea9 Compare February 2, 2024 22:03
@timoschwarzer timoschwarzer force-pushed the feature/control-render-transform branch from d8bdea9 to 7e3e661 Compare February 6, 2024 18:38
@timoschwarzer timoschwarzer force-pushed the feature/control-render-transform branch 2 times, most recently from dcadbb1 to 1383ae0 Compare February 17, 2024 17:40
@timoschwarzer timoschwarzer force-pushed the feature/control-render-transform branch 2 times, most recently from 950d99d to e7a7125 Compare February 23, 2024 13:40
@Mickeon
Copy link
Contributor

Mickeon commented Feb 23, 2024

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
Copy link
Contributor Author

timoschwarzer commented Feb 25, 2024

@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 timoschwarzer force-pushed the feature/control-render-transform branch from e7a7125 to 670b55d Compare March 4, 2024 13:31
@timoschwarzer timoschwarzer changed the title WIP: Add render transform to Control nodes Add render transform to Control nodes Mar 4, 2024
@timoschwarzer timoschwarzer marked this pull request as ready for review March 4, 2024 13:32
@timoschwarzer timoschwarzer requested a review from a team as a code owner March 4, 2024 13:32
@timoschwarzer
Copy link
Contributor Author

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

@Mickeon
Copy link
Contributor

Mickeon commented Mar 4, 2024

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.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected. There's the question of whether we should shift input coordinates as well, but this isn't always desired if you're using this for animation purposes. For instance, when hovering an UI element and shifting it upwards, if you held your mouse cursor on the bottom edge of the element, you could end up in a situation where the UI element is being hovered and unhovered continuously in a loop. Not shifting input coordinates prevents this kind of situation, at the cost of less a precise idea of where you can actually click the button (particularly for rotated buttons).

godotengine/godot-proposals#4338 would be a particularly good pairing with this PR, so that you can easily extend the clickable area of your buttons to cater for the space they can take during an animation.

Testing project: test_pr_87081.zip

cards.mp4

@timoschwarzer
Copy link
Contributor Author

@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
Copy link

conde2 commented Mar 12, 2024

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
Copy link
Contributor

markdibarry commented Mar 12, 2024

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

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.

None yet

8 participants