Only use physical coords internally in `bevy_ui` #16375

ickshonpe · 2024-11-13T13:43:38Z

Objective

We switch back and forwards between logical and physical coordinates all over the place. Systems have to query for cameras and the UiScale when they shouldn't need to. It's confusing and fragile and new scale factor bugs get found constantly.

Solution

Use physical coordinates whereever possible in bevy_ui.
Store physical coords in ComputedNode and tear out all the unneeded scale factor calculations and queries.
Add an inverse_scale_factor field to ComputedNode and set nodes changed when their scale factor changes.

Migration Guide

ComputedNode's fields and methods now use physical coordinates.
ComputedNode has a new field inverse_scale_factor. Multiplying the physical coordinates by the inverse_scale_factor will give the logical values.

atlv24

This needs a migration guide. Its a particularly sneaky type of breaking change, because it causes almost no compile errors, but underlying semantics change. @BenjaminBrienen @doup have you tested these changes/run any examples? I haven't yet.

crates/bevy_ui/src/render/mod.rs

ickshonpe · 2024-11-21T13:23:42Z

@atlv24 I ran the UI testbed and a couple more examples. But, now I checked some other random example and there are indeed issues. 😬

So… I've run all the UI examples and this is what I've found:

border width are different (example: borders, ui_texture_atlas)

texture atlas slice border width (example: ui_texture_atlas_slice, ui_texture_slice_flip_and_tile, ui_texture_slice)

border radius are different (example: box_shadow)

text wrapping (examples: grid sidebar text, scroll, text_debug, text_wrap_debug)

wrong text/box (?) alignment, could be because of the wrapping issue (examples: display_and_visibility, text, size_constraints)

overflow clip margin (example: overflow_clip_margin)

a panic! (example: overflow_debug)

All of these should be fixed now I think.

doup · 2024-11-21T14:39:27Z

I've checked the examples that had issues, I still see some:

outlines width and offset (see: borders/ui_texture_atlas/display_and_visibility)… it wasn't the border as I though, it's the outline.
border radius and box shadow size (see: box_shadow). Note the first row right-most elements shadow is off.
overflow_debug still panics

thread 'Compute Task Pool (1)' panicked at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/num/f32.rs:1433:9:
min > max, or either was NaN. min = 0.0, max = NaN
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Encountered a panic in system `bevy_ui::layout::ui_layout_system`!
Encountered a panic in system `bevy_app::main_schedule::Main::run_main`!

doup

Everything is fine now! 🎉

atlv24

ran the ui examples, all looks good, and migration guide is good!

We switch back and forwards between logical and physical coordinates all over the place. Systems have to query for cameras and the UiScale when they shouldn't need to. It's confusing and fragile and new scale factor bugs get found constantly. * Use physical coordinates whereever possible in `bevy_ui`. * Store physical coords in `ComputedNode` and tear out all the unneeded scale factor calculations and queries. * Add an `inverse_scale_factor` field to `ComputedNode` and set nodes changed when their scale factor changes. `ComputedNode`'s fields and methods now use physical coordinates. `ComputedNode` has a new field `inverse_scale_factor`. Multiplying the physical coordinates by the `inverse_scale_factor` will give the logical values. --------- Co-authored-by: atlv <email@atlasdostal.com>

# Objective We switch back and forwards between logical and physical coordinates all over the place. Systems have to query for cameras and the UiScale when they shouldn't need to. It's confusing and fragile and new scale factor bugs get found constantly. ## Solution * Use physical coordinates whereever possible in `bevy_ui`. * Store physical coords in `ComputedNode` and tear out all the unneeded scale factor calculations and queries. * Add an `inverse_scale_factor` field to `ComputedNode` and set nodes changed when their scale factor changes. ## Migration Guide `ComputedNode`'s fields and methods now use physical coordinates. `ComputedNode` has a new field `inverse_scale_factor`. Multiplying the physical coordinates by the `inverse_scale_factor` will give the logical values. --------- Co-authored-by: atlv <email@atlasdostal.com>

ickshonpe added 6 commits November 13, 2024 12:37

Store physical coords in Node and use them in rendering.

06e9915

clean up

4b711b7

Use physical coords in focus

9cfb164

use physical viewport in picking_backend

c6f831b

convert pointer position to physical coords in picking_backend

a3d2e2c

Removed some comments, updated some others.

f7ad8d1

ickshonpe added A-UI Graphical user interfaces, styles, layouts, and widgets C-Code-Quality A section of code that is hard to understand or change S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Nov 13, 2024

ickshonpe added 2 commits November 13, 2024 13:59

fixed ui_rounding_test

456ef4d

Remove unneeded map and associated comment.

c922960

alice-i-cecile added C-Bug An unexpected or incorrect behavior X-Uncontroversial This work is generally agreed upon labels Nov 13, 2024

doup approved these changes Nov 16, 2024

View reviewed changes

BenjaminBrienen approved these changes Nov 16, 2024

View reviewed changes

BenjaminBrienen added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Nov 16, 2024

ickshonpe added 4 commits November 18, 2024 11:32

Merge branch 'main' into ui-physical-coords

5112975

removed scaling vertex attribute from ui.wgsl

1d088aa

Removed inverse_scale_factor fields from UiVertex and TransparentUI

5617e40

remove unused aa type

6ad21ca

alice-i-cecile added this to the 0.15 milestone Nov 19, 2024

alice-i-cecile added this pull request to the merge queue Nov 19, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 19, 2024

atlv24 approved these changes Nov 19, 2024

View reviewed changes

crates/bevy_ui/src/render/mod.rs Outdated Show resolved Hide resolved

crates/bevy_ui/src/render/mod.rs Outdated Show resolved Hide resolved

alice-i-cecile modified the milestones: 0.15, 0.16 Nov 19, 2024

ickshonpe added 2 commits November 21, 2024 12:47

Merge branch 'ui-physical-coords-node-sf' into ui-physical-coords

02419d6

apply scale factor to overflow clip margin

7047e34

ickshonpe requested a review from doup November 21, 2024 13:22

ickshonpe added 7 commits November 21, 2024 14:57

Fixed outlines resolver

7722ce8

cleanup outline calculations

56f3313

Fixed inner corner radius clamping

e701a78

Fixed imports

a0fe4f8

fix shadow scalings

d9afb58

Fixed overflow_debug example

0ea34ae

Merge branch 'main' into ui-physical-coords

81c189b

ickshonpe added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Nov 21, 2024

Removed unneeded import

68134e4

doup approved these changes Nov 21, 2024

View reviewed changes

atlv24 approved these changes Nov 21, 2024

View reviewed changes

atlv24 added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Nov 21, 2024

cart approved these changes Nov 22, 2024

View reviewed changes

alice-i-cecile modified the milestones: 0.16, 0.15 Nov 22, 2024

alice-i-cecile added this pull request to the merge queue Nov 22, 2024

Merged via the queue into bevyengine:main with commit 8a3a8b5 Nov 22, 2024
30 checks passed

JeanMertz mentioned this pull request Dec 5, 2024

DebugUiPlugin doesn't respect scale factor properly #16666

Closed

janriemer mentioned this pull request Dec 29, 2024

0.15 Migration Guide: Provide examples for creating Rect with logical values (missing logical_rect method on Node - also see PR 15163 and PR 16375) bevyengine/bevy-website#1920

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only use physical coords internally in `bevy_ui` #16375

Only use physical coords internally in `bevy_ui` #16375

ickshonpe commented Nov 13, 2024 •

edited

Loading

atlv24 left a comment

ickshonpe commented Nov 21, 2024 •

edited

Loading

doup commented Nov 21, 2024

doup left a comment

atlv24 left a comment

Only use physical coords internally in bevy_ui #16375

Only use physical coords internally in bevy_ui #16375

Conversation

ickshonpe commented Nov 13, 2024 • edited Loading

Objective

Solution

Migration Guide

atlv24 left a comment

Choose a reason for hiding this comment

ickshonpe commented Nov 21, 2024 • edited Loading

doup commented Nov 21, 2024

doup left a comment

Choose a reason for hiding this comment

atlv24 left a comment

Choose a reason for hiding this comment

Only use physical coords internally in `bevy_ui` #16375

Only use physical coords internally in `bevy_ui` #16375

ickshonpe commented Nov 13, 2024 •

edited

Loading

ickshonpe commented Nov 21, 2024 •

edited

Loading