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

Fix UI draw order #9062

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

Conversation

ickshonpe
Copy link
Contributor

@ickshonpe ickshonpe commented Jul 6, 2023

Objective

Items to be drawn for the UI are added to batches in order of their position in the UiStack but the batches themselves are sorted by the z depth of the last item added to each batch. If a user modifies the z translation of the transform of a UI node it can result in batches being drawn in the wrong order.

This can potentially lead to very confusing bugs. A UI node with an invalid transform and all of its descendants may still display correctly, while another seemingly unrelated UI node that isn't even in the same transform hierarchy can disappear.

ZIndex

Also, there is a second issue if ZIndex is used. With ZIndex the draw order for UI items may not correspond to their depth in the UI layout parent-child hierarchy, in which case the transform propagation system will not propagate the z translations correctly.

ZIndex is the reason why it's not enough to just fix the sort order for TransparentUI and the z-components need to be overwritten.

Example

An app that draws three rows of text.

use bevy::prelude::*;
use bevy::window::WindowResolution;

const EXAMPLE_VALUES: &[(&str, Color, f32, f32)] = &[
    ("First Text Section", Color::RED, 24., 0.),
    ("Second Text Section", Color::GREEN, 16., 0.),
    ("Third Text Section", Color::BLUE, 16., 0.),
];

fn main() {
    App::new()
        .add_plugins(DefaultPlugins.set(WindowPlugin {
            primary_window: Some(Window {
                resolution: WindowResolution::new(250., 100.),
                ..Default::default()
            }),
            ..Default::default()
        }))
        .add_systems(Startup, setup)
        .run();
}

fn spawn_element(commands: &mut Commands, message: String, color: Color, font_size: f32, z_depth: f32) -> Entity {
    commands
    .spawn(NodeBundle {
        style: Style {
            flex_direction: FlexDirection::Column,
            ..Default::default()
        },
        transform: Transform::from_translation(z_depth * Vec3::Z),
        ..Default::default()
    }).with_children(|commands| {
        commands.spawn(TextBundle {
            text: Text::from_section(
                message,
                TextStyle {
                    font_size,
                    color,
                    ..Default::default()
                },
            ),
            ..Default::default()
        });
    })
    .id()

}

fn setup(mut commands: Commands) {
    commands.spawn(Camera2dBundle::default());
    let mut parent =  commands
        .spawn(NodeBundle {
            style: Style {
                flex_basis: Val::Percent(100.),
                ..Default::default()
            },
            background_color: Color::BLACK.into(),
            ..Default::default()
        })
        .id();

    for &(message, color, font_size, z_depth) in EXAMPLE_VALUES {
        let child = spawn_element(&mut commands, message.into(), color, font_size, z_depth);
        commands.entity(parent).add_child(child);
        parent = child;
    }
}

As expected:

z_1

The text nodes are leaf nodes in a tree:

black background root node
|
node -> red text leaf node
|
node -> green text leaf node
|
node -> blue text leaf node

Changes to any node's transform will be propagated down the tree.

So, if we change the z-component of the translation of the first child node to -100:

const EXAMPLE_VALUES: &[(&str, Color, f32, f32)] = &[
    ("First Text Section", Color::RED, 24., -100.),
    ("Second Text Section", Color::GREEN, 16., 0.),
    ("Third Text Section", Color::BLUE, 16., 0.),
];

The z translation of -100 is propagated down the transform hierarchy and none of the text is shown:

z_2

But now look what happens if we set the z translation of the third text bundle to -100 instead:

const EXAMPLE_VALUES: &[(&str, Color, f32, f32)] = &[
    ("First Text Section", Color::RED, 24., 0.),
    ("Second Text Section", Color::GREEN, 16., 0.),
    ("Third Text Section", Color::BLUE, 16., -100.),
];
z_3

You'd naturally expect only the third text section to vanish. But the second section is also gone.

What has happened is that prepare_ui_nodes sorts the extracted UI items by their stack index, which results in an ordering of:

  • The black background
  • The red text section
  • The green text section
  • The blue text section

Next, the items are separated into batches for each texture. This example uses two texture atlases: one to store the glyphs used by the red text section and one to store the glyphs for the smaller green and blue text sections. Each texture atlas has one texture, so there are two UI batches:

  • The first batch contains the black background (untextured items can be added to any batch) and the glyphs for the red text section.
  • The second batch contains the glyphs for the green and blue text sections.

These batches are then sorted by the z depth of the last item added to each batch.
Since the blue text has the highest stack index, the last glyph from the blue text section is the last item added to the second batch. But since it also has a z coordinate of -100, the second batch will be drawn first, and then painted over by the black background when the first batch is drawn.

This is only a simple example. If you set a ZIndex as well, it can get extremely complicated.

Solution

Set the z translation for UI nodes to zero every time the layout is updated.

Sort the UI batches so that the batches with the lowest stack index are drawn first.


Changelog

ui_layout_system

  • Now sets each UI node's Transform's translation z-component to zero every time it's run.

TransparentUi

  • Changed the type of the sort key to u32.

UiBatch

  • Removed the z field.
  • Added a field order: u32. The batches are now sorted by this field.

prepare_uinodes

  • Removed the z-depth ordering.
  • Now the batches are enumerated by the order in which they are created.

ickshonpe added 2 commits July 6, 2023 20:57
* Changed the type of its sort key to u32.

`UiBatch`
* Removed `z` field.
* Added a field `order: u32`. The batches are sorted by this field.

`prepare_uinodes`
* Replaced the z depth ordering with the natural batch order ordering.
@IceSentry IceSentry added A-UI Graphical user interfaces, styles, layouts, and widgets C-Feature A new feature, making something new possible labels Jul 6, 2023
@ickshonpe
Copy link
Contributor Author

This should be labelled a bug as well.

@ickshonpe ickshonpe changed the title Draw UI Nodes in order of their stack index. UI Nodes should be drawn in order of their position in the UiStack Jul 7, 2023
@ickshonpe ickshonpe changed the title UI Nodes should be drawn in order of their position in the UiStack UI Nodes should be drawn in order of their position in UiStack Jul 7, 2023
@ickshonpe ickshonpe changed the title UI Nodes should be drawn in order of their position in UiStack Fix UI draw order Aug 22, 2023
@ickshonpe ickshonpe mentioned this pull request Aug 22, 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-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants