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

Shader UI #1158

Closed
wants to merge 6 commits into from
Closed

Shader UI #1158

wants to merge 6 commits into from

Conversation

TheRawMeatball
Copy link
Member

This PR implements Overflow::Hidden, rounded corners and borders by extending the UI fragment shader. There are various bugs: anti-aliasing is broken for rounded overflows, overflow mask randomly shifts 1px up or down on certain node widths and text is entirely ignored, but its ready enough to talk over.

@Moxinilian Moxinilian added C-Feature A new feature, making something new possible A-UI Graphical user interfaces, styles, layouts, and widgets labels Dec 28, 2020
@cart
Copy link
Member

cart commented Jan 3, 2021

Ok this is really cool 😄 Huge step forward in usability.

Assorted thoughts:

  • In general I like the public api exposed. I think this is basically good to go from that perspective
  • We might be able to optimize this by only doing border rendering / data transfer for nodes where that applies. But its hard to make a decision about that without benchmarking. A good place to start would be to do a frame-time comparison of the ui.rs example on master vs this branch.
  • These algorithms are new to me, so I'll need some time to look over them.
  • I'd love to consolidate bindings, but thats largely out of your hands at this point. We need the RenderResources derive to be a bit more flexible. If/when we do that, we can optimize shaders everywhere.
  • I think all of the #[render_resources(ignore)] calls in Style are a good illustration that sometimes having separate datatypes for gpu data and components is a good idea. I think Color is another case that would benefit from that. Not something that we need to handle here, but once we adjust RenderResources / RenderResourcesNode to handle that nicely, we should revisit Style.

@TheRawMeatball
Copy link
Member Author

I don't have much to say on most of these points, but for the last one I'd rather not have an extra system shoveling data into a different type. An alternative I considered is an #[render_resources(invert)] tag so its easier to pick a small subset, but I'd like your input before implementing it.

@cart
Copy link
Member

cart commented Jan 3, 2021

Yeah it wouldn't be an extra system. It would be something more along the lines of RenderResourcesNode taking Into instead of RenderResources.

Imo #[render_resources(invert) would add too much complexity for too little benefit (both in terms of implementation complexity and "user facing" complexity). One more quirk people need to think about.

@TheRawMeatball
Copy link
Member Author

Hmm, how about #[render_resources(From<R>)] with a type parameter on the RenderResources trait? If not, I'll implement something taking Into instead.

@cart
Copy link
Member

cart commented Jan 8, 2021

Hmm, how about #[render_resources(From)] with a type parameter on the RenderResources trait? If not, I'll implement something taking Into instead.

I think I'd prefer it if we made RenderResourcesNode take Into<RenderResources>. That feels cleaner to me. The only major consideration is that RenderResourcesNode currently assumes that it can use RenderResources methods directly. We need to make sure it doesn't don't do a bunch of unnecessary conversions to RenderResources, as Into<RenderResources> could have a non-zero cost (and might allocate).

@cart
Copy link
Member

cart commented Jan 8, 2021

Semi-related: #1208 (comment)

Base automatically changed from master to main February 19, 2021 20:44
@TheRawMeatball
Copy link
Member Author

The code in this PR is rather old, and implementation very much less-than-ideal, so I'm closing this PR :)

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.

4 participants