wined3d: Another GLSL shader status update

[Ivan Gyurdiev]

+    /* 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?

+                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.

+        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)?

+                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?

+            /* 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?

+void shader_glsl_add_instruction_modifiers(SHADER_OPCODE_ARG* arg

Doesn't seem to handle multiple modifiers - I think I changed this 
elsewhere recently.

+            FIXME("Unrecognized modifier(0x%#lx)\n", mask >> 
D3DSP_DSTMOD_SHIFT);

0x and # are redundant.

+    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?

+extern void shader_glsl_add_cast(
+    int numSwizzles,
+    char *hwLine);

Where is this defined?

+void pshader_glsl_def(SHADER_OPCODE_ARG* arg) {
...
+    shader->constants[reg & 0xFF] = VS_CONSTANT_CONSTANT;

That's not what the ARBfp code does - pixel shaders are different from 
vertex shader (and I don't see why, conceptually this is all the same 
thing, only the limit is slightly different).

+    /* 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.

+        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.

+ *  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 :)
Maybe a test case would be useful?

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?
What objects are attached to each?
Which linked lists does each program go into, and
What's the sequence of things being attached/detached, programs 
created/destroyed?