http://bugs.winehq.org/show_bug.cgi?id=9775
Summary: Max Payne 1.05 - Screenshot fails Product: Wine Version: 0.9.45. Platform: PC-x86-64 OS/Version: Linux Status: UNCONFIRMED Severity: normal Priority: P2 Component: wine-directx-d3d AssignedTo: wine-bugs@winehq.org ReportedBy: liquid.acid@gmx.net
The MaxPayne.exe can be called with the "-screenshot" parameter. This enables an integrated screenshot functionality. You can then take in-game screenies when pressing the F10 key. Shots are saved in the screenshots directory inside the Max Payne directory. This works without problems in Windows.
Running Max Payne on wine results in completly black TGA screenshot file.
http://bugs.winehq.org/show_bug.cgi?id=9775
Vitaliy Margolen vitaliy@kievinfo.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Severity|normal |trivial
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #1 from Tobias Jakobi liquid.acid@gmx.net 2007-09-28 15:47:15 --- It's not only screenshots that fail.
The savegame function stores an image of the current game situation when creating a savegame. When switching levels an autosave is generated with the corresponding 2D comic image in it.
Now the snapshots when saving "in-game" are also completly black. Only the automatic saves between levels (when only 2D is displayed) are correct.
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #2 from Tobias Jakobi liquid.acid@gmx.net 2007-11-14 17:41:43 --- Confirming this with wine-0.9.49 - screenshots of 3D material are always completly black
http://bugs.winehq.org/show_bug.cgi?id=9775
Tobias Jakobi liquid.acid@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |liquid.acid@gmx.net
--- Comment #3 from Tobias Jakobi liquid.acid@gmx.net 2008-02-10 18:18:27 --- Rechecked the bug with wine version 0.9.54 and it still exists.
Attaching a log of the console output during running the game...
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #4 from Tobias Jakobi liquid.acid@gmx.net 2008-02-10 18:22:51 --- Created an attachment (id=10711) --> (http://bugs.winehq.org/attachment.cgi?id=10711) console output during running the MaxPayne.exe
I'm creating a quicksave in the game, so this should produce a picture in the load game menu, but it doesn't.
I think the fixme:d3d_surface:flush_to_framebuffer_drawpixels >>>>>>>>>>>>>>>>> GL_INVALID_VALUE (0x501) from glDrawPixels @ surface.c / 1060 line is worth taking a look, but also fixme:d3d_surface:IWineD3DSurfaceImpl_LoadTexture (0x1eac310) Operation not supported for scratch textures looks suspicious to me.
Greets, Tobias
http://bugs.winehq.org/show_bug.cgi?id=9775
Myk Taylor myk002@yahoo.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |NEW Ever Confirmed|0 |1
--- Comment #5 from Myk Taylor myk002@yahoo.com 2008-02-25 03:05:47 --- *** This bug has been confirmed by popular vote. ***
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #6 from Tobias Jakobi liquid.acid@gmx.net 2008-03-09 07:24:28 --- Reconfirming this with wine-0.9.57, attaching another log...
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #7 from Tobias Jakobi liquid.acid@gmx.net 2008-03-09 07:25:39 --- Created an attachment (id=11259) --> (http://bugs.winehq.org/attachment.cgi?id=11259) MaxPayne.exe output when started with wine-0.9.57
Starting the game, going in-game and doing a quicksave, sadly no screenshot appear in the load game section. Exiting game...
http://bugs.winehq.org/show_bug.cgi?id=9775
Alexander Dorofeyev alexd4@inbox.lv changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |alexd4@inbox.lv
--- Comment #8 from Alexander Dorofeyev alexd4@inbox.lv 2008-03-15 11:16:09 --- This is a duplicate of bug # 7801 I believe.
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #9 from Tobias Jakobi liquid.acid@gmx.net 2008-03-15 12:00:07 --- Looks like that. I can confirm that the problem I encountered also appears with wine-0.9.33. Back when I found the problem I checked if it was a regression and went back to wine-0.9.16, no version does produce screenshots.
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #10 from Tobias Jakobi liquid.acid@gmx.net 2008-03-15 12:00:59 --- Additional spam: However this other bug is about Max Payne 2, I think we should keep this separate.
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #11 from Tobias Jakobi liquid.acid@gmx.net 2008-06-06 17:54:40 --- Reconfirming with wine-1.0_rc3
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #12 from Tobias Jakobi liquid.acid@gmx.net 2008-06-12 13:26:02 --- Hi there,
I think I have found a fix for the problem.
MSDN documentation states that a call to the D3D8 method CreateImageSurface uses a "internal" D3DPOOL_SCRATCH flag.
Read here: http://msdn.microsoft.com/en-us/library/bb204851(VS.85).aspx (in "CreateImageSurface Changes")
Also see http://msdn.microsoft.com/en-us/library/bb174358(VS.85).aspx
But I also found this website stating that the MSDN is erroneous here: http://www.xmission.com/~legalize/book/dx9-bugs.html
I tested this my modifying dlls/d3d8/device.c: In IDirect3DDevice8Impl_CreateImageSurface I changed the pool parameter from D3DPOOL_SCRATCH to D3DPOOL_SYSTEMMEM.
This seems to fix the problem with the empty sreenshots. Game scenes are now displayed in the savegame loading menu.
Can someone (Alexander?) take a look at this "fix"? I'm really no expert in D3D so I'm absolutely not sure if that's correct at all. At least it fixes the problem here, I don't know about Max Payne 2 though.
Cheers, Tobias
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #13 from Tobias Jakobi liquid.acid@gmx.net 2008-06-13 07:40:20 --- Another piece of evidence that IDirect3DDevice8::CreateImageSurface is not using D3DPOOL_SCRATCH.
I finally found a copy of the DX8.1 SDK on the net and from the docs (quote):
Remarks: ... Image surfaces are placed in the D3DPOOL_SYSTEMMEM memory class.
I tried writing a test for this, but I'm not really sure how to do this. First problem: That's my first time writing a Direct3D application... Second problem: What calls are I'm supposed to throw at the surface created by the CreateImageSurface call so I can be sure it's in the SYSTEMMEM memory pool?
Stefan Doesinger mentioned some like CopyRects, but according to the DX8.1 docs the surface should support that. Also it should support locking, but nothing more.
I was more thinking about using GetDesc on the surface and then checking the returned struct for the pool?! Any advice on this one?
Greets, Tobias Jakobi
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #14 from Alexander Dorofeyev alexd4@inbox.lv 2008-06-13 10:06:45 --- (In reply to comment #13)
I was more thinking about using GetDesc on the surface and then checking the returned struct for the pool?! Any advice on this one?
I don't really know too much about d3d8 specifics, but GetDesc sounds like a good way.
About how to write - I guess you should find appropriate file in d3d8 tests (probably d3d8/tests/surface.c). Easiest is duplicate an existing test and modify it for your purposes.
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #15 from Tobias Jakobi liquid.acid@gmx.net 2008-06-14 17:10:32 --- I managed to write some simple test for the behaviour. That turned out not to be the major problem.
Compiling a executable for windows turned out to be a real pain in the butt! Took me the entire saturday to setup a working compile environment in my win2k vm.
Anyway, I patched up some EXE with vctoolkit 2005, the ms platform sdk from 2003 and the original dx 8.1 sdk. My flat mate tested it on his xp machine and the test passes there, including the newly added one. On wine it doesn't pass. Still have to give the EXE to my friend with the vista machine.
Cheers, Tobias
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #16 from Tobias Jakobi liquid.acid@gmx.net 2008-06-14 17:12:25 --- Created an attachment (id=14034) --> (http://bugs.winehq.org/attachment.cgi?id=14034) modified dlls/d3d8/test/surface.c source file
includes a new test_image_surface_pool test and some comment about the problem with the different statements in the dx8 and dx9 sdk
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #17 from Alexander Dorofeyev alexd4@inbox.lv 2008-06-14 17:53:02 --- Hmm. Are you aware you can build test executables for Windows under Linux? For ddraw and d3d tests at least it works great. If Wine's configure detects presence of mingw, then say "cd dlls/d3d8/tests; make crosstest" gives you a nice individual dll test executable (d3d8_crosstest.exe).
"The Direct3D9 documentation incorrectly states that CreateImageSurface returns a surface object with D3DPOOL_SCRATCH configuration."
Maybe I'm missing something, but this reasoning seems a bit suspect. Do you know for sure that D3D9 docs really are wrong, or maybe there just is a differences between d3d8 and d3d9 in this thing? It would be ideal to add a similar test for d3d9. Then you can change pool parameter in d3d8 and submit the patch with this stuff. Since it's simple, clear and tested and fixes a bug it should be accepted. Nice research :).
Oh, one note about the test though, in case of test failures it's best to print all relevant values, not just state what is implied by failure anyway. E.g. if you test surf_desc.Pool == D3DPOOL_SYSTEMMEM, then it's best to print something like "surf_desc.Pool was %u instead of D3DPOOL_SYSTEMMEM" or "unexpected memory pool: %u". It's recommended in test writing docs IIRC, and it makes debugging test failures (if they happen) easier to debug.
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #18 from Tobias Jakobi liquid.acid@gmx.net 2008-06-14 20:39:18 --- (In reply to comment #17)
Hmm. Are you aware you can build test executables for Windows under Linux? For ddraw and d3d tests at least it works great. If Wine's configure detects presence of mingw, then say "cd dlls/d3d8/tests; make crosstest" gives you a nice individual dll test executable (d3d8_crosstest.exe).
Yep, I know about crosscompiling. That's the first thing I tried. But somehow it didn't work really well (maybe because the host system is 64bit one?). I didn't investigate this further and tried next a mingw installation on my vm. Turned out that mingw wasn't really happy with the DX8.1 sdk, or vice versa? Next I tried vctoolkit and failed at installation. The 2008 version has win 2k support dropped, so I had to get the 2005 version, which works now. The recent platform SDK also doesn't work with w2k, but the 2003 version does the trick
I'm going to investigate the crosscompile problem if I find some free time. Currently the vctoolkit setup on my vm works for me, so at least I have possibility to build windows EXEs.
"The Direct3D9 documentation incorrectly states that CreateImageSurface returns a surface object with D3DPOOL_SCRATCH configuration."
Maybe I'm missing something, but this reasoning seems a bit suspect.Do you know for sure that D3D9 docs really are wrong, or maybe there just is a differences between d3d8 and d3d9 in this thing?
Direct3D9 doesn't even have a CreateImageSurface method anymore. According to the MSDN docs the method was renamed to CreateOffscreenPlainSurface. Again that's not entirely correct. The method isn't simply renamed, they also added the pool type parameter, which was somehow "hardcoded" in CreateImageSurface.
The wrong statement is "D3DPOOL_SCRATCH will return a surface that has identical characteristics to a surface created by IDirect3DDevice8::CreateImageSurface". That can't be correct. The D3DPOOL_X flag meaning hasn't change between DX8 and DX9. D3DPOOL_SYSTEMMEM hasn't miraculously become D3DPOOL_SCRATCH. DX8.1 sdk docs states that the surface pool type returned by CreateImageSurface is D3DPOOL_SCRATCH. Tests on WinXP verify this behaviour. D3D9 can't change that behaviour because D3DPOOL_SCRATCH and D3DPOOL_SYSTEMMEM are not "compatible".
It would be ideal to add a
similar test for d3d9. Then you can change pool parameter in d3d8 and submit the patch with this stuff. Since it's simple, clear and tested and fixes a bug it should be accepted. Nice research :).
Nothing to test then for D3D9 I think. :-)
Oh, one note about the test though, in case of test failures it's best to print all relevant values, not just state what is implied by failure anyway. E.g. if you test surf_desc.Pool == D3DPOOL_SYSTEMMEM, then it's best to print something like "surf_desc.Pool was %u instead of D3DPOOL_SYSTEMMEM" or "unexpected memory pool: %u". It's recommended in test writing docs IIRC, and it makes debugging test failures (if they happen) easier to debug.
Gonna change that!
Greets, Tobias
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #19 from Tobias Jakobi liquid.acid@gmx.net 2008-06-19 15:08:13 --- Created an attachment (id=14192) --> (http://bugs.winehq.org/attachment.cgi?id=14192) test_image_surface_pool
Rewritten test according to advices from Alexander.
Currently the test goes into surface.c, but device.c should be more correct (since CreateImageSurface is a device method. I didn't put it into device.c because surface.c already does all d3d init stuff for me and just hands me the proper setup device.
The test itself should be correct now. It fails in wine and passes on windows xp. Vista still needs a test.
Cheers, Tobias
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #20 from Alexander Dorofeyev alexd4@inbox.lv 2008-06-19 17:15:41 --- (In reply to comment #19)
Created an attachment (id=14192)
--> (http://bugs.winehq.org/attachment.cgi?id=14192) [details]
test_image_surface_pool
Rewritten test according to advices from Alexander.
Currently the test goes into surface.c, but device.c should be more correct (since CreateImageSurface is a device method. I didn't put it into device.c because surface.c already does all d3d init stuff for me and just hands me the proper setup device.
The test itself should be correct now. It fails in wine and passes on windows xp. Vista still needs a test.
I think surface.c is ok for this test. Sure it's created by a device method, but any surface or texture can only be created by methods of device and all tests in surface.c use one or another device method. Important that it tests properties of a surface.
If you remove // comments and change CreateImageSurface to do the right thing, it should be ok to submit to wine-patches IMO (patches are generated by git-diff or git-format-patch, see http://www.winehq.org/site/sending_patches). Test passes XP + fixes a game should be enough.
http://bugs.winehq.org/show_bug.cgi?id=9775
Alexander Dorofeyev alexd4@inbox.lv changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |hoehle@users.sourceforge.net
--- Comment #21 from Alexander Dorofeyev alexd4@inbox.lv 2008-07-01 09:45:04 --- *** Bug 12348 has been marked as a duplicate of this bug. ***
http://bugs.winehq.org/show_bug.cgi?id=9775
Alexander Dorofeyev alexd4@inbox.lv changed:
What |Removed |Added ---------------------------------------------------------------------------- Summary|Max Payne 1.05 - Screenshot |Syberia 2, Max Payne 1 & 2 - |fails |Screenshots and thumbnails | |broken
--- Comment #22 from Alexander Dorofeyev alexd4@inbox.lv 2008-07-01 09:59:57 --- Updating summary to include some other affected games.
Tobias, looks like this patch wasn't accepted yet? If the length of comments and msdn links are controversial, I suggest trimming them for brevity or maybe avoiding altogether. msdn links are mostly avoided afaik because they change all the time, besides, it's best to shortly describe what happens in your own words and/or write a test, since msdn is misleading quite often. Since there is a test, there shouldn't be much questions, it may be ok even without comments at all or maybe with "... creates surfaces in D3DPOOL_SYSTEM; games ... rely on it" or similar one liner. And wrong comment in d3d9 can simply be removed, maybe in a separate patch.
http://bugs.winehq.org/show_bug.cgi?id=9775
Alexander Dorofeyev alexd4@inbox.lv changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |mahasler@gmail.com
--- Comment #23 from Alexander Dorofeyev alexd4@inbox.lv 2008-07-01 10:02:08 --- *** Bug 7801 has been marked as a duplicate of this bug. ***
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #24 from Tobias Jakobi liquid.acid@gmx.net 2008-07-01 10:54:59 --- (In reply to comment #22)
Updating summary to include some other affected games.
Tobias, looks like this patch wasn't accepted yet?
No, and I dunno why. Henri Verbeet had some provisos about it and I explained it to him, but he didn't reply after that. I'm going to paste my message here:
H. Verbeet wrote: I don't feel strongly about it, it's just that the comment talks a lot about what MSDN says or doesn't say, without actually describing what D3DPOOL_SCRATCH is supposed to mean, while ultimately that's all we care about.
I replied: That wasn't my primary intention. I didn't know why exactly the comment in CreateOffscreenPlainSurface was there, maybe it's just because this information was also given in the MSDN DirectX9 docs (see docs to IDirect3DDevice9::CreateOffscreenPlainSurface, there it tells you the same thing).
I just wanted to make clear that MSDN is wrong there. I don't know very much about how D3D is implemented in wine, but I think most of the time it's just looking into the docs and implementing the behaviour that's described there. Now someone searching for a bug or doing a rewrite of the D3D (or something else that involves big changes to the code) could be confused to see that behaviour described by MSDN and implemented behaviour differ. That's the only reason for the large amount of comments. And I thought it would be good to also add a comment in CreateOffscreenPlainSurface, so the whole thing is consistent (it's bad if one comment tells you one thing and then the other the opposite). I would rather not rip the whole comment out, because now we've got a better documentation of the DX behaviour than the MSDN itself :-)
Concerning the description of the pool types. I'm not exactly sure what you would want in such a comment. The current comment just talks about what happens if you plug D3DPOOL_SYSTEMMEM (previously D3DPOOL_SCRATCH, but we now know this is wrong) into CreateOffscreenPlainSurface. According to the docs that results in a surface object with same properties like the old DX8 method CreateImageSurface. That's all what the comment states. It's not about how the different pool types work internally and I doubt CreateOffscreenPlainSurface would be the right method where to place such a general comment. I don't know where pool types are actually "implemented" in wine, but description of the types should belong there.
Keep in mind that I'm absolutely newbie on this matter (apart from some programming with OpenGL), so I could be talking a lot of nonsense here :-D
Greets, Tobias
(quote END)
If the length of comments and msdn links are controversial, I suggest trimming them for brevity or maybe avoiding altogether.
There are no links to the MSDN in the patch. In my opinion the comments are necesarry, see my message to H. Verbeet.
msdn links are mostly avoided afaik because they change all the time, besides, it's best to shortly describe what happens in your own words and/or write a test, since msdn is misleading quite often.
Again, I didn't included any links. There are some keywords mentioned in the patch, which you can search in the MSDN to find what I'm basing my statements on. But that's all. I'm well aware that MSDN is always in flux, so the critical (mis)information is included in the patch.
Since there is a test, there shouldn't be much questions, it may be ok even without comments at all or maybe with "... creates surfaces in D3DPOOL_SYSTEM; games ... rely on it" or similar one liner. And wrong comment in d3d9 can simply be removed, maybe in a separate patch.
Again see my message to H. Verbeet.
Wine's implementation of D3D is based on what you can read in the MSDN. If the MSDN is wrong then we should comment that in wine's code.
I have an idea Alexander, why don't you hand in the (my) patch? I guess it's going to be applied in no time ;-)
Greets, Tobias
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #25 from Alexander Dorofeyev alexd4@inbox.lv 2008-07-01 12:12:03 --- (In reply to comment #24)
Wine's implementation of D3D is based on what you can read in the MSDN. If the MSDN is wrong then we should comment that in wine's code.
I'm not sure, but I may guess that clarity may have been the concern. I think a small note in IDirect3DDevice8Impl_CreateImageSurface (something like "creates a surface in sysmem pool. max payne and other games rely on that") will suffice. The rest is "documented" by test. For already implemented and tested Wine code we probably don't want to care too much about what msdn says anyway and give it lots of space (anything on msdn can be altered or removed any time btw, whenever ms deems fit). Comments in d3d9 that are wrong can be simply deleted, but maybe in different patch.
I have an idea Alexander, why don't you hand in the (my) patch? I guess it's going to be applied in no time ;-)
It's more fair if your fix goes in under your name, and it's not like my patches don't ever get delayed or rejected (sigh)...
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #26 from Tobias Jakobi liquid.acid@gmx.net 2008-07-02 16:10:26 --- (In reply to comment #25)
(In reply to comment #24)
Wine's implementation of D3D is based on what you can read in the MSDN. If the MSDN is wrong then we should comment that in wine's code.
I'm not sure, but I may guess that clarity may have been the concern. I think a small note in IDirect3DDevice8Impl_CreateImageSurface (something like "creates a surface in sysmem pool. max payne and other games rely on that") will suffice.
The point is not that Max Payne (or other games) rely on that behaviour. The allocation of the surface in SYSMEM pool IS the correct behaviour by definition. That is verified two times: (i) Original DirectX8 SDK documentation (I take the part from the D3D reference as a definition) (ii) testcase run on both XP and Vista (backing up that what DX8 docs says is right)
It's not like we're implementing something that was documented wrong in the first place. The person who implemented IDirect3DDevice8Impl_CreateImageSurface in wine was probably basing his impl on what the DX9 (NINE!) docs state. Presumably because the DX8 docs were not not available any longer.
To sum it up: DX8 docs explained correct behaviour of the method, DX9 docs spread misinformation about the pool type in a short remark.
The rest is "documented" by test. For already implemented and tested Wine code we probably don't want to care too much about what msdn says anyway and give it lots of space (anything on msdn can be altered or removed any time btw, whenever ms deems fit).
Microsoft would break backward compatibility big time if they changed this behaviour now. That applies to large parts of their docs.
Comments in d3d9 that are wrong can be simply deleted, but maybe in different patch.
So if I get this right I create three patches, right? (i) Testcase (ii) Patch to correct behaviour of IDirect3DDevice8Impl_CreateImageSurface (iii) Removal of the faulty comment
And the bulk of my comments why the behaviour is correct and MSDN is wrong, and all this stuff, should go into the testcase patch?
I have an idea Alexander, why don't you hand in the (my) patch? I guess it's going to be applied in no time ;-)
It's more fair if your fix goes in under your name, and it's not like my patches don't ever get delayed or rejected (sigh)...
I couldn't care less who is sending in this patch. I didn't dig into this for the credit, but to get it working (and perhaps a bit of interest in the D3D architecture).
So, I think I'm gonna do a last rewrite (and split) of the patch and post it here, so you (Alexander) can review it. Then I send it to wine-patches and if it's not applied then I leave it to other people to resend it again. I really don't have the nerve to invest this much time in a patch that consists basically of only one changed line of code.
Greets, Tobias
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #27 from Tobias Jakobi liquid.acid@gmx.net 2008-07-02 18:36:08 --- Created an attachment (id=14542) --> (http://bugs.winehq.org/attachment.cgi?id=14542) adds the testcase
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #28 from Tobias Jakobi liquid.acid@gmx.net 2008-07-02 18:36:36 --- Created an attachment (id=14543) --> (http://bugs.winehq.org/attachment.cgi?id=14543) fixes bug and activates test (which should pass now)
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #29 from Tobias Jakobi liquid.acid@gmx.net 2008-07-02 18:40:03 --- Created an attachment (id=14544) --> (http://bugs.winehq.org/attachment.cgi?id=14544) remove comment from d3d9 files (i didn't add anything there)
So here is the split patch set.
I refrained myself from adding additional comments to the IDirect3DDevice9::CreateOffscreenPlainSurface impl and only deleted the faulty part. All interesting comment part is now in the testcase patch, no links to the MSDN, just keywords...
A short comment in front of the IDirect3DDevice8Impl::CreateImageSurface impl.
I hope this is correct now?!
http://bugs.winehq.org/show_bug.cgi?id=9775
Austin English austinenglish@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Keywords| |patch, testcase
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #30 from Alexander Dorofeyev alexd4@inbox.lv 2008-07-03 11:03:34 --- I would also shorten the test's comment to one or few lines, but it's just my opinion. I do know that msdn references are not in very good favor in general, I think previously there even have been patches specifically removing msdn links. There is some sense in it, to discourage reliance on msdn for purpose of documenting the code (or there is a risk that after 5 years code documentation consists of references to materials that went the same way as e.g. DX6 docs went). Ultimately it's Alexandre Julliard who commits (or not) the patch. BTW a good way to know the status of patches / rejection reason is to ask him (nick julliard) on #winehackers, he is usually very responsive when online. It sometimes happens that patches get lost in high traffic.
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #31 from Tobias Jakobi liquid.acid@gmx.net 2008-07-03 12:20:36 --- (In reply to comment #30)
I would also shorten the test's comment to one or few lines, but it's just my opinion.
I don't know how to fit all important information in two lines. Even if I dropped the MSDN refs it would be much longer than two lines.
I do know that msdn references are not in very good favor in general, I think previously there even have been patches specifically removing msdn links. There is some sense in it, to discourage reliance on msdn for purpose of documenting the code (or there is a risk that after 5 years code documentation consists of references to materials that went the same way as e.g. DX6 docs went).
I don't think this is a valid point. It's true that Microsoft doesn't provide the docs anymore on their homepage, BUT there are still mirrors on the net providing these old docs for download. I have currently docs for DX7, 8 and 9 installed - from which only 9 is provided by MS currently (docs for 8 and 7 were removed already).
Also I don't think devs are reverse engineering the entire DX lib by watching what call XYZ does (like nouveau figures out how the nvidia hw works). If I'm going to fix say DX6 code I need the DX6 SDK anyway.
I do understand that paving the code with MSDN links isn't very clever, but at least allow it to contain some refs so you can look it up if you want further information (and have access to the docs).
-------------------------------------------------
Let's take a look at my comment block:
Test the behaviour of the IDirect3DDevice8::CreateImageSurface method.
What method is tested...
Expected behaviour (and also documented in the original DX8 docs) is that the call returns a surface with the SYSTEMMEM pool type. Games like Max Payne 1 and 2 which use Direct3D8 calls depend on this behaviour.
What the method should do and which apps that rely on the behaviour. Includes remark why this behaviour should be correct (further verified by the testcase)
A short remark in the DX9 docs however states that the pool of the returned surface object is of type SCRATCH. This is misinformation and results in screenshots not appearing in the savegame loading menu of both games mentioned above (engine tries to display a texture from the scratch pool).
Explaining why the bug popped up in the the first place: misinformation from DX9 Also xplaining the results of the wrong implementation.
This test verifies that the behaviour described in the original D3D8 docs is the correct one.
Summing up..
For more information about this issue, see the MSDN: D3D9 docs: "Converting to Direct3D 9" D3D9 reference: "IDirect3DDevice9::CreateOffscreenPlainSurface" D3D8 reference: "IDirect3DDevice8::CreateImageSurface"
Now for this one is important: it's COMPLETLY optional! As stated "for MORE information" - you don't need this to understand the core of the bug and the bugfix. Only a hint...
----------------------------------------
I'm always grateful of hints in sourcecode. Makes it easier for someone with no insight in the code to get involved. Usually you spend hours skimming through code until you get an idea what it is doing. I find it very helpful to get some clues where I can find further (documenting) information to get an overview. It also helps me speeding up my google searches, because with the overview I know what to search for (because I now know the right keywords).
I'm currently writing another testcase for a different D3D8 method (LockBox, the current implementation isn't really behaving how it should) and up to now it has almost no sourcecode comments - and it's not going to contain any refs to the MSDN, simply because it wouldn't make any sense there (the docs don't describe when locks should succeed or fail). So don't think I added the three refs just for fun, they are actually there to support/help further development.
Ultimately it's Alexandre Julliard who commits (or not) the patch. BTW a good way to know the status of patches / rejection reason is to ask him (nick julliard) on #winehackers, he is usually very responsive when online. It sometimes happens that patches get lost in high traffic.
Sure, but I was hoping to get some sort of ACK from a guru like you ;-) And I don't want to spam on wine-patches if the patches aren't applied anyway.
Greets, Tobias
http://bugs.winehq.org/show_bug.cgi?id=9775
Matteo Modesti mattemod@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |mattemod@gmail.com
--- Comment #32 from Matteo Modesti mattemod@gmail.com 2008-07-07 04:52:44 --- I tried to manually modify 'dlls/d3d8/device.c' and then ran Morrowind: - it now displays the character in Inventory window (even if upside-down) - the local and global maps now works correctly, they're not black anymore :) (and they're not upside-down) - for me, it seems the game runs a little bit faster - it doesn't print anymore lots and lots of these FIXMEs on the shell: fixme:d3d_surface:IWineD3DSurfaceImpl_LoadTexture (0x5f65c20) Operation not supported for scratch textures fixme:d3d_surface:fb_copy_to_texture_direct >>>>>>>>>>>>>>>>> GL_INVALID_OPERATION (0x502) from glCopyTexSubImage2D @ surface.c / 2745
Two words: great work! ;)
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #33 from Matteo Modesti mattemod@gmail.com 2008-07-07 04:57:03 --- Uh, forgot to say that i'm running wine-1.1.0-253-g2f1c7b1
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #34 from Matteo Modesti mattemod@gmail.com 2008-07-07 05:18:17 --- I noted that now it prints these FIXMEs on the shell every time i load a game:
fixme:d3d_surface:IWineD3DSurfaceImpl_PreLoad >>>>>>>>>>>>>>>>> GL_INVALID_ENUM (0x500) from glGenTextures @ surface.c / 513 fixme:d3d_surface:IWineD3DSurfaceImpl_PreLoad >>>>>>>>>>>>>>>>> GL_INVALID_ENUM (0x500) from glBindTexture @ surface.c / 517 fixme:d3d_surface:surface_allocate_surface >>>>>>>>>>>>>>>>> GL_INVALID_ENUM (0x500) from glTexImage2D @ surface.c / 340 fixme:d3d_surface:read_from_framebuffer_texture >>>>>>>>>>>>>>>>> GL_INVALID_ENUM (0x500) from glCopyTexSubImage2D @ surface.c / 936
Maybe fixing them will fix the upside-down character in inventory too :)
p.s.: sorry for multi-commenting in so short time, i didn't imagine i would've had something to add so soon ^^
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #35 from Tobias Jakobi liquid.acid@gmx.net 2008-07-07 07:36:50 --- Since I own Morrowind I can take a look at these. Did the FIXMEs appear before applying the patch?
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #36 from Tobias Jakobi liquid.acid@gmx.net 2008-07-07 08:06:37 --- Hi Matteo,
could you try patching the source (wined3d/surface.c):
In line 509, add checkGLcall("glEnable"); after the glEnable call.
The GL_INVALID_ENUM state is probably not coming from GenTextures at all.
Greets, Tobias
http://bugs.winehq.org/show_bug.cgi?id=9775
Vit Hrachovy vit.hrachovy@sandbox.cz changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |vit.hrachovy@sandbox.cz
--- Comment #37 from Vit Hrachovy vit.hrachovy@sandbox.cz 2008-07-07 13:59:17 --- I've applied the patches to 1.0rc5 sources and local map works 100%, same for character inventory portrait (No upside down for me). Great!! Now only to propagate the patches to the mainstream Wine :)
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #38 from Vit Hrachovy vit.hrachovy@sandbox.cz 2008-07-07 14:01:17 --- I've forgotten to mention I'm using fbo as OffscreenRenderingMode. Anyway - good luck and wish you quick way into Wine!
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #39 from Vit Hrachovy vit.hrachovy@sandbox.cz 2008-07-07 14:05:24 --- *** Bug 9892 has been marked as a duplicate of this bug. ***
http://bugs.winehq.org/show_bug.cgi?id=9775
ClanCC clancc@shadowkitsune.net changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |clancc@shadowkitsune.net
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #40 from Matteo Modesti mattemod@gmail.com 2008-07-07 22:22:56 --- Okay, I found a few very interesting things.
First of all, all my tests were done with OffscreenRenderingMode="pbuffers" (with the final 's'... I've never understood if it exists or if it use the default, but anyway it worked) because Morrowind crashed in outside areas for me with "fbo". About half an hour ago I tried to use "fbo" with your patch and... *tada!* It worked! So your patch also fix that bug :) I did some more testing then and found out the following things.
Since I own Morrowind I can take a look at these. Did the FIXMEs appear before applying the patch?
No, but instead of them there was fixme:d3d_surface:fb_copy_to_texture_direct >>>>>>>>>>>>>>>>> GL_INVALID_OPERATION (0x502) from glCopyTexSubImage2D @ surface.c / 2745
In line 509, add checkGLcall("glEnable"); after the glEnable call.
The GL_INVALID_ENUM state is probably not coming from GenTextures at all.
___With "pbuffers"___
As I said, maps work correctly but the character in inventory in upside-down and there's a red line above it.
After adding the checkGLcall() call, the first FIXME changed:
fixme:d3d_surface:IWineD3DSurfaceImpl_PreLoad >>>>>>>>>>>>>>>>> GL_INVALID_ENUM (0x500) from glEnable @ surface.c / 510 fixme:d3d_surface:IWineD3DSurfaceImpl_PreLoad >>>>>>>>>>>>>>>>> GL_INVALID_ENUM (0x500) from glBindTexture @ surface.c / 518 fixme:d3d_surface:surface_allocate_surface >>>>>>>>>>>>>>>>> GL_INVALID_ENUM (0x500) from glTexImage2D @ surface.c / 340 fixme:d3d_surface:read_from_framebuffer_texture >>>>>>>>>>>>>>>>> GL_INVALID_ENUM (0x500) from glCopyTexSubImage2D @ surface.c / 937
I noted that the FIXMEs aren't printed out when loading for the first time (in main menu) but only when you load from a different location or when you load games saved in outside areas.
___With "fbo"___
The FIXMEs aren't printed out at all. The character in inventory is not upside-down and the red line's not there anymore... it works correctly :)
BUT the local map doesn't work at all for me: it's all dark, apart some very little parts which fade from clear to dark. I'm referring to the one that appears right-clicking the mouse, not the one in the bottom-right corner of the screen. If I'm not wrong, there's only one "faded part" for each area (you can better see it going outside and scrolling the local map).
I think I'll continue using "pbuffers", because the map's more important than the character in the inventory and because I'd like to help you fix "pbuffers" problems :) ...uh, and because it seems "fbo" is a bit slower :P
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #41 from Matteo Modesti mattemod@gmail.com 2008-07-07 22:30:22 --- Sorry, in my previous comments I forgot to say that I'm using fglrx, so I have to get rid of bug 12929 commenting out GL_ATI_envmap_bumpmap in 'dlls/wined3d/directx.c' (line 60) else Morrowind would crash on start (in ActivateContext if i remember properly). I don't know if it can help or is a problem...
http://bugs.winehq.org/show_bug.cgi?id=9775
tyle7@hotmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |tyle7@hotmail.com
--- Comment #42 from tyle7@hotmail.com 2008-07-08 16:34:08 ---
Tested with latest wine from Git. Touhou Bunkachou (mentioned in duplicate bug 7801) now shows high score pictures as it is supposed to, and there are no more "Operation not supported for scratch textures" messages.Good work!
http://bugs.winehq.org/show_bug.cgi?id=9775
Tobias Jakobi liquid.acid@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |RESOLVED Resolution| |FIXED
--- Comment #43 from Tobias Jakobi liquid.acid@gmx.net 2008-07-08 16:47:52 --- Bug is fixed in wine GIT master, setting to RESOLVED FIXED.
@Matteo: We should open a new bug for the Morrowind problems and try to solve them there :-)
Greets, Tobias
http://bugs.winehq.org/show_bug.cgi?id=9775
Alexandre Julliard julliard@winehq.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED
--- Comment #44 from Alexandre Julliard julliard@winehq.org 2008-07-11 11:08:58 --- Closing bugs fixed in 1.1.1.
http://bugs.winehq.org/show_bug.cgi?id=9775
--- Comment #45 from Jörg Höhle hoehle@users.sourceforge.net 2008-09-15 07:24:45 --- How can I vote for Tobias Jakobi's bugfix to be added to the stable 1.0 branch? I mean commit 9ba3d03e7848ec7881ffc3cfdcf01a76d1aa1cc5 cf. comment #12 "In IDirect3DDevice8Impl_CreateImageSurface I changed the pool parameter from D3DPOOL_SCRATCH to D3DPOOL_SYSTEMMEM." It is a very local patch, unlikely to break compatibility in 1.0, which I applied it to my copy of 1.0. It's beautiful to finally see the thumbnails in the load/save game dialog -- I even reloaded some old savegames and saved them again just to get the pictures.
http://bugs.winehq.org/show_bug.cgi?id=9775
Austin English austinenglish@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Target Milestone|--- |1.0.1
--- Comment #46 from Austin English austinenglish@gmail.com 2008-09-15 13:09:00 --- Nominating for 1.0.1
-
wine-bugs＠winehq.org