Re: wined3d: Another GLSL shader status update

[Jason Green]

On 6/4/06, Ivan Gyurdiev  wrote:
> - /* Declare textures samplers */
> - for(i = 0; i < /*This->baseShader.limits.texture */ 4; i++) {
>
> +//        if (reg_maps->texcoord & (1 << i))
> -
>        shader_addline(buffer,"uniform sampler2D mytex%lu;\n", i);
> - }
>
> What's the problem here?

A lot of this is just from debugging.  The code is still in the proof
of concept phase.
> -
>            if (wined3d_settings.shader_mode == SHADER_GLSL)
> -
>                shader_glsl_add_instruction_modifiers(&hw_arg);
>
> If you choose to pull modifier handling out of the per-opcode function,
> this should be done for SHADER_ARB as well, imho.

All of the ARB modifiers are run prior to the command.  Whereas, (at
least so far), the GLSL instruction modifiers are necessary to run
after the command.
> -
>    for (i=0; i<max_constants; ++i) {
>
> ...
> -
>                for (i = 0; i <=  WINED3D_VSHADER_MAX_CONSTANTS; ++i) {
>
> Is this fixing off-by-one bug in the old code in the process (or
> introducing one)?

Possibly introducing - nice catch.
> -
>            loadConstants(iface, GL_VERTEX_PROGRAM_ARB, constants,
>
> NULL,
> -
>
> vshader->baseShader.limits.constant_float, programId, TRUE);
>
> What's the purpose of the "skip_type_check" parameter - TRUE here.
> Doesn't the current code only work for float constants anyway?

It's passing NULL as the type array (because VertexDeclaration
constants are always floats and they don't have one).
> -
>        /* Update the constants */
> -
>        loadConstants(iface, GL_FRAGMENT_PROGRAM_ARB,
>
> This->stateBlock->pixelShaderConstantF,
> -
>                      This->stateBlock->pixelShaderConstantT,
>
> pshader->baseShader.limits.constant_float,
> -
>                      programId, FALSE);
>
> Those limits aren't checked against GL capabilities yet.
> The current 3.0 limit is 224 float constants - does this work properly?

The limits should be checked against the caps in the functions that
set all of the limits based on the shader version.  We should probably
do something like set all of the limits as they "should" be, then do a
min() against the GL caps.
> +void shader_glsl_add_instruction_modifiers(SHADER_OPCODE_ARG* arg
>
> Doesn't seem to handle multiple modifiers - I think I changed this
> elsewhere recently.

Possibly not.  I just haven't hit (or noticed) that test case yet.
> - SHADER_PARAM_FCT_T              param_fct;
>
> Please don't add any more fields that do not need to persist into base
> shader. Why are two different param functions needed?

This method avoids the base class having to know about its subclasses.
 I'm sure there's a better way to do it, still, though.
> - /* Set constants for the temporary argument */
>
> I'm not sure I like how the mnxn functions use a temporary argument
> structure and re-invoke the gen. functions. Do they initialize every
> field necessary in the arg? I might have added new fields and missed the
> initialization part in there - maybe this arg should be 0-ed first with
> memcpy.

Agreed.
> -
>    GLhandleARB objList[4];   /* There should never be more than 2
>
> objects attached
> -
>                                 (one pixel shader and one vertex
>
> shader), but we'll be safe */
>
> This doesn't make sense to me - either you know that there are 2 objects
> attached exactly, or you don't. In either case, it's wrong to cover up a
> potential bug with a 4-element array.

Yeah, I must have been tired.  :-)
> -
> - Note: The downside to this method is that we may end up compiling
>
> and linking
> -
> - programs that will not be used.  Linking is expensive, but it's
>
> typically better to
> -
> - create more programs than necessary and just bind them when needed
>
> than it is
> -
> - to create a new program every time you set the shaders
> - */
>
> This is an elaborate scheme with linked lists in each shader - is that
> really necessary?
> I'm very confused after reading the code (but maybe that's just me :)

If we link the programs together every time the Set__Shader() is
called, it brings performance to a halt (that what my glsl_hack4.diff
did).  Everything was roughly 100 times slower that way.  So, this
method stores the program for every combination which is used in the
app.  That way, when the shaders are set, we already have a linked
program to use.  Linking is a very expensive operation.  Keeping a
list of them seems like the best method that we could come up with.
> If I call:
>
> X = CreatePixelShader(dev);
> Y = CreateVertexShader(dev);
> Z = CreateVertexShader(dev);
> SetPixelShader(dev, X);
> SetVertexShader(dev, Y);
> SetVertexShader(dev, Z);
>
> How many programs are being created?

3
> What objects are attached to each?

Program:    Pixel Shader:    Vertex Shader:
   1                 X                      NULL
   2                 X                       Y
   3                 X                       Z
> Which linked lists does each program go into, and
> What's the sequence of things being attached/detached, programs
> created/destroyed?

If you run the WINEDEBUG=+d3d_shader trace, you'll get detailed
information about all of the steps involed.  Currently, the only time
that the list doesn't get de-allocated is when the app doesn't
properly clean up after itself.  So, that will need to be fixed before
this code can be submitted.

On a separate topic, we've learned that the reason that some of these
pixel shaders fail is because I've hard-coded texture2D() and just
assumed that the textures would be 2-dimensional (which is what the
ARB programs do now, too).  However, in ARB, only the instructions
that reference the 3D texture are failing.  In GLSL, the entire shader
fails if one of the textures is wrong.  So, this will have to be
thought out some more, since prior to PS 2.0, the app doesn't have to
declare if it's a 2D or a 3D texture ahead of time.