szeged / webrender

A GPU-based renderer for the web
https://doc.servo.org/webrender/
Mozilla Public License 2.0
45 stars 7 forks source link

Remove all the uniforms #346

Closed zakorgy closed 4 years ago

zakorgy commented 4 years ago

This adresses https://github.com/szeged/webrender/issues/345 ~WIP because these changes are not tested on macOS yet.~


This change is Reviewable

kvark commented 4 years ago

Yes, what I mentioned is still what we need to do.

So the question is - is that "uniform Locals" the constant buffer associated with a render target (and changed at the render target frequency)? If so, can we call it something more descriptive than "Locals"?

-Dzmitry

On 1/16/20 2:01 PM, Gyula Zakor wrote:

@zakorgy commented on this pull request.


In webrender/build_gfx.rs https://github.com/szeged/webrender/pull/346#discussion_r367594605:

@@ -341,20 +331,11 @@ fn extend_sampler_definition( }

fn replace_non_sampler_uniforms(new_data: &mut String) {

  • new_data.push_str(
  • "\tlayout(push_constant) uniform PushConsts {\n\
  • \t\tmat4 uTransform; // Orthographic projection\n\
  • \t\t// A generic uniform that shaders can optionally use to configure\n\
  • \t\t// an operation mode for this batch.\n\
  • \t\tint uMode;\n\
  • \t} pushConstants;\n",
  • ); new_data.push_str(&format!( "\tlayout(set = {}, binding = 0) uniform Locals {{\n\ \t\tuniform mat4 uTransform; // Orthographic projection\n\

I think we have a misunderstanding here, in #345 https://github.com/szeged/webrender/issues/345 . you mentionedWebRender has it associated with the target, so we can safely move it into a constant buffer within the per-target descriptor set.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/szeged/webrender/pull/346?email_source=notifications&email_token=AAA2GJLLXUB4365MLEIE5UDQ6CVIJA5CNFSM4KFZLRT2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCSBTPHA#discussion_r367594605, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA2GJOEU6YNU6Y4F34ZPK3Q6CVIJANCNFSM4KFZLRTQ.

zakorgy commented 4 years ago

@kvark I've removed the descriptor binding related changes (I will make that in a different PR, updated #335 for that), added the change to set the uniforms on a per render target basis. I will squash the commits after the review.

zakorgy commented 4 years ago

That's right.