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