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

Skip the UV calculations for untextured UI nodes #7809

Merged
merged 3 commits into from Mar 17, 2023

Conversation

ickshonpe
Copy link
Contributor

@ickshonpe ickshonpe commented Feb 24, 2023

Objective

The UV calculations in prepare_uinodes can be skipped for nodes without images.

Solution

Skip the UV calculations if the image handle id is equal to DEFAULT_IMAGE_HANDLE.id().


@mockersf
Copy link
Member

mockersf commented Feb 24, 2023

Could you check perfs with example many_buttons? They are very slightly worse (by ~0.15ms) on my laptop with this PR. This example has no images at all, so it would be supposedly worse with many textures nodes.

@mockersf mockersf added C-Performance A change motivated by improving speed, memory usage or compile times A-UI Graphical user interfaces, styles, layouts, and widgets S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help labels Feb 24, 2023
@ickshonpe
Copy link
Contributor Author

many_buttons draws lots of text, and each glyph is an image.
Text is a special case and I've got a separate idea on how to improve performance there, once I've finished with the text wrapping PR.

Try comparing with this example instead:

use bevy::{
    diagnostic::{FrameTimeDiagnosticsPlugin, LogDiagnosticsPlugin},
    prelude::*,
    window::{PresentMode, WindowPlugin},
};

fn main() {
    App::new()
        .add_plugins(DefaultPlugins.set(WindowPlugin {
            primary_window: Some(Window {
                present_mode: PresentMode::Immediate,
                ..default()
            }),
            ..default()
        }))
        .add_plugin(FrameTimeDiagnosticsPlugin::default())
        .add_plugin(LogDiagnosticsPlugin::default())
        .add_startup_system(setup)
        .run();
}

fn setup(mut commands: Commands) {
    commands.spawn(Camera2dBundle::default());
    let parent = commands.spawn(NodeBundle {
        style: Style {
            flex_basis: Val::Percent(100.),
            flex_wrap: FlexWrap::Wrap,
            align_content: AlignContent::FlexStart,
            align_items: AlignItems::FlexStart,
            justify_content: JustifyContent::FlexStart,
            ..Default::default()
        },
        background_color: BackgroundColor(Color::BLACK),
        ..Default::default()
    })
    .id();
    
    for _ in 0..100_000 {
        let child = commands.spawn(NodeBundle {
            style: Style {
                size: Size::all(Val::Px(2.0)),
                align_self: AlignSelf::FlexStart,
                ..Default::default()
            },
            background_color: BackgroundColor(Color::YELLOW),
            ..Default::default()
        })
        .id();    
        commands.entity(parent).add_child(child);
    }
}

@mockersf
Copy link
Member

Right, forgot that this example has text as it's too small to read 😄

Removing the text, this PR improves perfs by a similar amount 👍

@mockersf mockersf removed the S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help label Feb 25, 2023
@Weibye Weibye 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 Mar 12, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 17, 2023
Merged via the queue into bevyengine:main with commit 52b91ac Mar 17, 2023
20 checks passed
@james7132 james7132 added this to the 0.10.1 milestone Mar 17, 2023
@james7132
Copy link
Member

Adding to 0.10.1 as this is useful to anyone using UI and is non-breaking.

@ickshonpe
Copy link
Contributor Author

ickshonpe commented Mar 17, 2023

Some benches:

cargo run --example many_buttons --features trace_tracey --profile stress-test

many_buttons_prepare_default

A slight performance regression but many_buttons with text is about the worst possible case here.

cargo run --example many_buttons --no-default-features --features bevy_ui,bevy_sprite,bevy_render,bevy_core_pipeline,bevy_asset,bevy_winit,trace_tracy --profile stress-test

many_buttons_no_text

The second is `many_buttons` with text disabled. I don't know what the reason is for the two peaks.

Got the #[cfg(not(feature = "bevy_text"))] many_buttons on a separate branch I'm going to PR in a moment.

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-Performance A change motivated by improving speed, memory usage or compile times 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.

None yet

5 participants