Move UI `Transform` to a field on `Node`

[ickshonpe]

Objective

Animating UI elements by modifying the Transform component of UI nodes doesn't work very well because ui_layout_system overwrites the translations each frame. The overflow_debug example uses a horrible hack where it copies the transform into the position that'll likely cause a panic if any users naively copy it.

Solution

There have been a couple of other attempts to fix this with UiTransform components etc but they were quite complicated and controversial and got bogged down in review. So I thought about it again and came up with this simpler alternative:

• Opt-out of transform propagation by requiring GlobalTransform instead of Transform on Node.
• Add a transform: Transform field to Node.
• Update UI node global transforms during layout updates.

[alice-i-cecile]

I like this as an incremental move forward. We shouldn't be storing a full Transform here, but that's easy to migrate from this PR.

Should this be on ComputedNode instead though? I don't think this is intended to be user-modifiable.

[github-actions[bot]]

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy? It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

[NthTensor]

This does seem like the least-bad option. But I thought there was code in bevy_transform to specifically filter out ui elements from propagation. Could you check if that's still there and remove it if it is?

I believe this should fix the issues big_space had with the use of Transform by ui elements, which is nice.

[ickshonpe]

I like this as an incremental move forward. We shouldn't be storing a full Transform here, but that's easy to migrate from this PR.

Should this be on ComputedNode instead though? I don't think this is intended to be user-modifiable.

The new transform field on Node is intended to be user-modifiable. It's meant to be simar to the CSS transform property and allows users to add simple UI animations or whatever without weird hacks to get around the conflicts between the transformation propagation and UI layout update systems.

I made another branch first just before this one that removed both Transform and GlobalTransform from UI nodes and instead added the transform to ComputedNode but it felt like it would be difficult to get through review as it required lots of changes to extraction and focus and new a UI specific visibility system. Performance is much better than main with that approach though and would allow for some nice helper methods on Node like contains_point etc which would simplify a lot of the systems.

[ickshonpe]

This does seem like the least-bad option. But I thought there was code in bevy_transform to specifically filter out ui elements from propagation. Could you check if that's still there and remove it if it is?

I believe this should fix the issues big_space had with the use of Transform by ui elements, which is nice.

I haven't seen anything like this. Afaik the only way to opt-out of transform propagation is by removing Transform and there's no UI specific filter.

[alice-i-cecile]

@ickshonpe I'm really keen on this as a step towards the great transform splittification :) Is this ready for review?

[ickshonpe]

@ickshonpe I'm really keen on this as a step towards the great transform splittification :) Is this ready for review?

Probably, I'll take another look at this first thing tomorrow. I've been thinking though do we even need a transform component for UI? We could just stick the transform values in ComputedNode along with everything else. ComputedNode is huge already but a lot of UI focused devs are allergic to linear algebra and we could add all the contains_point and relative_position helper methods that people have been requesting forever.

Another alternative is a TransformedNode(&ComputedNode, &UiTransform) wrapper that we implement all the helper functions on.

[ickshonpe]

Looks ok. I'm not sold on the transform as field in ComputedNode but I'm not sold either on any alternative.

Yeah I don't know either. I did it this way because it was easy to implement but it's relatively trivial to switch it to use a separate UiTransform/UiGlobalTransform.

Quick Q: How one would create a 3D UI with this change? Is there a 3D transform at the root of the node hierarchy? Or would the user have to render to an off screen render target the 2D UI then lay it out in 3D in the scene as a texture?

You couldn't really create a 3d UI before this change it was all 2d coords, it just used the 3d transform. Either way you have to render to texture for world UI.

[github-actions[bot]]

It looks like your PR has been selected for a highlight in the next release blog post, but you didn't provide a release note.

Please review the instructions for writing release notes, then expand or revise the content in the release notes directory to showcase your changes.

[ickshonpe]

@alice-i-cecile I think this is more or less good enough now. Abandoned the weird ideas I had, just got a UiTransform component that replaces Transform. I wanted to keep this a simple PR but ended up making a lot of other changes, figured it's better to fix everything here than migrate all the bugs to the new setup.

[djeedai]

Should this PR move to Draft? Was said to be nearly done, then got 12 commits merged. Also designed completely changed from initial version. I'm not sure as a reviewer when it's safe to spend time reviewing again. Thanks

[ickshonpe]

Should this PR move to Draft? Was said to be nearly done, then got 12 commits merged. Also designed completely changed from initial version. I'm not sure as a reviewer when it's safe to spend time reviewing again. Thanks

Yeah my fault keep changing my mind as to how this should be implemented. I don't intend to make any more changes now though.

[NthTensor]

Starting a review. From just the readme, this seems fantastic. Exactly what I was looking for in basically every way.

[ickshonpe]

Starting a review. From just the readme, this seems fantastic. Exactly what I was looking for in basically every way.

thanks that's great, I was just about to go and beg on discord for someone to review this

[viridia]

The cursor position is transformed to local node space during picking so that it respects rotations and scalings.

I'm not sure this solves the problem I have been having with pointer interactions for multi-part widgets. Consider a slider with a draggable thumb: you click on the thumb entity, so the picking event you get has the coordinates relative to the thumb. But what you need for the drag calculation is the coordinates relative to the slider as a whole.

What I'd like is for the observer function to have the option to transform the picking coordinates into local coordinates for any arbitrary ancestor of the picked element. The easiest way to do this, I think, is to have the event contain global coordinates, and then have the observer do a conversion to local coordinates using the global transform component on the ancestor.