wined3d: Another GLSL shader status update

[Ivan Gyurdiev]

>>
>> +                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.
Well, _SAT needs to go on the same opcode, but shifts for example go 
"after" the command. I have no idea how to do centroid right now.  
Anyway, it just seemed to me that they should be in the same place - 
either both in the generation fn, or both out of it, for consistency. 
There's a lot of SHADER_ARB opcodes right now that don't handle 
modifiers, while they need to do that, so we should think about 
centralized modifier processing [ or at least add modifier processing on 
the opcodes where it is missing ].
>>
>> 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.
No, I think we need to do error reporting as well - a min would hide the 
bugs. If the app uses too many constants, we should probably abort and 
report that to the application somehow. Right now our shader function 
doesn't even return a status code - maybe it should ?

>> +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.
I see _centroid with _pp sometimes, so since we ignore _pp that's not 
really an issue. Anyway, the spec says multiple output modifiers are 
allowed, so they should be supported.
>
>> +    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.
If the param functions aren't too different they should probably be 
merged together somehow. Also, fields like these would better go into 
the SHADER_OPCODE_ARG, not into the shader class - they're only used 
during generation. There's really no problem adding new fields into the 
arg structure - some of those don't need to be reset on every 
instruction, so they should just be set at the beginning when you start 
to generate code.

>> 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.  
Well, it seems to store a bit more than that. If I create pshaders A,B, 
and vshaders C,D, and do:
set_pixel(A); set_vertex(C);
draw_stuff();
set_pixel(B); set_vertex(D);
draw_stuff();
set_pixel(NULL), set_vertex(NULL)

Won't this create...5 different programs, all kept in memory for the 
entire lifetime of the program, while we only need 2 [1 active]?
(A, NULL), (A,C), (B,C), (B,D), (NULL, D)

Also, is it necessary to store link pair info in both shaders - isn't 
this redundant somehow?
> 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.
Ok... I guess maybe it's necessary.
> 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.
Yes, I'm not really sure how this is handled prior to ps2, but surely 
there's tutorials online that explain this. Maybe the shader does its 
own per-type processing using those texreg2ar, and other instructions 
(tex matrix instructions?). For 2.0+ I can send you a patch, which 
probably works, but needs more testing. It stores the sampler types in 
the SHADER_OPCODE_ARG - I just changed the hw_tex function a little bit.