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

Improved UI render batching #8793

Merged
merged 21 commits into from
Jun 22, 2023
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
8c74012
In `prepare_uinodes` set the UVs to a large number if the quad is unt…
ickshonpe Jun 8, 2023
293eea6
cargo fmt --all
ickshonpe Jun 8, 2023
f119009
clippy::collapsible-if
ickshonpe Jun 8, 2023
a50b00d
Improved improved batching.
ickshonpe Jun 8, 2023
14f555c
cargo fmt --all
ickshonpe Jun 8, 2023
3373cdd
Fixed multiple textures bug.
ickshonpe Jun 9, 2023
7bfb12f
Switch to using a flag
ickshonpe Jun 9, 2023
42a2796
replace mode values with consts
ickshonpe Jun 9, 2023
1bffb98
Set
ickshonpe Jun 10, 2023
de1b468
Removed `last_z` from UiBatch` as it is always equal to `0`
ickshonpe Jun 10, 2023
5bb1fb6
Removed the `start != end` check in the main loop as it's redundant …
ickshonpe Jun 10, 2023
189fe19
Replaced `handle.id() != default_id` predicates with a function `is_t…
ickshonpe Jun 10, 2023
6b76e17
renamed `staored_image_handle` to `current_batch_image`
ickshonpe Jun 10, 2023
79fe5fd
Don't need to update `current_batch_image` when it is already equal t…
ickshonpe Jun 10, 2023
e7e686a
Removed the `sort_key` field from `TransparentUi`
ickshonpe Jun 10, 2023
d97219f
fix clippy::unused-unit
ickshonpe Jun 10, 2023
34a1d4f
Reversed the z ordering changes. They aren't important and can be lef…
ickshonpe Jun 11, 2023
be82618
Should be `QUAD_INDICES.len()`, I don't know how this got changed.
ickshonpe Jun 12, 2023
ebe7a39
The `start != end` check is required inside the loop as well to accou…
ickshonpe Jun 19, 2023
0871be0
Merge branch 'main' into improved-ui-batching
ickshonpe Jun 20, 2023
75d4092
`ui.wgsl`
ickshonpe Jun 20, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 29 additions & 13 deletions crates/bevy_ui/src/render/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,7 @@ struct UiVertex {
pub position: [f32; 3],
pub uv: [f32; 2],
pub color: [f32; 4],
pub mode: u32,
}

#[derive(Resource)]
Expand Down Expand Up @@ -382,6 +383,9 @@ pub struct UiBatch {
pub z: f32,
}

const TEXTURED_QUAD: u32 = 0;
const UNTEXTURED_QUAD: u32 = 1;
Comment on lines +591 to +592
Copy link
Contributor

Choose a reason for hiding this comment

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

In the futures, this could be turned into a bitflags, (for example if you want to cram more metadata into the vertex attributes) but I think it should be kept at two u32 consts for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I kept it this basic because the shader will be replaced soon for one that does rounded corners etc.


pub fn prepare_uinodes(
mut commands: Commands,
render_device: Res<RenderDevice>,
Expand All @@ -398,20 +402,31 @@ pub fn prepare_uinodes(

let mut start = 0;
let mut end = 0;
let mut current_batch_handle = Default::default();
let mut stored_batch_handle = DEFAULT_IMAGE_HANDLE.typed();
let mut last_z = 0.0;
let default_id = DEFAULT_IMAGE_HANDLE.id();

for extracted_uinode in &extracted_uinodes.uinodes {
if current_batch_handle != extracted_uinode.image {
if start != end {
commands.spawn(UiBatch {
range: start..end,
image: current_batch_handle,
z: last_z,
});
start = end;
let mode = if extracted_uinode.image.id() != default_id {
if stored_batch_handle.id() != default_id {
if stored_batch_handle.id() != extracted_uinode.image.id() {
if start != end {
commands.spawn(UiBatch {
range: start..end,
image: stored_batch_handle,
z: last_z,
});
start = end;
}
stored_batch_handle = extracted_uinode.image.clone_weak();
}
} else {
stored_batch_handle = extracted_uinode.image.clone_weak();
Copy link
Contributor

Choose a reason for hiding this comment

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

This code could be dramatically improved. It's very hard to parse and understand what is going on.

First, I would name the predicates:

    let is_extracted_default = extracted_uinode.image.id() == default_id;
    let is_batch_default     = stored_batch_handle.id() == default_id;
    let is_batch_extracted   = stored_batch_handle.id() == extracted_uinode.image.id() ;

Then, I would extract mode values outside of all that nasty logic:

    let mode = if !is_extracted_default { TEXTURED_QUAD } else { UNTEXTURED_QUAD };

Then, I would invert some comparisons so that the single lines are not separated from their predicate, it makes it easier to see when each line are triggered:

    let mode = if is_extracted_default { UNTEXTURED_QUAD } else { TEXTURED_QUAD };

    if !is_extracted_default {
        if is_batch_default {
            stored_batch_handle = extracted_uinode.image.clone_weak();
        } else {
            if !is_batch_extracted {
                stored_batch_handle = extracted_uinode.image.clone_weak();
                if start != end {
                    commands.spawn(UiBatch {
                        range: start..end,
                        image: stored_batch_handle,
                        z: last_z,
                    });
                    start = end;
                }
            }
        }

Finally, I notice a bit of redundant logic here, so we end up with

        if is_batch_default {
            stored_batch_handle = extracted_uinode.image.clone_weak();
        } else if !is_batch_extracted {
            stored_batch_handle = extracted_uinode.image.clone_weak();
            if start != end {
                commands.spawn(UiBatch {
                    range: start..end,
                    image: stored_batch_handle,
                    z: last_z,
                });
                start = end;
            }
        }

^^^ This is the code as I would submit it, but if it was a personal project, I would use the following:

    match () {
        () if is_extracted_default => {},
        () if is_batch_default || !is_batch_extracted && start == end => {
            stored_batch_handle = extracted_uinode.image.clone_weak();
        }
        () if !is_batch_extracted => {
            stored_batch_handle = extracted_uinode.image.clone_weak();
            commands.spawn(UiBatch {
                range: start..end,
                image: stored_batch_handle,
                z: last_z,
            });
            start = end;
        }
    }

Copy link
Contributor

Choose a reason for hiding this comment

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

AbleHeathen on discord mentioned another possible improvement:

if is_batch_default || !is_batch_extracted {
    stored_batch_handle = extracted_uinode.image.clone_weak();
}

if !is_batch_extracted && start != end {
    commands.spawn(UiBatch { 
      range: start..end,
      image: stored_batch_handle,
      z: last_z,
    });
    start = end;
}

Didn't check the logic, but seems even better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The conditionals are all necessary I think. It's more complicated than it looks at first when there are multiple different textures being queued.

This is the result we want:
prepare_uinodes

But with your changes we get:
prepare_uinodes_2

You are right that it can be cleaned up though, I'll see what I can do.

Copy link
Contributor

Choose a reason for hiding this comment

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

I triple checked my code. It wasn't easy to do (hence the remark about it being difficult to reason about) but I've done it slowly and step by step. I'm not 100% sure, but I'm fairly confident the logic is equivalent, at least for the following code:

    let mode = if is_extracted_default { UNTEXTURED_QUAD } else { TEXTURED_QUAD };

    if !is_extracted_default {
        if is_batch_default {
            stored_batch_handle = extracted_uinode.image.clone_weak();
        } else if !is_batch_extracted {
            stored_batch_handle = extracted_uinode.image.clone_weak();
            if start != end {
                commands.spawn(UiBatch {
                    range: start..end,
                    image: stored_batch_handle,
                    z: last_z,
                });
                start = end;
            }
        }
    }

I made a gist so the changes can be visualized: https://gist.github.com/nicopap/2511c8761eccb059757c88e4d6d69689/revisions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No this is definitely incorrect.
stored_batch_handle holds the handle of the texture for the next UiBatch.
Then after that UiBatch is spawned, we set stored_batch_handle to the texture for the next batch from extracted_uinode.

Your change overwrites stored_batch_handle first before the UiBatch is created so the batch will be spawned with the wrong texture. Also, it doesn't look like it will compile because UiBatch takes ownership of stored_batch_handle, so the value can't be used in the next iteration of the loop.

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 think the lastest version I just pushed is looking ok now. You were right that stored_batch_handle could be set at the end of the code paths, instead of in both of the different if blocks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed that this section of code is still hard to understand. I don't feel confident reviewing it atm.

}
current_batch_handle = extracted_uinode.image.clone_weak();
}
TEXTURED_QUAD
} else {
UNTEXTURED_QUAD
};

let mut uinode_rect = extracted_uinode.rect;

Expand Down Expand Up @@ -469,7 +484,7 @@ pub fn prepare_uinodes(
continue;
}
}
let uvs = if current_batch_handle.id() == DEFAULT_IMAGE_HANDLE.id() {
let uvs = if mode == UNTEXTURED_QUAD {
[Vec2::ZERO, Vec2::X, Vec2::ONE, Vec2::Y]
} else {
let atlas_extent = extracted_uinode.atlas_size.unwrap_or(uinode_rect.max);
Expand Down Expand Up @@ -514,6 +529,7 @@ pub fn prepare_uinodes(
position: positions_clipped[i].into(),
uv: uvs[i].into(),
color,
mode,
Copy link
Contributor

Choose a reason for hiding this comment

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

For other reviewers, this is like the most important line of this PR

});
}

Expand All @@ -525,7 +541,7 @@ pub fn prepare_uinodes(
if start != end {
commands.spawn(UiBatch {
range: start..end,
image: current_batch_handle,
image: stored_batch_handle,
z: last_z,
});
}
Expand Down
2 changes: 2 additions & 0 deletions crates/bevy_ui/src/render/pipeline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ impl SpecializedRenderPipeline for UiPipeline {
VertexFormat::Float32x2,
// color
VertexFormat::Float32x4,
// mode
VertexFormat::Uint32,
],
);
let shader_defs = Vec::new();
Expand Down
9 changes: 8 additions & 1 deletion crates/bevy_ui/src/render/ui.wgsl
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ var<uniform> view: View;
struct VertexOutput {
@location(0) uv: vec2<f32>,
@location(1) color: vec4<f32>,
@location(3) mode: u32,
@builtin(position) position: vec4<f32>,
};

Expand All @@ -14,11 +15,13 @@ fn vertex(
@location(0) vertex_position: vec3<f32>,
@location(1) vertex_uv: vec2<f32>,
@location(2) vertex_color: vec4<f32>,
@location(3) mode: u32,
) -> VertexOutput {
var out: VertexOutput;
out.uv = vertex_uv;
out.position = view.view_proj * vec4<f32>(vertex_position, 1.0);
out.color = vertex_color;
out.mode = mode;
return out;
}

Expand All @@ -30,6 +33,10 @@ var sprite_sampler: sampler;
@fragment
fn fragment(in: VertexOutput) -> @location(0) vec4<f32> {
var color = textureSample(sprite_texture, sprite_sampler, in.uv);
ickshonpe marked this conversation as resolved.
Show resolved Hide resolved
color = in.color * color;
if in.mode == 0u {
ickshonpe marked this conversation as resolved.
Show resolved Hide resolved
color = in.color * color;
} else {
color = in.color;
}
return color;
}