[Bug 14762] GeforceFX series: fullscreen PP effect issues / RECT texcoord fixup

[wine-bugs at winehq.org]

http://bugs.winehq.org/show_bug.cgi?id=14762





--- Comment #46 from Tobias Jakobi <liquid.acid at gmx.net>  2008-12-05 02:54:49 ---
Hi Stefan!

(In reply to comment #43)
> I'll look at your code, but I don't think you want to access an
> IWineD3DSurfaceImpl anywhere here. You probably have an IWineD3DBaseTexture *
> and are casting it to an IWineD3DSurfaceImpl *, which as you guessed is not
> correct. If you know that it is a RECT texture, you know you have an
> IWineD3DTexture *, so you can safely cast it to IWineD3DTextureImpl *. In there
> you have the np2 correction matrix, and you'll want to use element 0 and 5 from
> it.
I'm using a cast to IWineD3DTextureImpl now, which does seem to work:
const IWineD3DTextureImpl *tex = (IWineD3DTextureImpl*)stateBlock->textures[i];

However IWineD3DTextureImpl doesn't contain a np2 correction matrix. I'm using
the width and height members instead, which seem to contain the correct
information. At least the dimensions printed by the debug code are now correct
(backbuffer dims).

> This is the matrix:
> 
> w 0 0 0
> 0 h 0 0
> 0 0 1 0
> 0 0 0 1
> 
> This shows why you want elements 0 and 5
I assume this is the matrix normally used when doing the fixup in the fixed
function pipeline?

(In reply to comment #44)
> Yes, this is the problem:
> 
> /* can we safely assume that the cast works here? */
> const IWineD3DSurfaceImpl *surf =
> (IWineD3DSurfaceImpl*)stateBlock->textures[i];
> const float tex_dim[2] = {surf->currentDesc.Width, surf->currentDesc.Height};
> 
> The cast is not safe, because a texture is not a surface. It is a container
> that contains one or more surfaces(or volumes, in case of a 3D texture).
Yeah, I'm still a bit confused by all the terminology used :)
Thanks for explaining!

> I also noticed this:
> if(arg->opcode_token & WINED3DSI_TEXLD_BIAS) {
>     ...
> +    /* what to do here when texrect is enabled? */
>     shader_addline(arg->buffer, "%s(Psampler%u, %s, %s)%s);\n",
> }
> 
> I'd argue that this should never happen, because D3DSI_TEXLD_BIAS specifies a
> level of detail bias to use a different mipmap sublevel(see below), which
> doesn't make much sense with RECT textures since they only have one level
> anyway. However, it could happen that D3D supports this, and simply ignores the
> LOD Bias in this case. I think this is a candidate for a test case.
OK, I keep the testcase thing in mind, but I prefer to first get the rest
running. I think I just drop a FIXME there if texrect is enabled. This way we
at least see when this situation does happen or if it happens at all.

> FYI:
> A 1024x1024 2D texture can for example contain 10 surfaces, which are then
> usually called mipmap sublevels. The first one has 1024x1024 pixels, the 2nd
> 512x512, then 256x256 etc. Depending on the level of detail needed when
> sampling from the texture, different a different mipmap is read. This reduces
> filtering artifacts because the artist can draw the smaller mipmaps separately
> and optimize the content for looking well instead of relying on strictly
> mathematical filtering algorithms. (Ever played an old game where fences,
> bricks or other recursive patterns start flickering or show other odd effects?)
Yeah, Nyquist-Shannon sampling theorem and all that stuff. I have some
literature around (Computer Graphics - Principles and Practices), so I think I
know at least the basics. :)
Oh yes, you forgot a important advantage of mipmaps: texture cache trashing ;)

> Don't worry: You don't have to care about mipmaps here. Why?
> 
> There are also cube textures. Those essentially 6 2D textures(possibly with
> mipmaps) arranged in a cubic layout. When sampling from such a texture, you sit
> in the center of the cube, and a 3D direction vector points to a texel. This is
> often used for drawing reflections.
> 
> If you do need a surface in this code
I'm not sure if I understand what you're trying to tell me here? IIRC
arb_tex_rect does only "work" with 2D targets anyway. It's not specified for
1D, 3D and cube targets, so...?

(In reply to comment #45)
> I forgot to add a few things:
> 
> The patch looks reasonable, appart of the things noted above here are a few
> tiny recommendations:
> 
> +        for(i = 0; i < MAX_FRAGMENT_SAMPLERS; ++i) {
> +            if(compile_args.texrect_fixup & (1 << i)) {
> +                sprintf(name, "PsamplerRectFixup%d", i);
> +                entry->rectFixup_location[i] =
> GL_EXTCALL(glGetUniformLocationARB(programId, name));
> +            } else
> +                entry->rectFixup_location[i] = -1;
> 
> This is not needed, glGetUniformLocationARB returns -1 if the name isn't found.
> So GL does this check for you. I personally have some problems if havin {} in
> one of case and not in the other, but that's mostly my personal opinion.
No, in fact this is very much needed. I also first had it without the
else-part, but this crashes most apps early. The thing is that
glGetUniformLocationARB is ONLY called when the bit in texrect_fixup set. So if
this bit changes from set to not-set and the above-forloop is called again it
does not overwrite the data that was previously there. The old data is still
there, which causes chaos (stateBlock->textures[i] suddenly becomes NULL or has
non-RECT type).
So if the bit is not set I have to clear rectFixup_location[i], otherwise we'll
end up with invalid data here.

> When I
> looked at the code I thought that a } is missing there
> 
> +        if(GL_TEXTURE_RECTANGLE_ARB ==
> IWineD3DBaseTexture_GetTextureDimensions(stateblock->textures[sampler]))
> +             args->texrect_fixup |= (1 << sampler);
> The line above that adds trailing whitespace
Fixed :)

> 
> +        /* trigger shader constant reloading (for texture coordinate fixup,
> when RECT samplers are used) */
> +        if(GL_TEXTURE_RECTANGLE_ARB ==
> IWineD3DBaseTexture_GetTextureDimensions(stateblock->textures[sampler]) &&
> I think, but I haven't checked, that the code around this place also uses
> GetTextureDimensions in a few cases. It may be an idea to read it once and
> store it in a local variable to avoid calling the COM method over and over.
I looked through the surrounding code in the function, but there are no more
calls to that COM method.

> Also, my unfinished sentence: If you really need a surface in the glsl_shader.c
> code, you have to use IWineD3DTexture_GetSurfaceLevel or
> IWineD33DBaseTextureImpl::surfaces[level]. I don't think you need it though
Yeah, looks like it works properly just with casting to IWineD3DTextureImpl*,
which I'm doing at the moment.
Still I have only tested the code with MP2, so there might be some more hidden
issues (going to test this later with FEAR and HL2).

> Your patch also ignores one case: When a vertex shader is used(texcoord mul
> doesn't apply), but no pixel shader is used(your new code doesn't apply). In
> this case the texrect will still be broken, but I think it is better to ignore
> this for now. It is broken now, it will be broken after your patch, so things
> don't get worse, but your patch fixes the much more common case of vertex+pixel
> shader used.
Yes, so I'm going to leave this open until I have the remaining issues fixed.

Greets,
Tobias


-- 
Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email
Do not reply to this email, post in Bugzilla using the
above URL to reply.
------- You are receiving this mail because: -------
You are watching all bug changes.