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

Node::transform field #17919

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ickshonpe
Copy link
Contributor

@ickshonpe ickshonpe commented Feb 17, 2025

Objective

Add UI node transform support with minimal changes.

Solution

  • Add a transform field to Node.
  • In layout updates apply the layout position translation to the transform field and then set_if_neq it to the node entity's Transform component.

Then leave propagate_transforms to update the GlobalTransforms.

Testing

Reviewers can look at the overflow_debug example, which is the only current UI example that modifies the transform.

In layout updates apply the layout position translation to the transform field and then `set_if_neq` it to the node entitie's `Transform` component.
@ickshonpe ickshonpe added A-UI Graphical user interfaces, styles, layouts, and widgets A-Transform Translations, rotations and scales D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward C-Bug An unexpected or incorrect behavior labels Feb 17, 2025
@aevyrie
Copy link
Member

aevyrie commented Feb 18, 2025

Isn't this a bit anti-composition? The Node struct is absolutely ginormous. What's the motivation for not using Transform?

@ickshonpe
Copy link
Contributor Author

Isn't this a bit anti-composition? The Node struct is absolutely ginormous.

Yep, this isn't good at all, it's just the minimum I thought might possibly get merged as an intermediate step towards some of the problems with the UI transforms. #8240 is my preferred approach but it's going to take forever to update for main and it's been really difficult to reach concensus on changes like this.

What's the motivation for not using Transform?

ui_layout_system sets UI node's Transform's translation with the position offset of the node, which overwrites any value users set to the translation field themselves. You can write a system that adjusts the translations post layout but before transform propagation but that's kind of horrible and a lot of attempts I've seen fail with a panic.

We could make Transform work by backing up the values of Transform before ui_layout_system runs and then restoring the values after the transforms are propagated but that seems really ugly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Transform Translations, rotations and scales A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants