Re: [4/4] [wined3d] trigger constant reloading when RECT textures change

[Stefan Dösinger]

Am Mittwoch, 25. März 2009 14:52:15 schrieb Tobias Jakobi:

A few comments on all the patches. I am a bit busy at university right now, so 
I didn't have a chance to answer earlier:
> From patch 4/4:
> +        if (!use_ps(stateblock)) {
> +            TRACE("Non power two matrix multiply fixup\n");
> +            glMultMatrixf(((IWineD3DTextureImpl *) 
> stateblock->textures[texUnit])->baseTexture.pow2Matrix);
> +        }
> I think a comment would be helpful why this isn't done when pixel shaders are 
> used, as this can't be seen from the context of this code.
> +        if (GL_TEXTURE_RECTANGLE_ARB == 
> IWineD3DBaseTexture_GetTextureDimensions(stateblock->textures[sampler]) &&
> +            use_ps(stateblock)) {
> Maybe it would be helpful to add a flag that says if the fixup matrix is 
> identity. WineD3D can emulate np2 textures with padded GL_TEXTURE_2D textures 
> as well. Now I can't think of any hardware that has shaders, but not 
> GL_ARB_texture_rectangle, but checking for GL_TEXTURE_RECTANGLE_ARB to find 
> out if we need a fixup isn't that nice(We might do it in other places too, 
> though)
> From patch 1:
> +    /* Bitmap for texture rect coord fixups (32 samplers max currently).
> +     * Increase size when MAX_FRAGMENT_SAMPLERS grows. */
> +    UINT                        texrect_fixup;
> I think there's no point in supporting more than 16 samplers. That's the limit 
> in d3d9 and will never change, and d3d10 makes unconditional NP2 support 
> mandatory. The ps_compile_args structure is memcmp'ed every time the shader 
> is changed, so the size matters. I also recommend to move it to the bottom of 
> the structure, since it will be 0x0000 in most cases, so it prolongs the 
> number of bytes the memcmp has to check before it finds a difference.(ok: In 
> which direction does memcmp search?)

General:
I also think that you could merge the 4 patches into one or two. I appreciate 
the effort to make the patches small, but that shouldn't be a self-sufficient 
purpose. Patches 1 and 2 can certainly be merged, as patch 1 alone doesn't do 
anything. Patch 4 could be merged into patch 3 because it is required for the 
code in patch 3 to work correctly.

The patches leave out one corner case(Subject for a separate patch though): 
The use of a vertex shader with fixed function fragment processing. In this 
case the vertex shader has to take care of the fixup. In GLSL that can 
conveniently be done in the pixelshader<->vertexshader glue function. It 
could read the same uniform and multiply the written texture coordinate.

Once all the cases work in GLSL, we could experiment with the R and RG float 
formats from GL_NV_float_buffer, since they're supported on RECT textures 
only, but may come handy on pre-geforce 8 hardware which doesn't support the 
ARB_RG extension.