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

Add border radius to UI nodes (adopted) #12500

Merged
merged 25 commits into from
Mar 19, 2024

Conversation

chompaa
Copy link
Member

@chompaa chompaa commented Mar 15, 2024

Objective

Implements border radius for UI nodes. Adopted from #8973, but excludes shadows.

Solution

  • Add a component BorderRadius which contains a radius value for each corner of the UI node.
  • Use a fragment shader to generate the rounded corners using a signed distance function.

Changelog

  • BorderRadius: New component that holds the border radius values.
  • NodeBundle & ButtonBundle: Added a border_radius: BorderRadius field.
  • extract_uinode_borders: Stripped down, most of the work is done in the shader now. Borders are no longer assembled from multiple rects, instead the shader uses a signed distance function to draw the border.
  • UiVertex: Added size, border and radius fields.
  • UiPipeline: Added three vertex attributes to the vertex buffer layout, to accept the UI node's size, border thickness and border radius.
  • Examples: Added rounded corners to the UI element in the button example, and a rounded_borders example.

@alice-i-cecile alice-i-cecile requested a review from viridia March 15, 2024 20:47
@alice-i-cecile alice-i-cecile added this to the 0.14 milestone Mar 15, 2024
@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 labels Mar 15, 2024
@viridia
Copy link
Contributor

viridia commented Mar 15, 2024

I think UiBorderRadius should be its own component. There has been a lot of confusion around the Style component, as to what exactly should and should not go in it, and part of the problem is that it's not well named. There's a proposal on the table to rename Style to LayoutConstraints: #10497 which should hopefully make it clearer - otherwise Style becomes a dumping ground for random properties that don't have an obvious home. All of the properties currently in Style are in fact layout constraints, but border radius is not a layout constraint.

@alice-i-cecile
Copy link
Member

From previous conversation with @viridia about the precursor:

I looked at the rounded rect PR some more today, and have some further thoughts:

  1. The changes to UiRect seem unnecessary, and are not required for rounded corners.
  2. Part of what the PR is doing is changing how borders are rendered in general. Traditionally, borders have been rendered as a separate primitive. The PR instead changes it so that a single shader renders both borders and background, passing in separate colors for each. Assuming you want to go down that path, you could split out a separate PR to do just that part. However, you can also expect some push-back due to the extra per-vertex overhead needed.
  3. I disagree with the author's decision to make border_radius part of Style, I think it should be a separate component as others have suggested.
  4. As someone who uses rounded corners, my opinions on this PR are somewhat mixed. In my own work, about half of the use cases for rounded corners also require clipping, which this PR doesn't address (and which is really difficult to do without a full 2d clipping solution such as Vello provides).
  5. I can get around the lack of clipping by writing my own custom UiMaterial for rounded corners, but if I'm going to do that, then I don't need rounded corner support in Bevy.

@alice-i-cecile alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Mar 15, 2024
@alice-i-cecile
Copy link
Member

I don't want to delay this unnecessarily, but here are my initial thoughts:

  1. I really like your generated test cases in the PR description: can you add a variant of this as an example so users and maintainers can quickly check how this works?
  2. I think that clipping out the background is required for this to be shippable: right now it looks fundamentally buggy.
  3. Adjusting hit detection would be nice, but can be moved to future work.
  4. Definitely agree on moving this out to its own component.

@viridia
Copy link
Contributor

viridia commented Mar 15, 2024

Assuming that you can get the rendering folks to sign off on the extra overhead involved in adding additional vertex params to UI node rendering, I would support this PR. It doesn't do everything I need, but a lot of people have wanted this for a long time, and as mentioned, I can always write custom shaders to fill the gaps.

What would be helpful, I think, is if we could post some actual perf data showing the impact. I'm not so familiar on how to do this, but I know that for my previous PR @cart posted some numbers.

@JMS55 JMS55 self-requested a review March 15, 2024 21:40
@chompaa
Copy link
Member Author

chompaa commented Mar 15, 2024

I don't want to delay this unnecessarily, but here are my initial thoughts:

  1. I've added an example rounded_borders analogous to borders, does this suffice?
  2. Should be fixed now.
  3. I'm not really sure how to approach this right now unfortunately.
  4. I've moved UiBorderRadius to a component. Should this just be BorderRadius to be in line with BorderColor now?

Copy link
Contributor

@pablo-lua pablo-lua left a comment

Choose a reason for hiding this comment

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

Just a reflection comment.

crates/bevy_ui/src/ui_node.rs Outdated Show resolved Hide resolved
@alice-i-cecile
Copy link
Member

Awesome, merging now: we can refine this more before 0.14 if issues come up <3

Thank you, I've wanted this feature for literally years.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 19, 2024
Merged via the queue into bevyengine:main with commit e7a31d0 Mar 19, 2024
30 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Mar 20, 2024
)

# Objective

- #12500 broke images and background colors in UI. Try examples
`overflow`, `ui_scaling` or `ui_texture_atlas`

## Solution

- Makes the component `BorderRadius` optional in the query, as it's not
always present. Use `[0.; 4]` as border radius in the extracted node
when none was found
github-merge-queue bot pushed a commit that referenced this pull request Mar 20, 2024
# Objective

- since #12500, text is a little bit more gray in UI

## Solution

- don't multiply color by alpha. I think this was done in the original
PR (#8973) for shadows which were not added in #12500
github-merge-queue bot pushed a commit that referenced this pull request Mar 21, 2024
# Objective

- #12500 use the primary window resolution to do all its calculation.
This means bad support for multiple windows or multiple ui camera

## Solution

- Use camera driven UI (#10559)
github-merge-queue bot pushed a commit that referenced this pull request Mar 21, 2024
# Objective

- A new example `rounded_borders` was introduced in #12500, similar to
the `borders` example, but containing labels to describe each border,
leaving inconsistency between the examples.

## Solution

- Update the `borders` example to be consistent with `rounded_borders`.

Co-authored-by: François Mockers <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Mar 21, 2024
# Objective

- #12500 added dead code to the ui shader

## Solution

- Remove it
github-merge-queue bot pushed a commit that referenced this pull request Apr 27, 2024
# Objective

- #12500 broke rotating ui nodes, see examples `pbr` (missing "metallic"
label) or `overflow_debug` (bottom right box is empty)

## Solution

- Pass the untransformed node size to the shader
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-Feature A new feature, making something new possible M-Needs-Release-Note Work that should be called out in the blog due to impact 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