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.