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