http://bugs.winehq.org/show_bug.cgi?id=14762
Summary: Max Payne 2: sepia tone filter issues (high PPE) Product: Wine Version: 1.0.0 Platform: PC-x86-64 URL: http://www.rockstargames.com/maxpayne2/mp2_downloads.htm l OS/Version: Linux Status: UNCONFIRMED Severity: normal Priority: P2 Component: directx-d3d AssignedTo: wine-bugs@winehq.org ReportedBy: liquid.acid@gmx.net CC: onety-three@gmx.net
Hi,
this is a cleaned up version of the issues first described in http://bugs.winehq.org/show_bug.cgi?id=14038.
Game affected: Max Payne 2 (demo version tested)
Issue: MP2 uses some fullscreen post-processing effects to enhance gaming experience. The famous bullet time effect is using a sepia tone filter, which breaks on wine when certain ingame options are set.
MP2 offers two modes of post-processing effect quality (switching them completly off is also possible): (i) medium -> the sepia filter works (ii) high -> sepia filter breaks
Notice that MP2 also uses some other PP (post-processing) effects, like a motion blur filter in cutscenes. The filter from the cutscenes is NOT affected by this issue described here, although it also affects the entire screen.
The issue also appears with wine-0.9.56. Issue is gone when going back to wine-0.9.45, but the rendered area is now confined in a rectangular portion of the visible screen. (read more about this at the end (*))
Regression testing turned up this commit: [d09cbcec91561186bee77aaba55b29cbf2d7e7ef] wined3d: Activate GL_ARB_texture_rectangle.
http://source.winehq.org/git/wine.git/?a=shortlog;h=d09cbcec91561186bee77aab... Notice the other five wined3d commits by Stefan Dösinger.
Somehow using ARB_tex_rect instead of using NP2 emulation breaks the sepia filter, but leaves the motion blur intact. In fact this commit does also correct "scaling" issues with the other PP effect, motion blur (again see below (*)).
My wine settings for MP2: UseGLSL=disabled OffscreenRenderingMode=backbuffer RenderTargetLockMode=auto
It doesn't matter though is ORM is set to backbuffer or fbo, the issue also is reproducible with ORM=fbo. However testing this with ORM=backbuffer makes it easier for me because of other issues with FBO (plus FBO generates a lot of FIXME noise on the console).
Graphics hardware used to confirm: (i) Me, using a Geforce FX 5900 (ii) Frank Roscher, using a Geforce FX 5700
Drivers versions: (i) FX 5900 driven with 100.14.19 and 173.14.09 (makes no difference) (ii) FX 5700 driven by driver version 169.12
(*) Interesting facts with wine-0.9.45: Like I said above PPE set to high work with this version, but the rendered area is suffering from "scaling issues". This can be reproduced all the time with the bullettime filter.
However there are also the cutscenes, using the motion blur filter. The funny thing there is that they are also affected by the scaling issue, BUT not always. Watch the first cutscene sequence after you gain control over Max in the hospital room (the cutscene begins when you are right in front of the double door). The scaling issue changes during the cutscene to correct rendering, and back after some moments. I don't know if the changes from "wrong scaling" to "correct scaling" and vice-versa are always the same but I'm going to check that again.
Greets, Tobias
http://bugs.winehq.org/show_bug.cgi?id=14762
Tobias Jakobi liquid.acid@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |stefandoesinger@gmx.at Status Whiteboard| |regression, download
http://bugs.winehq.org/show_bug.cgi?id=14762
Tobias Jakobi liquid.acid@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |jb.faq@gmx.de
http://bugs.winehq.org/show_bug.cgi?id=14762
Tobias Jakobi liquid.acid@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |alexd4@inbox.lv
http://bugs.winehq.org/show_bug.cgi?id=14762
Tobias Jakobi liquid.acid@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |hverbeet@gmail.com
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #1 from Tobias Jakobi liquid.acid@gmx.net 2008-08-04 15:48:03 --- Alexander Dorofeyev could verify this bug on his Geforce6100 (integrated) by commenting out ARB_tex_npot support in directx.c (see http://bugs.winehq.org/show_bug.cgi?id=14038#c64)
http://bugs.winehq.org/show_bug.cgi?id=14762
Austin English austinenglish@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Keywords| |download, regression Status Whiteboard|regression, download |
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #2 from Tobias Jakobi liquid.acid@gmx.net 2008-08-04 17:05:55 --- Jan Bücken can confirm this on his ATI Mobility Radeon HD2600 with latest git master. Drivers: ati-drivers-8.512 (=8.7, =8.51.3) (fglrx)
He has also disabled ARB_tex_npot in directx.c
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #3 from Alexander Dorofeyev alexd4@inbox.lv 2008-08-04 17:35:01 --- Yes it's there on geforce6100 with non-pow2 extension disabled. In backbuffer it's "bullet time filled with solid color", in fbo it's even worse in the current git, the "corridor flythrough" intro with motion blur shows total corrupted garbage and console is spammed with tons of errors including some glsl shader stuff (i have it default which is on i guess).
Disabling texture_rectangle as well in directx.c makes it render semi-correctly, even with orm=fbo, but there's is only a subrectangle rendered in bullet time.
Either way lots of:
fixme:d3d:transform_texture Non-power2 texture being used with generated texture coords
I guess what this all means is there were several versions of code none of which really worked 100% perfectly but each has its own drawbacks. For this particular game older code used to work better.
http://bugs.winehq.org/show_bug.cgi?id=14762
Jan Buecken jb.faq@gmx.de changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |NEW Ever Confirmed|0 |1
--- Comment #4 from Jan Buecken jb.faq@gmx.de 2008-08-04 18:28:52 --- *** This bug has been confirmed by popular vote. ***
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #5 from Tobias Jakobi liquid.acid@gmx.net 2008-08-04 19:03:22 --- (In reply to comment #3)
Yes it's there on geforce6100 with non-pow2 extension disabled. In backbuffer it's "bullet time filled with solid color", in fbo it's even worse in the current git, the "corridor flythrough" intro with motion blur shows total corrupted garbage and console is spammed with tons of errors including some glsl shader stuff (i have it default which is on i guess).
OK, I have GLSL disabled globally. But you might wanna take a look at: http://bugs.winehq.org/show_bug.cgi?id=14751
I can reconfirm this (nearly) black screen bug with ORM=fbo, but not with ORM=backbuffer. Maybe that's related.
Disabling texture_rectangle as well in directx.c makes it render semi-correctly, even with orm=fbo, but there's is only a subrectangle rendered in bullet time.
I think we're hitting a safe codepath here.
Either way lots of:
fixme:d3d:transform_texture Non-power2 texture being used with generated texture coords
Hmm, I only get these with mirrors enabled. But I have to check again. Can you try turning mirrors off in the ingame options?
I guess what this all means is there were several versions of code none of which really worked 100% perfectly but each has its own drawbacks. For this particular game older code used to work better.
I hope this is fixable, though :)
http://bugs.winehq.org/show_bug.cgi?id=14762
H. Verbeet hverbeet@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC|hverbeet@gmail.com |
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #6 from Tobias Jakobi liquid.acid@gmx.net 2008-10-07 15:34:26 --- Reconfirming with wine-1.1.5 and wine git master.
ORM=backbuffer, PPE=high: motion blur works, sepia doesn't ORM=fbo, PPE=high: nothing works (see bug #14751)
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #7 from Tobias Jakobi liquid.acid@gmx.net 2008-11-02 06:52:10 --- Note to me: Try dumping GLSL shader source with medium and high PPE settings and compare them.
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #8 from Tobias Jakobi liquid.acid@gmx.net 2008-11-02 12:44:44 --- Created an attachment (id=17046) --> (http://bugs.winehq.org/attachment.cgi?id=17046) WINEDEBUG=+d3dshader extract
Did two runs with WINEDEBUG=+d3d_shader and switching PP effect to medium / high.
Grepped both logs for shader_trace_init. D3D asm code with both runs is the same, so this doesn't seem to be the issue.
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #9 from Tobias Jakobi liquid.acid@gmx.net 2008-11-02 12:57:31 --- I tried to somehow verify what I suspected to be the issue.
Claim: The issue is due to incorrect texture coordinates. Maybe the ones for the quad/tris when doing the blit of the processed framebuffer image. It seems like texture coordinates are supplied in the range [0,1]x[0,1] while the texture object used is of type RECT and therefore wants them in range [0,width]x[0,height].
This is due to the lack of hw-accelerated arb_tex_npot support (only arb_tex_rect is supported without software fallback). Wine doesn't seem to correct fixup the texcoords.
The following three screenshots "verify" the claim. Explanation: 1) First shot: Notice the light switch in the upper left corner of the screen. It has a red area inside.
2) Second shot: Positioning the camera so the most upper-left pixel is a redish one. I dunno how D3D adresses the screen, but this is possibly what ends up being pixel with adress (0,0) in the framebuffer.
3) Third shot: Activating fb effect (bullettime / sepia filter). This ends up producing a screen with redish color flood-fill.
Try with other object with distinct colors. It's always the same.
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #10 from Tobias Jakobi liquid.acid@gmx.net 2008-11-02 13:00:39 --- Created an attachment (id=17047) --> (http://bugs.winehq.org/attachment.cgi?id=17047) img seq 1/3 (see comment #9 for explanation)
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #11 from Tobias Jakobi liquid.acid@gmx.net 2008-11-02 13:00:59 --- Created an attachment (id=17048) --> (http://bugs.winehq.org/attachment.cgi?id=17048) img seq 2/3 (see comment #9 for explanation)
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #12 from Tobias Jakobi liquid.acid@gmx.net 2008-11-02 13:01:17 --- Created an attachment (id=17049) --> (http://bugs.winehq.org/attachment.cgi?id=17049) img seq 3/3 (see comment #9 for explanation)
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #13 from Tobias Jakobi liquid.acid@gmx.net 2008-11-02 14:03:23 --- Additional hacking:
I figured out that with my setup the GSLS program used for the PP screen processing was always no. 12.
Checked the traces and it was revealed that program 12 was composed out of shader objects 10, 11 and 13.
Object 10 (in PPE=high mode) was reading something from a rect sampler. Compared this with the code for PPE=medium. Not a rect sampler this time, but a normal 2d one. This was the only difference in the GLSL program code.
------------------------------------------------------------
Object 10 (PPE=high): #version 120 #extension GL_ARB_texture_rectangle : enable uniform vec4 PC[8]; uniform sampler2DRect Psampler0; uniform sampler2D Psampler2; vec4 T0 = gl_TexCoord[0]; vec4 T1 = gl_TexCoord[1]; vec4 T2 = gl_TexCoord[2]; vec4 R0; vec4 tmp0; vec4 tmp1; void main() { T0.xyzw = (texture2DRect(Psampler0, T0.xy).xyzw); tmp0.x = dot(T1.xyz, T0.xyz); tmp0.y = dot(T2.xyz, T0.xyz); T2.xyzw = (texture2D(Psampler2, tmp0.xy).xyzw); R0.xyzw = (mix(T0.xyzw, T2.xyzw, PC[4].xyzw)); gl_FragColor = R0; float Fog = clamp(gl_FogFragCoord * gl_Fog.start + gl_Fog.end, 0.0, 1.0); gl_FragColor.xyz = mix(gl_Fog.color.xyz, gl_FragColor.xyz, Fog); }
------------------------------------------------------------
For the GLSL source with PPE=medium just replace sampler2DRect with sampler2D and texture2DRect with texture2D.
My current ingame resolution is 640x480. This crude hack returns proper rendering of the bullettime effect:
In shader_glsl_generate_pshader (dlls/wined3d/glsl_shader.c) add this before the shader_generate_main(...) call: if (shader_obj == 10) { shader_addline(buffer, "T0.x = T0.x * 640.0;\n"); shader_addline(buffer, "T0.y = T0.y * 480.0;\n"); }
You probably have to adjust the 10 to the id that is used on your machine. 640.0 and 480.0 are of course the ingame resolutions.
Greets, Tobias
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #14 from Tobias Jakobi liquid.acid@gmx.net 2008-11-02 17:09:01 --- Note to me: Try to understand the functions Henri told me. Look for functions (in glsl_shader.c) calling shader_glsl_get_sample_function.
List of functions where texrect has an effect: pshader_glsl_tex shader_glsl_texldl
Figure out a effective way of passing texture dimensions for fixup into the GLSL shader.
Static hardcoding would be the best in the case of Max Payne 2 since the texture dimension only changes when the ingame resolution is switched. This can't be done while the engine is running. A restart is needed.
However this doesn't look like a good generic way to do this. Need more input on this one...
Or pass tex dimensions via another texcoord, uniform, whatever. Still don't know GLSL well enough... :(
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #15 from Stefan Dösinger stefandoesinger@gmx.at 2008-11-02 17:14:19 --- You could try to implement a coordinate correction like this, but it is pretty tricky. Here are a few rules that have to be implemented:
1) No vertex or pixel shader: Correct the coords in the texture matrix 2) vertex shader, but no pixel shader: Correct the coords in the vertex shader 3) Pixel shader, vertex shader yes or no: Correct the coords in the pixel shader
(1) and (2) are needed because without a pixel shader there is no other option to fix the coordinates. OK, you may get away with it if a fixed function pipeline replacement is active, e.g. GL_ATI_fragment_shader, or GL_ARB_fragment_program, but this would be kinda ugly.
(3) is needed because pixel shaders can do arbitrary calculations in the texture coords before they sample the texture
Now the ugly part is that this adds many dependencies across different states. It ties pixel, vertex, texture, texture matrix states all together, although the are mostly independent right now. The performance implications would be not so good.
The other bad aspect is that the coordinate correction needs at least 2 single float uniforms per sampler, which are supposed to be available to the application. (even hardcoding the values into the shader won't help - the driver needs a uniform for this)
I think the best is to close this bug as WONTFIX and ignore the issue. It only occurs on one card type, the GeForce 5 series. All other cards either have native NP2 texture support, or the driver emulates it in hardware if we stick to the texture rectangle limitations.
I really hate to do that - my credo is that everything that works on Windows should work on Wine, with the same hardware, at the same feature level and the same performance. But this is one of the corner cases we can't fix.
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #16 from Tobias Jakobi liquid.acid@gmx.net 2008-11-02 18:20:33 --- (In reply to comment #15)
The other bad aspect is that the coordinate correction needs at least 2 single float uniforms per sampler, which are supposed to be available to the application. (even hardcoding the values into the shader won't help - the driver needs a uniform for this)
I wonder if this has any effect at all on the current situation. I mean the fixup code would only be needed for the 5-series. And currently situations where it's needed are not even rendered close to what it looks like in a native environment. So if after some fixup code exists the amount of uniforms is too large for the driver, we'll end up in the same situation. Scene is not rendered correctly.
Or would this have additional impacts on previously "working" apps?
I'm just referring to the uniform count. I totally agree that additional depedencies inside the state code are a really bad thing.
I think the best is to close this bug as WONTFIX and ignore the issue. It only occurs on one card type, the GeForce 5 series. All other cards either have native NP2 texture support, or the driver emulates it in hardware if we stick to the texture rectangle limitations.
What's going on with other cards, like the <=4-series? I dunno their HW caps, but AFAIK the GeForce2 already support tex_rect, but probably no real tex_npot. I realize that these cards are old, but thanks to nouveau we'll probably get some long-term support for these :)
I really hate to do that - my credo is that everything that works on Windows should work on Wine, with the same hardware, at the same feature level and the same performance. But this is one of the corner cases we can't fix.
I wonder how this is done in a native environment.
How comes the OpenGL driver implements this software fallback when on Windows it works just fine. Is this really a flaw in the hardware design or is it just the driver?
If it's a hw thing then why can't I find any information about a similar fallback in D3D?
Greets, Tobias
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #17 from Stefan Dösinger stefandoesinger@gmx.at 2008-11-02 20:25:54 ---
I wonder if this has any effect at all on the current situation. I mean the fixup code would only be needed for the 5-series. And currently situations where it's needed are not even rendered close to what it looks like in a native environment.
You definitely have a point here. It can't be any worse than broken.
Ok, one may argue that it can. Currently the rendering is incorrect. The worst case with the uniform limitation is that the app fails to run(if we report the limit), or the shader fails to compile(thus incorrect rendering, gl errors, etc)
I'm just referring to the uniform count. I totally agree that additional depedencies inside the state code are a really bad thing.
We can now build the state linking based on the GL extensions that are available. You may be able to limit the problems to GF5 cards
What's going on with other cards, like the <=4-series?
GF2 cards have no pixel shaders and no vertex shaders. We just fix up in the fixed function pipeline and are happy
GF3 and GF4 cards have vertex shader support in wine right now, so in theory we can run into the VS+FFP problem, but I haven't seen any app doing that yet. They also support pixel shaders, but only via GL_NV_texture_shader, which we don't support for pixel shaders yet. If we implement it, we have the same problem on those cards
I wonder how this is done in a native environment.
It works just fine, because in d3d conditional NP2 textures are addressed using normalized coords. This is an odd invention of GL_ARB_texture_rectangle that makes the problems here.
So here's the possible irony: We burn 2 uniforms per texture to upload the texture size, and the opengl driver probably re-normalizes the coordinates and eats another 2 uniforms. So in the end we may need a full 4 component vector to something that is a NOP in the end.
wrt noveau, if it ever goes somewhere, we can implement the fast GL_TEXTURE_2D emulation that we make use of with WINE_normalized_texrect to avoid the whole mess.
You could file a feature request at Nvidia too, it's worth a try.
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #18 from Tobias Jakobi liquid.acid@gmx.net 2008-11-24 14:39:02 --- Just a quick note: Another game is affected by this problem. FEAR 1.08 doesn't correctly render it's "bullettime mode". Also some effects are missing, like flames (attaching screenshot).
I'm resolving my other bug (http://bugs.winehq.org/show_bug.cgi?id=15875) about HL2 to duplicate, because it's without doubt the same problem.
I'm not giving up on the issue.
At least it's possible to hack/fix the issue when RECT textures are used for fullscreen PP effects. This includes the filters for MP2, the PP effects used in HL2 and also the bullettime effects. It won't work that easily for FEAR's flame effects.
@Stefan: Who would be the correct person to write a request too? I don't think that simply "complaining" about the issue through the customer service will have any effect at all.
I was thinking about speaking to Aaron Plattner about this problem, since he does seem to be in charge for the xf86-video-nv driver. IIRC he's a developer at NVIDIA, so he should have some contacts with the the developer team of the proprietary driver.
Greets, Tobias
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #19 from Tobias Jakobi liquid.acid@gmx.net 2008-11-24 14:41:00 --- *** Bug 15875 has been marked as a duplicate of this bug. ***
http://bugs.winehq.org/show_bug.cgi?id=14762
Tobias Jakobi liquid.acid@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Summary|Max Payne 2: sepia tone |GeforceFX series: fullscreen |filter issues (high PPE) |PP effect issues / RECT | |texcoord fixup
--- Comment #20 from Tobias Jakobi liquid.acid@gmx.net 2008-11-24 14:42:58 --- List of affected games so far: HL2, Max Payne 2, FEAR
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #21 from Tobias Jakobi liquid.acid@gmx.net 2008-11-24 14:44:56 --- Created an attachment (id=17433) --> (http://bugs.winehq.org/attachment.cgi?id=17433) FEAR 1.08: flame effects not rendered properly
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #22 from Tobias Jakobi liquid.acid@gmx.net 2008-11-24 17:16:49 --- Created an attachment (id=17438) --> (http://bugs.winehq.org/attachment.cgi?id=17438) Small hack to de-normalize texcoords when used in RECT samplers
This hack just adds a new constant vec2 to the GLSL preamble, containing the dimensions of the first rendertarget / backbuffer.
This is then used in pshader_glsl_tex to scale the texcoords when a RECT sampler is used.
This works for me and fixes the issues for all three games. HL2 intro sequence works, all PP effect for Max Payne 2 work and FEAR no longer has issues for it's bullettime effect plus the flame effect is also correct now.
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #23 from Stefan Dösinger stefandoesinger@gmx.at 2008-11-24 18:08:40 --- You can't use the render target size here. You have to use the texture size of the texture you're sampling from. In all your apps the texture happens to have the size size as the render target, but that isn't necessarily the case.
You can get the texture sizes from device->stateblock->texture[i]->currentDesc.(With,Height). You have to keep in mind though that the texture doesn't have to be a 2D texture(the shader code is checking that already, otherwise it won't generate a RECT read), or it could be NULL.
http://bugs.winehq.org/show_bug.cgi?id=14762
Austin English austinenglish@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Keywords| |patch
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #24 from Tobias Jakobi liquid.acid@gmx.net 2008-11-25 05:26:16 --- My current assumption is that all RECT textures that are used have the dimensions of the backbuffer. This assumption holds as long as the game is only using RECTs for fullscreen post processing effects.
You're right that this won't work (correctly) if the texture where the shader samples from doesn't have this dimensions.
However currently the most that is sampled from a RECT texture is one single pixel (since we supply texcoords in the range [0,1]x[0,1]).
This hack isn't valid in general at all, but it's kinda "better" than the current situation. And after all it's only a hack :)
@"You have to use the texture size of the texture you're sampling from.": Seems like my current approach won't work there, since I'm hardcoding the fixup vector in the shader. I don't think I can simply assume that the shader that is generated is only used for the currently bound texture (if there is anything bound at all).
So as soon as the bound texture changes I would have to regenerate the shader, modifying the fixup vector. That doesn't look like a good/clean approach to me.
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #25 from Stefan Dösinger stefandoesinger@gmx.at 2008-11-25 05:56:28 ---
Seems like my current approach won't work there, since I'm hardcoding the fixup vector in the shader. I don't think I can simply assume that the shader that is generated is only used for the currently bound texture (if there is anything bound at all).
So as soon as the bound texture changes I would have to regenerate the shader, modifying the fixup vector. That doesn't look like a good/clean approach to me.
You can use a Uniform(shader constants called in d3d. That avoids the recompile.
Note that you won't save a uniform by hardcoding the fixup. Shaders don't support immediate values in the hardware bytecode, so the driver is transparently consuming a uniform for hardcoded values on its own.
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #26 from Tobias Jakobi liquid.acid@gmx.net 2008-11-25 17:54:59 --- Yeah, thought so that this won't solve the issue with uniform consumption.
I only did choose this approach because I didn't know where wined3d passes uniforms to the shader.
I see more problems here: What if a shader program doesn't sample from a single texture but multiple ones. In that case we even consume more than a single vec2.
So we need at least 2 * nr_of_textures_sampled_from float uniforms here to make this work.
In any case the uniform state has to be updated when the texture bindings change. That looks like the hardest part to me, but probably it gets much worse than I can imagine now :(
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #27 from Tobias Jakobi liquid.acid@gmx.net 2008-11-30 17:52:43 --- Created an attachment (id=17576) --> (http://bugs.winehq.org/attachment.cgi?id=17576) IRC discussion with Stefan and Henri about the issue
Just some material for me, so I don't forget what I should do to cleanly implement a fix for this problem.
What I have understood so far: 1) Add new BOOL texrect_fixup to ps_compile_args (in wined3d_private.h) so we can device whether fixup is needed or not.
Question: Where do I "configure" / initialize the ps_compile_args structure?
2) Add code to sampler() (in state.c) to trigger fixup uniform reloading when pshaders and rect textures are used.
So I think this is the correct check: use_ps(stateblock->wineD3DDevice) && IWineD3DBaseTexture_GetTextureDimensions(stateblock->textures[sampler]) == GL_TEXTURE_RECTANGLE_ARB)
Question: What do I call then? I don't think I can simply call a method from glsl_shader.c since in the end this whole fix should also kill the issue when ARB mode is used (and not GLSL).
I suspect I should call shader_backend->load_constants there? However isn't that call kinda "heavy"? (since I "only" wanna reload the fixup uniforms).
3) For each sampler2DRect also generate a vec2 uniform containing the texture dimensions. This is probably going into shader_generate_glsl_declarations (glsl_shader.c)
Question: Currently I have put this into the texture samplers declaration, but from the discussion on IRC I assume this has to move into a separate loop.
4) Disable the (now) superfluous texcoord fixup in transform_texture() (state.c) in case pshaders are used.
What's also a mystery to me is the "register a handler for STATE_SAMPLER(0-16)" thing, which supposedly located in state.c and arb_program_shader.c. I don't think that kind of code is found somewhere in sampler() (state.c)
Does this "register"-thing mean adding stuff to the large xxx_template structures?
Sorry if I'm asking dumb question :(
Greets, Tobias
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #28 from Stefan Dösinger stefandoesinger@gmx.at 2008-12-01 10:46:03 ---
What I have understood so far:
- Add new BOOL texrect_fixup to ps_compile_args (in wined3d_private.h) so we
can device whether fixup is needed or not.
Question: Where do I "configure" / initialize the ps_compile_args structure?
In pixelshader.c, find_ps_args(), or similar
- Add code to sampler() (in state.c) to trigger fixup uniform reloading when
pshaders and rect textures are used.
So I think this is the correct check: use_ps(stateblock->wineD3DDevice) && IWineD3DBaseTexture_GetTextureDimensions(stateblock->textures[sampler]) == GL_TEXTURE_RECTANGLE_ARB)
Correct
Question: What do I call then? I don't think I can simply call a method from glsl_shader.c since in the end this whole fix should also kill the issue when ARB mode is used (and not GLSL).
Correct, you have to use the shader_backend structure
I suspect I should call shader_backend->load_constants there? However isn't that call kinda "heavy"? (since I "only" wanna reload the fixup uniforms).
shader_backend->load_constants is ok for the start. In ARB you can't avoid that because you have to prevent the normal constants from overwriting your fixup constants(or better: place the fixup constants in a place not used by the shader). In GLSL a separate function could do this, but that won't work with ARB
- For each sampler2DRect also generate a vec2 uniform containing the texture
dimensions. This is probably going into shader_generate_glsl_declarations (glsl_shader.c)
Question: Currently I have put this into the texture samplers declaration, but from the discussion on IRC I assume this has to move into a separate loop.
No, I think that place is ok. I'm not 100% sure though, put it anywhere where you feel its ok
- Disable the (now) superfluous texcoord fixup in transform_texture()
(state.c) in case pshaders are used.
Yep
What's also a mystery to me is the "register a handler for STATE_SAMPLER(0-16)" thing, which supposedly located in state.c and arb_program_shader.c. I don't think that kind of code is found somewhere in sampler() (state.c)
I was wrong there, since this is better handled in sampler() in step (2)
Does this "register"-thing mean adding stuff to the large xxx_template structures?
Yes(although you don't need it, as said above)
Sorry if I'm asking dumb question :(
No worries. The questions aren't dumb really :-)
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #29 from Tobias Jakobi liquid.acid@gmx.net 2008-12-03 10:01:50 --- Created an attachment (id=17614) --> (http://bugs.winehq.org/attachment.cgi?id=17614) [wined3d] modifications to find_ps_compile_args and ps_compile_args struct
So, something like this should for it for the ps_compile_args side?
(i) Adding the new BOOL to the struct (ii) Enable as soon as RECTs are used
Or am I missing something here?
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #30 from H. Verbeet hverbeet@gmail.com 2008-12-03 10:24:19 --- You'd have to do that for each sampler.
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #31 from Tobias Jakobi liquid.acid@gmx.net 2008-12-03 10:42:16 --- So I can't put it inside the loop?
What are "sampled samplers" anyway? Does this mean they're actually "used", or am I simply wrong here?
I thought I would suffice to check the texture dimensions for these ones...
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #32 from H. Verbeet hverbeet@gmail.com 2008-12-03 10:57:52 --- It has to be inside the loop, but you need to track per-sampler if it needs a fixup or not. sampled_samplers are the samplers that are actually sampled, yes.
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #33 from Tobias Jakobi liquid.acid@gmx.net 2008-12-03 11:03:01 --- Oh, so this implies that I also need more than a single boolean to store the fixup need, right?
Something like texrect_fixup[MAX_FRAGMENT_SAMPLERS] ?
http://bugs.winehq.org/show_bug.cgi?id=14762
Tobias Jakobi liquid.acid@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #17614|0 |1 is obsolete| |
--- Comment #34 from Tobias Jakobi liquid.acid@gmx.net 2008-12-03 11:29:31 --- Created an attachment (id=17617) --> (http://bugs.winehq.org/attachment.cgi?id=17617) [wined3d] modifications to find_ps_compile_args and ps_compile_args struct
Better?
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #35 from H. Verbeet hverbeet@gmail.com 2008-12-03 11:55:41 --- Yeah, that should work.
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #36 from Tobias Jakobi liquid.acid@gmx.net 2008-12-03 11:57:18 --- Created an attachment (id=17618) --> (http://bugs.winehq.org/attachment.cgi?id=17618) [wined3d] changes to pshader_glsl_tex (glsl_shader.c)
Next part...
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #37 from Tobias Jakobi liquid.acid@gmx.net 2008-12-03 12:26:20 --- Created an attachment (id=17619) --> (http://bugs.winehq.org/attachment.cgi?id=17619) [wined3d] changes to sampler() (state.c)
More stuff, again not tested (going to do this later).
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #38 from Stefan Dösinger stefandoesinger@gmx.at 2008-12-03 12:39:52 --- In theory your changes look good, I can't promise that it will work though. The GLSL shader part is incomplete(PsamplerRectFixup is not declared anywhere, and isn't loaded), and in state.c you're not taking care of the texture matrix side fixup yet. But all in all it moves into a reasonable direction.
by the way, I think you can ignore vertex samplers. I do not know any card that supports them, but doesn't support unconditional NP2 textures. (Geforce 6,7 support NP2, ati radeon <= X1999 do not support vertex samplers*, and I think the GF5 doesn't support vertex samplers either)
(*) Actually, they support them, but you can only use 0 of them concurrently. That's ATIs dirty trick to get Shader Model 3.0 support certified(where vertex samplers are mandatory), even though their cards don't support them.
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #39 from Tobias Jakobi liquid.acid@gmx.net 2008-12-03 17:38:35 --- (In reply to comment #38)
In theory your changes look good, I can't promise that it will work though. The GLSL shader part is incomplete(PsamplerRectFixup is not declared anywhere, and isn't loaded), and in state.c you're not taking care of the texture matrix side fixup yet. But all in all it moves into a reasonable direction.
Yes, I should have mentioned that the patch(set) isn't complete yet. Still working on the rest though. And I also haven't made a compile yet, it's just implementing what you told me. If it works.... well, we'll see about that in the next week maybe.
by the way, I think you can ignore vertex samplers. I do not know any card that supports them, but doesn't support unconditional NP2 textures. (Geforce 6,7 support NP2, ati radeon <= X1999 do not support vertex samplers*, and I think the GF5 doesn't support vertex samplers either)
OK, but this only concerns the comment above the bitfield, right?
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #40 from Tobias Jakobi liquid.acid@gmx.net 2008-12-04 11:18:15 --- Created an attachment (id=17634) --> (http://bugs.winehq.org/attachment.cgi?id=17634) [wined3d] even more changes to glsl_shader.c
I think I need a bit of comment on this one.
Doesn't still contain the texture matrix fixup stuff, and isn't even compile tested. I'd like to know if the changes are basically correct.
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #41 from H. Verbeet hverbeet@gmail.com 2008-12-04 12:41:08 ---
if(prog->rectFixup_location[i]) {
You need to test against -1 instead of 0 there. For the rest the changes look correct. You need to actually use the fixup as well of course, which will probably be a bit more interesting.
http://bugs.winehq.org/show_bug.cgi?id=14762
Tobias Jakobi liquid.acid@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #17438|0 |1 is obsolete| | Attachment #17617|0 |1 is obsolete| | Attachment #17618|0 |1 is obsolete| | Attachment #17619|0 |1 is obsolete| | Attachment #17634|0 |1 is obsolete| |
--- Comment #42 from Tobias Jakobi liquid.acid@gmx.net 2008-12-04 18:06:40 --- Created an attachment (id=17638) --> (http://bugs.winehq.org/attachment.cgi?id=17638) [wined3d] geforcefx rect patchset (incomplete)
Still incomplete patchset for the issue.
I think I need more help here. The problem is that it "kinda" works.
What means that as long as I "hardcode" tex_dim to {800.0, 600.0} in shader_glsl_load_constants it works. Effects rendered fine, no GL errors, compilation warningless, etc.
However... const float tex_dim[2] = {surf->currentDesc.Width, surf->currentDesc.Height};
This does not seem to produce the results I would like to have.
That's what is stored there: dims: 0.000000 , 1.000000
This surely can't be the backbuffer dimensions... I think I just can't do the case there, and end up getting wrong data (from a wrong memory adress) - but not so sure about that.
Any ideas?
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #43 from Stefan Dösinger stefandoesinger@gmx.at 2008-12-04 18:14: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.
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
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #44 from Stefan Dösinger stefandoesinger@gmx.at 2008-12-04 18:42:41 --- 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).
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.
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?)
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
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #45 from Stefan Dösinger stefandoesinger@gmx.at 2008-12-04 19:20:58 --- 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. 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
+ /* 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.
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
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.
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #46 from Tobias Jakobi liquid.acid@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));
} elseentry->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
http://bugs.winehq.org/show_bug.cgi?id=14762
Tobias Jakobi liquid.acid@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #17638|0 |1 is obsolete| |
--- Comment #47 from Tobias Jakobi liquid.acid@gmx.net 2008-12-05 03:19:00 --- Created an attachment (id=17650) --> (http://bugs.winehq.org/attachment.cgi?id=17650) [wined3d] geforcefx rect patchset (incomplete)
Next revision
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #48 from Tobias Jakobi liquid.acid@gmx.net 2008-12-05 04:49:52 --- Just tested this with HL2 and it really breaks the game.
I get both GL errors and visual glitches. Still trying to figure out what does wrong there.
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #49 from Stefan Dösinger stefandoesinger@gmx.at 2008-12-05 05:05:24 --- (In reply to comment #46)
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
It is in the BaseTextureClass structure: IWineD3DTextureImpl->baseTexture.np2Matrix. A IWineD3DBaseTextureImpl * is ok here too in fact. Its our hacky way of setting up inheritance without a C++ compiler
The NP2 fixup matrix is in all 3 texture types because we also have NP2 emulation with cube and volume textures. E.g. a 200x200x200 cube texture is scaled up to 256x256x256. However, I think we can drop this code because Windows D3D does conditional NP2 textures only with 2D textures. So you don't have to care about cube and volume textures here.
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?
Correct. It is set when creating the texture.
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...?
I just wanted to elaborate why a texture structure cannot be a surface, even though they seem very related in the 2D case. But since you know the 3D graphics stuff I can stop this lecturing :-)
No, in fact this is very much needed. I also first had it without the else-part, but this crashes most apps early.
I thought about dropping the if entirely and call GetUniform unconditionally, but maybe I am missing something here. Having the if doesn't really hurt though, since that code shouldn't be performance critical.
http://bugs.winehq.org/show_bug.cgi?id=14762
Tobias Jakobi liquid.acid@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #17650|0 |1 is obsolete| |
--- Comment #50 from Tobias Jakobi liquid.acid@gmx.net 2009-01-24 18:28:14 --- Created an attachment (id=18971) --> (http://bugs.winehq.org/attachment.cgi?id=18971) geforcefx rect patchset (incomplete)
Hi there,
sorry - I didn't have lot of time lately to work on any patches. Now that my oral exam is over I did return to this patch and rebased it against current git master.
Seems like the code works flawlessly now. I have correct rendering behaviour now in Max Payne 2 as well as in HL2. Also no new error messages or rendering artifacts popping up in HL2 as it was the case when I tried the patch previously.
Still have to try FEAR though. However it's great to see that during my absence wine's code has changed so much that my code works perfectly now :D
Current issues with the patch: - only works in GLSL mode, lacks code for ARB mode - only works when pixelshaders are used, it doesn't provide code for a vertexshader-only usage environment
@Stefan: Could you take a(nother) look at the patch and give me your impressions about the quality? Is it ready to be pushed to wine-patches even with the two remaining issues? And do you see any additional problems?
I plan to solve the issue also for ARB mode, but probably it's a lot harder there since I can't use high-level shading syntax.
Greets, Tobias
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #51 from Tobias Jakobi liquid.acid@gmx.net 2009-01-25 10:39:10 --- Created an attachment (id=18987) --> (http://bugs.winehq.org/attachment.cgi?id=18987) hack to get this working in arb mode
Tried to fiddle around a bit with the arb code. Hacked this together. It's only working with fixed fb dimensions since I haven't figured out yet how to pass params inside arbfp programs.
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #52 from Tobias Jakobi liquid.acid@gmx.net 2009-01-31 18:20:13 --- Hi there,
haven't yet managed to produce any working code for ARB mode, but I think I'm getting closer.
Like Stefan told me I should do the constant loading for texrect fixup similar to that of the bump env matrix.
What I have figured out so far:
(1) I have to change shader_arb_load_constants, checking there if there are any rect textures used in the shader and if yes, also check if fixup is possible (maybe we were already out of constants). If both checks pass I'll proceed with glProgramEnvParameter2fv to push the texture dimensions into the program.
PROBLEM: What's the easiest way to get access to the ps_compile_args structure inside shader_arb_load_constants. Wasn't a big thing with GLSL, but I'm not really sure which structure that is available from inside the function leads me to ps_compile_args.
(2) Change shader_generate_arb_declaration: Similar to bump env matrix loading I carefully check if we're not already out of free constants. If there are still constants free I proceed to add "PARAM textureRectFixup..." lines into the shader code.
Stefan already told me that all data inside an ARB program is of type vec4, so I should probably come up with a way to not burn a full vec4 PARAM for only one texture. So think about a good way of storing two fixup vec2s inside one PARAM.
(3) Change shader_hw_sample: Check whether we're sampling from RECT and if constants for this RECT could be loaded. If yes, then scale coords and so on.
(4) Not sure about this: gen_arbfp_ffp_shader. Since it contains texture sampling instructions that can encounter RECT types, it should probably be changed as well, right?
Upon finishing the fixup thing with ARB I'm going to move on to also fixing this for vertexshader-only setups (Stefan told me before that this could also be the case, and currently the whole fixup thing only works when pixelshaders are used).
Greets, Tobias
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #53 from Stefan Dösinger stefandoesinger@gmx.at 2009-02-01 17:24:13 ---
PROBLEM: What's the easiest way to get access to the ps_compile_args structure inside shader_arb_load_constants. Wasn't a big thing with GLSL, but I'm not really sure which structure that is available from inside the function leads me to ps_compile_args.
I've no quick idea. The idea behind the compile_args struct is to be used only during compilation, so the answer is probably you don't access it(neither in GLSL, although there it is easy to use it). Probably fetch some values from the stateblock, and/or shadow them in the compiled_shader structure.
(actually, you may access the compile_args struct in a compiled_shader member, but don't try to access the same pointer that is passed to compile_shader)
(4) Not sure about this: gen_arbfp_ffp_shader. Since it contains texture sampling instructions that can encounter RECT types, it should probably be changed as well, right?
You don't have to do anything there necessarily. The vertex pipeline is supposed to take care of this if no d3d pixel shader is used.
However, you could add some fixup code there too instead of fixing the coords in the vertex shader. Note that for fully correct operation you still need to be able to handle the fixups in the vertex shader, but keeping it in the pixel shader and fp ffp replacement may be easier and keep the state management simpler. If it keeps the code simpler I'd be willing to say that an ffp replacement is needed for proper RECT operation with vertex shaders(all cards that have vshaders have either atifs, arbfp or nvts) The question is how this affects performance. We're adding extra instructions and probably a dependent read to the fragment processing, but avoid interdependencies between vertex and fragment pipeline.
I also don't know if we can handle the RECT fixup in atifs or nvts. They only have 8 instructions and 8 constants, and may not even be able to perform dependent reads on RECT textures.
http://bugs.winehq.org/show_bug.cgi?id=14762
Tobias Jakobi liquid.acid@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #18971|0 |1 is obsolete| |
http://bugs.winehq.org/show_bug.cgi?id=14762
Tobias Jakobi liquid.acid@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #18987|0 |1 is obsolete| |
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #54 from Tobias Jakobi liquid.acid@gmx.net 2009-02-01 18:09:33 --- Created an attachment (id=19177) --> (http://bugs.winehq.org/attachment.cgi?id=19177) [01] [wined3d] add glGetProgram{Env,Local}ParameterfvARB to wined3d_gl header
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #55 from Tobias Jakobi liquid.acid@gmx.net 2009-02-01 18:09:54 --- Created an attachment (id=19178) --> (http://bugs.winehq.org/attachment.cgi?id=19178) [02] [wined3d] add texrect_fixup bitfield to ps_compile_args structure
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #56 from Tobias Jakobi liquid.acid@gmx.net 2009-02-01 18:10:10 --- Created an attachment (id=19179) --> (http://bugs.winehq.org/attachment.cgi?id=19179) [03] [wined3d] initialize new texrect_fixup bitfield properly in pixelshader.c
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #57 from Tobias Jakobi liquid.acid@gmx.net 2009-02-01 18:10:27 --- Created an attachment (id=19180) --> (http://bugs.winehq.org/attachment.cgi?id=19180) [04] [wined3d] implement texrect coord fixup in GLSL mode
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #58 from Tobias Jakobi liquid.acid@gmx.net 2009-02-01 18:10:44 --- Created an attachment (id=19181) --> (http://bugs.winehq.org/attachment.cgi?id=19181) [05] [wined3d] trigger constant reloading when rect textures change
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #59 from Tobias Jakobi liquid.acid@gmx.net 2009-02-01 18:10:58 --- Created an attachment (id=19182) --> (http://bugs.winehq.org/attachment.cgi?id=19182) [06] [wined3d] add texrect fixup structures to shader_arb_priv
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #60 from Tobias Jakobi liquid.acid@gmx.net 2009-02-01 18:11:13 --- Created an attachment (id=19183) --> (http://bugs.winehq.org/attachment.cgi?id=19183) [07] [wined3d] implement texrect coord fixup in ARB mode
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #61 from Tobias Jakobi liquid.acid@gmx.net 2009-02-01 18:11:31 --- Created an attachment (id=19184) --> (http://bugs.winehq.org/attachment.cgi?id=19184) [08] [wined3d] various constifications in arb_program_shader.c
http://bugs.winehq.org/show_bug.cgi?id=14762
Tobias Jakobi liquid.acid@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #19177|0 |1 is obsolete| | Attachment #19178|0 |1 is obsolete| | Attachment #19179|0 |1 is obsolete| | Attachment #19180|0 |1 is obsolete| | Attachment #19181|0 |1 is obsolete| | Attachment #19182|0 |1 is obsolete| | Attachment #19183|0 |1 is obsolete| | Attachment #19184|0 |1 is obsolete| |
--- Comment #62 from Tobias Jakobi liquid.acid@gmx.net 2009-03-05 16:18:35 --- Created an attachment (id=19811) --> (http://bugs.winehq.org/attachment.cgi?id=19811) the patches (this time in an archive)
should now again build against current git master
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #63 from Tobias Jakobi liquid.acid@gmx.net 2009-03-06 18:00:06 --- @stefand: Could you maybe take a look at the current patchset? I think at least the GLSL part could go into git
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #64 from Tobias Jakobi liquid.acid@gmx.net 2009-03-28 15:11:34 --- Created an attachment (id=20170) --> (http://bugs.winehq.org/attachment.cgi?id=20170) New IRC discussion log (this time about ARB impl)
The np2 fixup works really good with GLSL currently, but ARB is still a major problem (which I want to fix before moving on to a faster card *g*).
This here is the IRC discussion with stefand about the problem. It's mainly a hint for me how to do it correctly.
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #65 from H. Verbeet hverbeet@gmail.com 2009-03-29 07:01:33 --- Actually, wrt. constant reloading, the way to do that in the long term would be to introduce the concept of constant buffers (d3d10) to the backends. Our internal fixups would be in their own buffer separate from application constants in that case. That's not completely trivial to implement though.
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #66 from Tobias Jakobi liquid.acid@gmx.net 2009-04-08 21:09:09 --- I looked into what Stefan told me, and I don't think it's going to work.
Quote from the ARB_fragment_program docs:
The error INVALID_VALUE is generated by commands ProgramEnvParameter{fd}ARB, ProgramEnvParameter{fd}vARB, and GetProgramEnvParameter{fd}vARB if <index> is greater than or equal to the value of MAX_PROGRAM_ENV_PARAMETERS_ARB corresponding to the program target <target>.
Like Stefan mentioned MAX_PROGRAM_ENV_PARAMETERS_ARB is usually 256.
Even if it would work to put something like "PARAM ThisIsMyPrivateThing = {programs.env[256 + 1]};" into a fragment/vertex program, there is no way to reliably fill the PARAM with data.
The docs clearly state that this would trigger an error. I'm very much against exploiting some driver functionality that may be there (I haven't actually tested what Stefan told me), but is potentially unstable and only exists for some drivers / some GPUs.
Or am I misunderstanding something here??
http://bugs.winehq.org/show_bug.cgi?id=14762
Tobias Jakobi liquid.acid@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #19811|0 |1 is obsolete| |
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #67 from Tobias Jakobi liquid.acid@gmx.net 2009-04-09 15:47:00 --- Created an attachment (id=20353) --> (http://bugs.winehq.org/attachment.cgi?id=20353) [wined3d] ARB np2fixup patchset (beta 2)
This is beta2 of the np2fixup patchset for ARB mode.
It still doesn't work with HL2 though. The implementation should be pretty much correct, but np2 constant reloading in ARB doesn't seem to work the way I want.
For example the correct values are loaded into the env PARAMs, but they don't seems to reach the shader. Something probably does overwrite them before the shader code is executed. Don't know why though. Or what could overwrite them.
This could possibly be solved by switching to local PARAMs, but this has to wait. stefand doesn't want to have this code merged with the current state of the ARB codebase.
Again, the code should pretty much work - but if you want it to work properly you have to figure out why the env params get overwritten before the shader can use it.
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #68 from Tobias Jakobi liquid.acid@gmx.net 2009-04-09 18:59:46 --- Another note for me: Port over the param/uniform "squeezing" code from ARB to GLSL. According to stefand the GLSL compilers don't seem very bright when it comes to efficiently packing vec2 uniforms. Looks like a vec2 uniform is just mapped to a vec4, essentially wasting a lot of space.
Since the squeezing code should both be implemented in GLSL and ARB I might introduce some patches from the ARB patchset, since they add the data-structures needed for the squeeze.
http://bugs.winehq.org/show_bug.cgi?id=14762
Tobias Jakobi liquid.acid@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #20353|0 |1 is obsolete| |
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #69 from Tobias Jakobi liquid.acid@gmx.net 2009-04-18 14:09:43 --- http://download.nvidia.com/developer/Papers/2005/OpenGL_2.0/NVIDIA_OpenGL_2....
According to this the NV30 (GeforceFX) series doesn't support vertex texture fetching in hardware, so a fixup for NP2 textures won't be necessary here.
I have the constant packing code for GLSL ready (going to submit this once Henri's patches are in, since I don't want too much collision) and also a working ARB patchset. The only thing missing is NP2 fixup with a vertexshader-only configuration. I'm only going to implement this for GLSL, since it's probably going to get ugly with ARB. Once this is done, the bug will be closed (doesn't make much sense to implement more corner cases which won't ever appear on the FX).
http://bugs.winehq.org/show_bug.cgi?id=14762
--- Comment #70 from Tobias Jakobi liquid.acid@gmx.net 2009-07-04 06:13:19 --- Short status update: (i) The constant packing code for GLSL is in git master, this should help with applications that use a lot of NP2 textures - however most apps don't do that and the FX won't run them at acceptable framerate anyway. The code still needs fallback implementation, which means that it should disable the fixup if the shader would fail because of too many uniforms (shader constants in GLSL) used. This isn't done currently since the used uniforms are counted nowhere.
(ii) NP2 fixup code for ARB also went into git master - this already implements constant packing and a fallback. Fallback was done because unsuccessfully compiled shaders generate a lot of GL error noise when wine attempts to draw with them. So we just disable the fixup once we run out of constants, peroid. This isn't going to look that good but at least it "works" to some extent. However I have yet to see a game that triggers the fallback.
(iii) All NP2 fixup code is currently exclusive to pixelshader mode, it won't kick in when there is only a vertexshader active. A GLSL implementation for vshader-only mode is planned - ARB.... maybe, but not very likely.
(iv) A testcase is still missing. Going to write one soon....
http://bugs.winehq.org/show_bug.cgi?id=14762
Henri Verbeet hverbeet@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Regression SHA1| |d09cbcec91561186bee77aaba55 | |b29cbf2d7e7ef
http://bugs.winehq.org/show_bug.cgi?id=14762
Stefan Dösinger stefandoesinger@gmx.at changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |RESOLVED Resolution| |FIXED
--- Comment #71 from Stefan Dösinger stefandoesinger@gmx.at 2011-08-18 18:03:24 CDT --- I'll resolve this bug, RECT fixup with shaders is basically there. Any further problems should be reported as new bugs. I think the main remaining problem is making sure that this functionality doesn't break now that most GPUs out there have proper unconditional non power of two support.
http://bugs.winehq.org/show_bug.cgi?id=14762
Alexandre Julliard julliard@winehq.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED
--- Comment #72 from Alexandre Julliard julliard@winehq.org 2011-08-26 13:23:43 CDT --- Closing bugs fixed in 1.3.27.
-
wine-bugs＠winehq.org