Skip to content
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

Split UI Overflow by axis #8095

Merged
merged 17 commits into from
Apr 17, 2023

Conversation

ickshonpe
Copy link
Contributor

@ickshonpe ickshonpe commented Mar 15, 2023

Objective

Split the UI overflow enum so that overflow can be set for each axis separately.

Solution

Change Overflow from an enum to a struct with x and y OverflowAxis fields, where OverflowAxis is an enum with Clip and Visible variants. Modify update_clipping to calculate clipping for each axis separately. If only one axis is clipped, the other axis is given infinite bounds.

overflow


Changelog

  • Split the UI overflow implementation so overflow can be set for each axis separately.
  • Added the enum OverflowAxis with Clip and Visible variants.
  • Changed Overflow to a struct with x and y fields of type OverflowAxis.
  • Overflow has new methods visible() and hidden() that replace its previous Clip and Visible variants.
  • Added Overflow helper methods clip_x() and clip_y() that return a new Overflow value with the given axis clipped.
  • Modified update_clipping so it calculates clipping for each axis separately. If a node is only clipped on a single axis, the other axis is given -f32::INFINITY to f32::INFINITY clipping bounds.

Migration Guide

The Style property Overflow is now a struct with x and y fields, that allow for per-axis overflow control.

Use these helper functions to replace the variants of Overflow:

  • Replace Overflow::Visible with Overflow::visible()
  • Replace Overflow::Hidden with Overflow::clip()

* Split the UI overflow implementation so overflow can be set for each axis separately.
* Added new enum `OverflowAxis` with `Hidden` and `Visible` variants.
* Changed `Overflow` to a struct with `x` and `y` fields of type `OverflowAxis`.
* `Overflow` has new methods `visible()` and `hidden()` that replace its previous `Hidden` and `Visible` variants.
* Added `Overflow` helper methods `x_hidden()` and `y_hidden()` that return a new `Overflow` with the given axis hidden.
* Added register type for `OverflowAxis`.
* Changed the `ui` example to use `Overflow::y_hidden()` instead of `Overflow::Hidden`.
* Modified `update_clipping` so it calculates clipping for each axis separately. If a node is only clipped on one axis, the other axis is given -/+ `f32::INFINITY` clipping bounds.
* Added `overflow_debug` example demonstrating how to use the `overflow`.
@ickshonpe ickshonpe changed the title Split overflow by axes Split overflow by axis Mar 15, 2023
@ickshonpe ickshonpe changed the title Split overflow by axis Split Overflow by axis Mar 15, 2023
@nicoburns nicoburns added the A-UI Graphical user interfaces, styles, layouts, and widgets label Mar 15, 2023
@ickshonpe ickshonpe changed the title Split Overflow by axis Split Ui Overflow by axis Mar 15, 2023
@ickshonpe ickshonpe changed the title Split Ui Overflow by axis Split UI Overflow by axis Mar 15, 2023
* Added `is_visible` methods to `Overflow` and `OverflowAxis`
* Replaced the second `match` statement in `update_clipping` with an `if` statement.
Copy link
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.

I like this, I just pointed a couple of nits. 👍

// Update this node's CalculatedClip component
match (clip, calculated_clip) {
(None, None) => {}
match (maybe_inherited_clip, maybe_calculated_clip) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: what do you think about switching the position to (before, after) as in #8147? For me at least it's a tiny bit easier to understand:

  • (None, None) => noop
  • (Some, None) => remove
  • (None, Some) => add
  • (Some, Some) => update

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was going to switch them because it's a good idea, I like semantic ordering etc, but then I realised that it's probably better to use nested if let statements as you avoid that which option is which ambiguity completely.

crates/bevy_ui/src/update.rs Outdated Show resolved Hide resolved
@@ -111,7 +111,7 @@ fn setup(mut commands: Commands, asset_server: Res<AssetServer>) {
flex_direction: FlexDirection::Column,
align_self: AlignSelf::Stretch,
size: Size::height(Val::Percent(50.)),
overflow: Overflow::Hidden,
overflow: Overflow::y_hidden(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I would maybe just use Overflow::hidden() just to keep the same behavior as before. It's not really a problem unless children use an explicit width bigger than the container… which we know doesn't happen.

Ignore me (resolve this) as you wish.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking Overflow::y_hidden() is more expressive and intentional as it's a vertically scrolling list.
There is a lot to be said for only making minimal changes though and I get carried away sometimes 😅

@doup
Copy link
Contributor

doup commented Mar 23, 2023

I would like to add more cases to the overflow example. I'm thinking we either:

  • Rename this PR example to overflow (so it's more of an user facing example); then create a separate overflow_debug example which can be used to stress the clipping.
  • Expand this PR example to include more cases.

I prefer the first option since the example from this PR, as it is now, is very didactic.

I want to test few cases related to images/text and related UV problems (see #8167), e.g.:

  • Axis-aligned but overflowed. e.g., just clipping from the bottom (e.g. scrollable case), overflowed from a corner, a thin and wide container with a square children, etc.
  • Rotated/scaled children.

The axis-aligned/scaling cases should be solvable without major refactors (🤞). The rotation is going to be trickier.

What do you think?

@ickshonpe
Copy link
Contributor Author

I'm happy enough with the example as it is now that the rectangle is replaced with a clipped image. I think if anyone has any ideas for improvement it would be better to wait till this is merged (if it is) and then open another PR.

Not really sure about rotation and scaling and what we should support or how etc, there are still lots of bugs that need to be dealt with before we make big changes like that I think.

@doup doup mentioned this pull request Mar 24, 2023
cart added a commit that referenced this pull request Apr 5, 2023
# Objective

- Add a new example that helps debug different UI overflow scenarios
- This example tests the clipping behavior for images and text when the
node is moved, scaled or rotated.

## Solution

- Add a new `overflow_debug` example

# Preview

**Note:** Only top-left is working properly right now.


https://user-images.githubusercontent.com/188612/227629093-26c94c67-1781-437d-8410-e854b6f1adc1.mp4

---

Related #8095, #8167

---------

Co-authored-by: Carter Anderson <[email protected]>
Co-authored-by: ickshonpe <[email protected]>
@nicoburns
Copy link
Contributor

nicoburns commented Apr 10, 2023

I would like to suggest that the current Hidden variant of the OverflowAxis enum be renamed to Clip. This is a standard CSS value for overflow and seems to be much closer to what is actually implemented in bevy (clipping of paint without any implications for layout). https://kilianvalkhof.com/2022/css-html/do-you-know-about-overflow-clip/

Overflow::Hidden can be reintroduced as an additional option once it's supported in Taffy (support has already landed on the main branch).

@ickshonpe
Copy link
Contributor Author

ickshonpe commented Apr 11, 2023

I would like to suggest that the current Hidden variant of the OverflowAxis enum be renamed to Clip. This is a standard CSS value for overflow and seems to be much closer to what is actually implemented in bevy (clipping of paint without any implications for layout). https://kilianvalkhof.com/2022/css-html/do-you-know-about-overflow-clip/

Overflow::Hidden can be reintroduced as an additional option once it's supported in Taffy (support has already landed on the main branch).

Yes, Clip makes sense to me. The code and comments feel much more specific now.

* Renamed `OverflowAxis::Hidden` to `OverflowAxis::Clip`
* Replaced `hide`/`hidden` with `clip in the names of the functions belonging to `Overflow` and `OverflowAxis`.
* Fixed errors in the examples.
@github-actions
Copy link
Contributor

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

@alice-i-cecile alice-i-cecile added the C-Bug An unexpected or incorrect behavior label Apr 17, 2023
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Apr 17, 2023
@alice-i-cecile
Copy link
Member

@ickshonpe I'm happy with this PR; can you please rebase?

@ickshonpe
Copy link
Contributor Author

ickshonpe commented Apr 17, 2023

@ickshonpe I'm happy with this PR; can you please rebase?

should be done

should be done now

@github-actions
Copy link
Contributor

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

Copy link
Contributor

@nicoburns nicoburns left a comment

Choose a reason for hiding this comment

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

@ickshonpe I think you need to add a category to the example metadata to make CI happy.

@@ -1758,6 +1758,14 @@ description = "Illustrates how FontAtlases are populated (used to optimize text
category = "UI (User Interface)"
wasm = true

[[example]]
name = "overflow"
path = "examples/ui/overflow.rs"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
path = "examples/ui/overflow.rs"
path = "examples/ui/overflow.rs"
category = "UI (User Interface)"
wasm = true

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see you have already done this.

@github-actions
Copy link
Contributor

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 17, 2023
Merged via the queue into bevyengine:main with commit 09df19b Apr 17, 2023
@Selene-Amanita Selene-Amanita added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior M-Needs-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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants