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

Include UI node size in the vertex inputs for UiMaterial. #11722

Merged
merged 2 commits into from
Feb 6, 2024

Conversation

viridia
Copy link
Contributor

@viridia viridia commented Feb 5, 2024

Objective

Includes the UI node size as a parameter to the UiMaterial shader, useful for SDF-based rendering, aspect ratio correction and other use cases.

Fixes #11392

Solution

Added the node size to the UiMaterial vertex shader params and also to the data that is passed to the fragment shader.

Migration Guide

This change should be backwards compatible, using the new field is optional.

Note to reviewers: render pipelines are a bit outside my comfort zone, so please make sure I haven't made any mistakes.

Use provided method for converting size to [f32;2]

Co-authored-by: Rob Parrett <[email protected]>
@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets labels Feb 5, 2024
@cart
Copy link
Member

cart commented Feb 6, 2024

I think the most controversial aspect of this is bandwidth usage. Each piece of vertex data we include reduces the number of UI widgets we can draw in a frame. We're now encoding writing/reading four copies of the same vec2 for each node. Given what I've seen on sprite benchmarks, this will likely show up in a reasonably big way on UI benchmarks.

@viridia
Copy link
Contributor Author

viridia commented Feb 6, 2024

@cart What's the alternative?

You are right that we don't need this information to be repeated, it could be a uniform.

But we do need this info to be accessible in some fashion - without this, UiMaterials aren't very useful, or at least, I can't think of many use cases that don't require knowledge of the size.

Yes, you can get around this by having a separate ECS system that measures the size of each node and updates the uniforms, but now you have to have a separate instance of the material for every widget - which I imagine is even less efficient than having extra per-vertex data.

@JMS55
Copy link
Contributor

JMS55 commented Feb 6, 2024

If we really wanted a performant UI renderer, we would use storage buffers to pull UI data from instead of repeating it per-vertex. For now, I'd rather focus on getting a usable and feature-ful UI system, and then come back to performance later, personally.

@cart
Copy link
Member

cart commented Feb 6, 2024

What's the alternative?

The alternative is splitting the pipeline / breaking batches for things that need the size, so we only pay the price in the cases that need it (although the cost of rebinds from splitting the pipeline might outweigh the data read/write costs) . And/or switching to storage buffers as @JMS55 mentioned. However we can't use storage buffers on WebGL, so we'd need a fallback.

Fortunately I just ran the many_buttons benchmark with 100,000 buttons and this change (surprisingly) didn't show up meaningfully for me. I'm down to call it a wash for now and we can revisit later.

@JMS55 JMS55 added this to the 0.13 milestone Feb 6, 2024
@alice-i-cecile alice-i-cecile 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 Feb 6, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 6, 2024
Merged via the queue into bevyengine:main with commit a57832b Feb 6, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets C-Feature A new feature, making something new possible 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.

UI node size in vertex shader
5 participants