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.

Sign up for GitHub

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

Jump to bottom

box shadow #15204

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

box shadow #15204

wants to merge 31 commits into from
+954 −0

Conversation

ickshonpe
Copy link
Contributor

@ickshonpe ickshonpe commented Sep 14, 2024 •
edited
Loading

Objective

UI box shadow support

Adds a new component BoxShadow:

pub struct BoxShadow {
    /// The shadow's color
    pub color: Color,
    /// Horizontal offset
    pub x_offset: Val,
    /// Vertical offset
    pub y_offset: Val,
    /// Horizontal difference in size from the occluding uninode
    pub x_spread: Val,
    /// Horizontal difference in size from the occluding uninode
    pub y_spread: Val,
    /// Blurriness of the shadow
    pub blur_radius: Val,
}

To use BoxShadow, add the component to any Bevy UI node and a shadow will be drawn beneath that node.
Also adds a resource BoxShadowSamples that can be used to adjust the shadow quality.

Notes

  • I'm not super happy with the field names. Maybe we need a struct Size { width: Val, height: Val } type or something.
  • The shader isn't very optimised but I don't see that it's too important for now as the number of shadows being rendered is not going to be massive most of the time. I think it's more important to get the API and geometry correct with this PR.
  • I didn't implement an inset property, it's not essential and can easily be added in a follow up.
  • Shadows are only rendered for uinodes, not for images or text.
  • The extracted shadow geometry still needs some tweaks, in particular border radius for shadows isn't quite right.
  • Still need to add batching in prepare_shadows.

Showcase

cargo run --example box_shadow -- --samples 4

br

cargo run --example box_shadow -- --samples 10

s10

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets C-Needs-Release-Note Work that should be called out in the blog due to impact X-Contentious There are nontrivial implications that should be thought through labels Sep 14, 2024
@JMS55
Copy link
Contributor

JMS55 commented Sep 15, 2024

If you need a reference, there's linebender/vello#665, although idk if it's suitable for games/is fast or not.

@alice-i-cecile alice-i-cecile added the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Sep 15, 2024
@github-actions GitHub Actions
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.

@ickshonpe ickshonpe marked this pull request as ready for review September 16, 2024 13:59
alice-i-cecile
alice-i-cecile reviewed Sep 16, 2024
View reviewed changes
crates/bevy_ui/src/render/box_shadow.wgsl
return exp(-(x * x) / (2. * sigma * sigma)) / (sqrt(2. * PI) * sigma);
}

fn erf(p: vec2<f32>) -> vec2<f32> {
Copy link
Member

Choose a reason for hiding this comment

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

Comment about what erf is, or a more descriptive name please.

alice-i-cecile
alice-i-cecile reviewed Sep 16, 2024
View reviewed changes
crates/bevy_ui/src/render/box_shadow.wgsl
fn erf(p: vec2<f32>) -> vec2<f32> {
let s = sign(p);
let a = abs(p);
var result = 1.0 + (0.278393 + (0.230389 + 0.078108 * (a * a)) * a) * a;
Copy link
Member

Choose a reason for hiding this comment

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

Comment or constants for these comments please.

alice-i-cecile
alice-i-cecile reviewed Sep 16, 2024
View reviewed changes
crates/bevy_ui/src/ui_node.rs
pub x_offset: Val,
pub y_offset: Val,
/// Difference in size from occluding uninode
pub x_spread: Val,
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I don't like this field name either. It's fine though and I can't think of anything better.

alice-i-cecile
alice-i-cecile reviewed Sep 16, 2024
View reviewed changes
crates/bevy_ui/src/ui_node.rs
derive(serde::Serialize, serde::Deserialize),
reflect(Serialize, Deserialize)
)]
pub struct BoxShadow {
Copy link
Member

Choose a reason for hiding this comment

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

Needs doc comments with a brief description and an explanation of how it's used. In particular, I want to know if this goes on my Node component, or as a child, and any limitations.

I'd also like to have a note about what Val's dimensions are computed relative to.

alice-i-cecile
alice-i-cecile reviewed Sep 16, 2024
View reviewed changes
crates/bevy_ui/src/render/ui_vertex_output.wgsl Outdated
// The size of the node in pixels. Order is width, height.
@location(2) @interpolate(flat) size: vec2<f32>,
@location(3) @interpolate(flat) size: vec2<f32>,
Copy link
Member

Choose a reason for hiding this comment

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

Can / should we avoid swapping this by moving the new border_radius binding to slot 3 rather than displacing size?

alice-i-cecile
alice-i-cecile reviewed Sep 16, 2024
View reviewed changes
crates/bevy_ui/src/render/box_shadow.rs
}

#[derive(Component)]
pub struct UiShadowsBatch {
Copy link
Member

Choose a reason for hiding this comment

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

Doc strings please

alice-i-cecile
alice-i-cecile reviewed Sep 16, 2024
View reviewed changes
crates/bevy_ui/src/render/box_shadow.rs
}

#[derive(Resource)]
pub struct BoxShadowMeta {
Copy link
Member

Choose a reason for hiding this comment

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

Doc strings for public types :)

alice-i-cecile
alice-i-cecile reviewed Sep 16, 2024
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Generally looking good: nice example and I'm happy with the scope of this. I have a few cleanup suggestions, and I'd like at least minimal docs for all public types and functions.

@alice-i-cecile alice-i-cecile added D-Shaders This code uses GPU shader languages D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes labels Sep 16, 2024
@nicoburns
Copy link
Contributor

Does this work for boxes with transparent backgrounds? (shadow should not render beneath content)

@TotalKrill
Copy link
Contributor

I read through it, and I agree with the comments by @alice-i-cecile. Other than that, I test-ran this in the browser and on linux and saw no issues.

Looking forward to this!

@TotalKrill TotalKrill self-requested a review September 16, 2024 19:43
tychedelia
tychedelia approved these changes Sep 20, 2024
View reviewed changes
Copy link
Contributor

@tychedelia tychedelia left a comment

Choose a reason for hiding this comment

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

Was confused about the changes to ui_vertex_output, but since those have been reverted, looks good to me!

tychedelia
tychedelia reviewed Sep 21, 2024
View reviewed changes
crates/bevy_ui/src/render/box_shadow.rs
/// Number of shadow samples.
/// A larger value will result in higher quality shadows.
#[derive(Resource, Copy, Clone, Debug, Reflect, ExtractResource)]
pub struct BoxShadowSamples(pub u32);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be camera driven.

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile left review comments

@tychedelia tychedelia tychedelia approved these changes

@TotalKrill TotalKrill TotalKrill approved these changes

Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets C-Needs-Release-Note Work that should be called out in the blog due to impact D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes D-Shaders This code uses GPU shader languages S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ickshonpe @JMS55 @alice-i-cecile @nicoburns @TotalKrill @tychedelia