WineD3D patches for review and discussion - not to be applied as-is.

[Jason Green]

All of these apply to the latest Git tree, and should come after my
last patch submission to wine-patches, but it shouldn't matter.  I'm
just looking for a little more feedback on these before I submit.
This should lay the groundwork to begin implementing GLSL opcode
generation.  I've hacked around with some of them, and this will
succesfully generate a GLSL shader program if the opcodes are
implemented properly.

Please let me know if any of my ideas here are fundamentally flawed.
We've worked a bunch of it out on IRC, but this should be a nicer
version to look at than my stuff at pastebin.  :-)

None of this code changes anything for the end-result
ARB_vertex/fragment_program.  It only generates GLSL if you have this
regkey:

HKLU\Software\Wine\Direct3D\UseGLSL = "enabled"

WineD3D patches for review and discussion - not to be applied as-is.

[H. Verbeet]

On 26/05/06, Jason Green  wrote:
/* baseshader.c */
> - /* Declare address variables */
> - for (i = 0; i < This->baseShader.limits.address; i++) {
> -
>    if (reg_maps->address & (1 << i))
> -
>        shader_addline(buffer, "ivec4 A%ld;\n", i);
> - }
> -
> - /* Declare temporary variables */
> - for(i = 0; i < This->baseShader.limits.temporary; i++) {
> -
>    if (reg_maps->temporary & (1 << i))
> -
>        shader_addline(buffer, "vec4 R%lu;\n", i);
> - }

Not so much about this patch, but should probably be mentioned
nevertheless: The register maps are a 32-bit int, but depending on
which shader model version you're targetting, the value for the limits
can be much higher than that. I think using a byte array would be
better.

/* vertexshader.c / pixelshader.c */
> -
>    /* Declare all named attributes (TODO: Add this to the

reg_maps and only declare those that are needed)*/
> -
>    for (i = 0; i < 16; ++i)
> -
>        shader_addline(&buffer, "attribute vec4 attrib%i;\n", i);

Is there a reason this can't be done from inside generate_base_shader()?
> -
>    GL_EXTCALL(glGetObjectParameterivARB(shader_obj,
> -
>               GL_OBJECT_COMPILE_STATUS_ARB, &success_flag));
> -
>    if (success_flag == FALSE) /* Print any compile errors in

our shader */
> -
>        print_glsl_info_log(&GLINFO_LOCATION, shader_obj);

I don't think this shouldn't be needed. Afaik calling
print_glsl_info_log() should do nothing if linking is succesfull.

/* */
> - /* If we're using GLSL, we need to possibly create a Shader

Program at this point,
> -
> * unless one is already set, then we need to attach the vertex

shader object to it */
> - if (NULL != pShader && wined3d_settings.shader_mode == SHADER_GLSL
> -
>    && oldShader != pShader) {

We should take into account the case where a shader is currently set,
and SetVertexShader is called with NULL. The shader should be detached
in that case.
The structure of the code should probably look something like this:

if (old_shader == new_shader) return;
program_id = get_program();
if (old_shader) detach(old_shader_id);
if (new_shader) attach(new_shader_id);
link(program_id);
> -
>    GLhandleARB programId = This->updateStateBlock->shaderPrgId;
> -
>    GLhandleARB shaderObj =

((IWineD3DVertexShaderImpl*)pShader)->baseShader.prgId;
> -
>    int success_flag = 0;
> -
> -
>    if (programId == 0) {
> -
>        /* No shader program is set, create a new program */
> -
>        programId = GL_EXTCALL(glCreateProgramObjectARB());
> -
>        TRACE_(d3d_shader)("Creating new shader program %u\n", programId);
> -
> -
>    /* Store this programId on the stateblock */
> -
>       This->updateStateBlock->shaderPrgId = programId;

Getting/Creating the program id could probably be a function of it's
own. You need it in when setting the pixel / vertex shader, in both
the device and the stateblock's Capture / Apply methods.
> -
>    GL_EXTCALL(glGetObjectParameterivARB(programId,
> -
>               GL_OBJECT_LINK_STATUS_ARB, &success_flag));
> -
>    if (!success_flag) {
> -
>        /* Print any linking errors in our shader */
> -
>        print_glsl_info_log(&GLINFO_LOCATION, programId);

See above.
> -
>        /* Bind vertex attributes to a corresponding index number */
> -
>        int i;
> -
>    char tmp_name[10];
> -
>        for (i = 0; i < 16; ++i) {
> -
>            sprintf(tmp_name, "attrib%i", i);
> -
>            glBindAttribLocationARB(programId, i, tmp_name);
> -
>        }

Similar to the comment for vertexshader.c / pixelshader.c, can't this
be in the same place we set all the other program attributes?

/* stateblock.c */
When we Capture / Apply the stateblock, we can't just copy the shader
id's over. We need to do the whole detach/attach/link thing again,
creating a new program object if needed. Somewhat related to that, I
think most of the Set functions on the device should just call a
function in the stateblock, rather than duplicating things between the
device and the stateblock.