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.