Skip to content

Only use physical coords internally in bevy_ui#16375

Merged
alice-i-cecile merged 31 commits intobevyengine:mainfrom
ickshonpe:ui-physical-coords
Nov 22, 2024
Merged

Only use physical coords internally in bevy_ui#16375
alice-i-cecile merged 31 commits intobevyengine:mainfrom
ickshonpe:ui-physical-coords

Conversation

@ickshonpe
Copy link
Copy Markdown
Contributor

@ickshonpe ickshonpe commented Nov 13, 2024 •
edited
Loading

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.

@ickshonpe 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
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior X-Uncontroversial This work is generally agreed upon labels Nov 13, 2024
@BenjaminBrienen 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
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Nov 19, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Nov 19, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 19, 2024
atlv24
atlv24 approved these changes Nov 19, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@atlv24 atlv24 left a comment

Choose a reason for hiding this comment

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

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.

@alice-i-cecile alice-i-cecile modified the milestones: 0.15, 0.16 Nov 19, 2024
@alice-i-cecile alice-i-cecile added M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Nov 19, 2024
@ickshonpe ickshonpe requested a review from doup November 21, 2024 13:22
@ickshonpe
Copy link
Copy Markdown
Contributor Author

ickshonpe commented Nov 21, 2024 •
edited
Loading

@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
Copy link
Copy Markdown
Contributor

doup commented Nov 21, 2024

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`!

@ickshonpe 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
doup
doup approved these changes Nov 21, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@doup doup left a comment

Choose a reason for hiding this comment

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

Everything is fine now! 🎉

atlv24
atlv24 approved these changes Nov 21, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@atlv24 atlv24 left a comment

Choose a reason for hiding this comment

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

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

@atlv24 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
@alice-i-cecile alice-i-cecile modified the milestones: 0.16, 0.15 Nov 22, 2024
@alice-i-cecile 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
mockersf pushed a commit that referenced this pull request Nov 22, 2024
@ickshonpe @atlv24 @mockersf
Only use physical coords internally in bevy_ui (#16375)
78a80c5 
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 protected]>
ecoskey pushed a commit to ecoskey/bevy that referenced this pull request Dec 2, 2024
@ickshonpe @atlv24 @ecoskey
Only use physical coords internally in bevy_ui (bevyengine#16375)
c8067d9 
# 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 protected]>
@JeanMertz JeanMertz mentioned this pull request Dec 5, 2024
Closed
@janriemer janriemer mentioned this pull request Dec 29, 2024
Open
ecoskey pushed a commit to ecoskey/bevy that referenced this pull request Jan 6, 2025
@ickshonpe @atlv24 @ecoskey
Only use physical coords internally in bevy_ui (bevyengine#16375)
7678c67 
# 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 protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cart cart cart approved these changes

+3 more reviewers

@doup doup doup approved these changes

@atlv24 atlv24 atlv24 approved these changes

@BenjaminBrienen BenjaminBrienen BenjaminBrienen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior C-Code-Quality A section of code that is hard to understand or change M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Uncontroversial This work is generally agreed upon

Projects

None yet

Milestone

0.15

Development

Successfully merging this pull request may close these issues.

6 participants

@ickshonpe @doup @cart @atlv24 @BenjaminBrienen @alice-i-cecile