Improved UI render batching

[ickshonpe]

Objective

prepare_uinodes creates a new UiBatch whenever the texture changes, when most often it's just queuing untextured quads. Instead of switching textures, we can reduce the number of batches generated significantly by adding a condition to the fragment shader so that it only multiplies by the textureSample value when drawing a textured quad.

Solution

Add a mode field to UiVertex.
In prepare_uinodes set mode to 0 if the quad is textured or 1 if untextured.
Add a condition to the fragment shader that only multiplies by the color value from textureSample if mode is set to 1.

---

Changelog

• Added a mode field to UiVertex, and added an extra u32 vertex attribute to the shader and vertex buffer layout.
• In prepare_uinodes mode is set to 0 for the vertices of textured quads, and 1 if untextured.
• Added a condition to the fragment shader in ui.wgsl that only multiplies by the color value from textureSample if the mode is equal to 0.

[ickshonpe]

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

[nicopap]

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;
        }
    }

[nicopap]

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.

[ickshonpe]

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:

But with your changes we get:

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

[nicopap]

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

[ickshonpe]

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.

[ickshonpe]

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.

[JMS55]

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

[nicopap]

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

[nicopap]

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.

[ickshonpe]

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

[nicopap]

Looks good to me. The logic of that nested if is much more apparent with the is_textured function. Just one question regarding the sort_key.

[nicopap]

Nit: I still would separate the logic from the value setting, in the imperative style (I say that as someone who has a strong bias in favor of functional programming)

Suggested change

	let mode = if is_textured(&extracted_uinode.image) {
	let mode = if is_textured(&extracted_uinode.image) {
	TEXTURED_QUAD
	} else {
	UNTEXTURED_QUAD
	};
	if is_textured(&extracted_uinode.image) {

[ickshonpe]

Removed the z ordering changes, they aren't important right now. Better to keep this simple.

[nicopap]

We should consider merging this before 0.11. It's a very small change with a major positive performance impact.