Re: [PATCH 7/7] wined3d: Don't use the builtin FFP uniform for the modelview matrix.

[Stefan Dösinger]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi

I've given this only a very quick look. My first impression is that a
few things can be split out into separate patches, e.g.
get_modelview_matrix. That's a fairly minor thing though.

On a somewhat related subject, I think we should check at some point
that the state linking still works as intended and doesn't accidentally
merge everything together into one huge state that's always updated.

Am 2015-03-17 um 15:52 schrieb Matteo Bruni:
> @@ -775,6 +777,15 @@ static void shader_glsl_load_constants(void *shader_priv, struct wined3d_context
>          checkGLcall("glUniform4fv");
>      }
>
> +    if (update_mask & WINED3D_SHADER_CONST_FFP_MODELVIEW)
> +    {
> +        struct wined3d_matrix mat;
> +
> +        get_modelview_matrix(context, state, &mat);
> +        GL_EXTCALL(glUniformMatrix4fv(prog->vs.modelview_matrix_location, 1, FALSE, (GLfloat *)&mat));
> +        checkGLcall("glUniformMatrix4fv");
> +    }
> +
>      if (update_mask & WINED3D_SHADER_CONST_PS_F)
>          shader_glsl_load_constantsF(pshader, gl_info, state->ps_consts_f,
>                  prog->ps.uniform_f_locations, &priv->pconst_heap, priv->stack, constant_version);
> Is it a good idea to put this in shader_glsl_load_constants? It may make
> sense to split fixed function and shader constants into separate
> functions. We'll never need shader constants and the modelview matrix in
> the same program.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBAgAGBQJVCKLoAAoJEN0/YqbEcdMw9wIQAIrMIkc5I9LTRiYnUI+VRAxq
wkSX0Uwny7D1sVPCpJxrUu6I04K0u2oWIr0o5XmYQzzETrgthPCsifaHVGlInuaq
BfJQCGE1OFEEtFKBtIIB8TCQpMM6dwxYnCc4Zo+rPX2oodJFe9TplJnQw/WUw/Td
OeMXNKD316ESUbTa9O5CdMyEcw9+U0TKc2MUGHX5QjGMqKEoYA8wfP+r5VU/dNHo
5hMNenwMawaXgICAsxwQQr/YmCRrUuLvLeAJSCNLGgqSH6v5S4tVmW9tgTjT9Idv
RHtFkrsiKg0FKDcyQc1vdmFYtpijE44bM4ULdFCJfIgeRl76Gt0WI17aEZpVvJOx
HG5nZsidn/TXwkGWla8CT6k7l4JF8rDNgFbOWkQ7JMTRBThamphABgGR9I57L+9+
mCDUqm775VWpBMHbS4iEM4y1s/K17zDRisd9lbP9JFTOGJ2qT31d3+6Ba4t6FGJR
mRZiLE+QVWaScWEJaktC4Cbcl1xKQfu653XdL6YumxRBU5fWBEGZnbSwGh/ePpim
YZrC0zHE8E1pVwu6hUfNzahB8L/7P7NoPoWb813NBkk8tul7dCCWcZZ0W75cJgac
R0sb4mPaLZ+TrWR4YaQ1jZ66WsABKcDPiGZmAzJpxnCcIVy2uZcC3C3zxXDTQqHZ
A3SrlC8X/Vx4iFLg1eD7
=jJVx
-----END PGP SIGNATURE-----

[PATCH 7/7] wined3d: Don't use the builtin FFP uniform for the modelview matrix.

[Matteo Bruni]

2015-03-17 22:55 GMT+01:00 Stefan Dösinger <stefandoesinger(a)gmail.com>:
> On a somewhat related subject, I think we should check at some point
> that the state linking still works as intended and doesn't accidentally
> merge everything together into one huge state that's always updated.

It seems to be still working at the moment, I had multiple instances
of forgetting to add some state dirtification and some tests failing
as a consequence. Clearly that doesn't count as a thorough check
though.
> Am 2015-03-17 um 15:52 schrieb Matteo Bruni:
> > @@ -775,6 +777,15 @@ static void shader_glsl_load_constants(void *shader_priv, struct wined3d_context
> >          checkGLcall("glUniform4fv");
> >      }
> >
> > +    if (update_mask & WINED3D_SHADER_CONST_FFP_MODELVIEW)
> > +    {
> > +        struct wined3d_matrix mat;
> > +
> > +        get_modelview_matrix(context, state, &mat);
> > +        GL_EXTCALL(glUniformMatrix4fv(prog->vs.modelview_matrix_location, 1, FALSE, (GLfloat *)&mat));
> > +        checkGLcall("glUniformMatrix4fv");
> > +    }
> > +
> >      if (update_mask & WINED3D_SHADER_CONST_PS_F)
> >          shader_glsl_load_constantsF(pshader, gl_info, state->ps_consts_f,
> >                  prog->ps.uniform_f_locations, &priv->pconst_heap, priv->stack, constant_version);
> > Is it a good idea to put this in shader_glsl_load_constants? It may make
> > sense to split fixed function and shader constants into separate
> > functions. We'll never need shader constants and the modelview matrix in
> > the same program.

There are a few uniforms which are going to be "shared" between fixed
function and programmable pipeline (from a quick look I see clipplanes
emulation, position fixup, fog emulation, alpha test) so at least part
of the function would probably stay in a common helper. At that point
I'm not sure the separation would help a lot.

I'm going to try to split this patch up in more digestible chunks and resend.