Skip to content

Image Node Rotation #19340

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

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

Image Node Rotation #19340

wants to merge 14 commits into from

Conversation

hukasu
Copy link
Contributor

@hukasu hukasu commented May 23, 2025

Objective

Adopt #6688
Allow rotation of ImageNodes

Solution

Replace ImageNode::flip_x for rotation, ImageNode::flip_y remains intact

Testing

testbed_full_ui, ui_texture_slice_flip_and_tile and new image_node_rotation examples

Showcase

image

image

image

image

Deliberate Rendering change

There have been changed to the testbed_full_iu example

TODO

  • Include ui slices on the new example
  • Migration Guide

Copy link
Contributor

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

.map(|pos| (*transform * (pos * rect_size).extend(1.)).xyz());
let positions = QUAD_VERTEX_POSITIONS.map(|pos| {
(*transform
* Mat4::from_rotation_z(*rotation)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should the rotation be flipped in flip_y is true?

@hukasu hukasu marked this pull request as ready for review May 23, 2025 01:32
@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-UI Graphical user interfaces, styles, layouts, and widgets 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 M-Deliberate-Rendering-Change An intentional change to how tests and examples are rendered A-Rendering Drawing game state to the screen labels May 23, 2025
@alice-i-cecile alice-i-cecile added this to the 0.17 milestone May 23, 2025
Copy link
Contributor

@ickshonpe ickshonpe left a comment

Choose a reason for hiding this comment

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

This misses what I was trying to do with #6688 and #9341 (to be fair part of the reason I gave up on those mostly because a lot of the reviewers misunderstood it too). We can already rotate UI content using Transform::from_rotation(Quat::from_rotation_z(angle)).
The problem though is that after rotation the content no longer fits inside the node's bounds. Those two PR's also recomputed the node's content size so the content would still fit inside the layout.

The main objections to those PRs were that:

  • They didn't support arbitrary rotations, only right angles.
  • The API was confusing using unfamiliar terms from group theory.
    I don't disagree with them (the other reason I didn't update those PRs), but the UI renderer currently only supports clipping axis-aligned elements atm and you get confusing visual artifacts with clipping and rotation otherwise and I felt that support for axis-aligned transformations was better than nothing.

Maybe it's okay to support arbitrary rotations though and fix clipping later.
To be useful this needs to update the content sizes and image extraction would need to use the size of the image and not the size of the node.

@hukasu
Copy link
Contributor Author

hukasu commented May 23, 2025

i did this test to see the behavior of overflow: Overflow::clip() for both using rotation and transform

image

neither got clipped

@ickshonpe
Copy link
Contributor

ickshonpe commented May 23, 2025

i did this test to see the behavior of overflow: Overflow::clip() for both using rotation and transform
neither got clipped

Probably one of two things:

  • Overflow::clip() on a parent node tells the UI to clip that node's children if they lay outside the parent's bounds. It shouldn't do anything if you set it on a leaf ImageNode.
  • The clipping logic ignores rotations. If the node would be entirely inside the unclipped region before rotation it won't get clipped, even if the rotation moves it outside the clipping rect.

@hukasu
Copy link
Contributor Author

hukasu commented May 23, 2025

IIRC I did on the leaf...
One of your PRs were to take rotation into account right?

@ickshonpe
Copy link
Contributor

ickshonpe commented May 23, 2025

IIRC I did on the leaf... One of your PRs were to take rotation into account right?

IIRC both of those PRs rotated the image by swapping around the UV coords which avoids any problems with clipping but only supports 90 degree rotations.

I did implement clipping of rotated elements for a bevy 0.13 fork for work but the implementation was a bit limited and slow so I didn't upstream it. I've got an improved very WIP version that might be ready for 0.17.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets C-Feature A new feature, making something new possible D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Deliberate-Rendering-Change An intentional change to how tests and examples are rendered S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants