Skip to content

Node::transform field #17919

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

Closed
wants to merge 3 commits into 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.

@aevyrie
Copy link
Member

aevyrie commented Feb 24, 2025

This seems like a good use case for a new transform type that better matches the semantics of how it is being used. I don't want to suggest that should be done here, but it does seem more and more compelling. Even if we make UI rendering more like the rest of our 3d rendering stack, as long as the layout system produces a GlobalTransform, UI can use whatever transform type works best.

I hope you don't take my initial comment as a vote against this change. I happened to stumble upon it and wanted to understand what the motivation was, the first post doesn't really explain why we want this.

@ickshonpe
Copy link
Contributor Author

This seems like a good use case for a new transform type that better matches the semantics of how it is being used. I don't want to suggest that should be done here, but it does seem more and more compelling. Even if we make UI rendering more like the rest of our 3d rendering stack, as long as the layout system produces a GlobalTransform, UI can use whatever transform type works best.

I hope you don't take my initial comment as a vote against this change. I happened to stumble upon it and wanted to understand what the motivation was, the first post doesn't really explain why we want this.

Yeah I had some older PRs that added UI transform types, but it was hard to get any sort of concensus, so I was going for the lowest impact change that would fix the bugs here. I don't think it's a great approach though, closing.

@ickshonpe ickshonpe closed this Apr 23, 2025
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