Re: [5/6] ddraw: fix palette copying when destination palette is NULL
Am Montag, 31. Dezember 2007 03:04:02 schrieb Alexander Dorofeyev:
I think there's a bug here. If pal == NULL that implies (because of preceding code) that wine_pal == NULL. So it's setting a NULL wined3d palette to the destination wined3d surface, yet increases reference counter on source palette. I guess it should pass source wined3d palette here, to make any sense. --- dlls/ddraw/texture.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) I am not sure about this, but honestly I don't really know much about IDirect3DTexture2::Load is supposed to work. When I ported ddraw.dll to use wined3d I only did routine adjustments to the code to cope with the wined3d objects. I think Lionel Ulmer wrote the original code, maybe he can comment on it?
I think we really need a few unit tests for this method. From the docs, it seems that this method is related to IDirect3DDevice9::LoadTexture, maybe we can map it to WineD3D's loadtexture.
Hi, Stefan. Well, I think at least it has to be more correct than existing code. The existing code would probably lead to a leak, if that codepath is entered (doesn't enter it in AVP1 though) - unless I'm missing something, it will break the reference count of "parent" ddraw palette (pal_src) which is maintained as a count of how many wined3d surfaces hold its "child" wined3d palette (plus of course any temporary references or references held by user). And all surrounding code suggests that what it is attempting to do is copy palette from the source to target texture. The thing that it tries this copy palette thing at all (AVP1 doesn't depend on that BTW, it does SetPalette on both source and target) - that shouldn't be difficult to test, I think I could write it. Beyond that, I don't know, the documentation on these old interfaces seems non-existant. Is there a way to obtain DX6 docs? Stefan Dösinger wrote:
Am Montag, 31. Dezember 2007 03:04:02 schrieb Alexander Dorofeyev:
I think there's a bug here. If pal == NULL that implies (because of preceding code) that wine_pal == NULL. So it's setting a NULL wined3d palette to the destination wined3d surface, yet increases reference counter on source palette. I guess it should pass source wined3d palette here, to make any sense. --- dlls/ddraw/texture.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) I am not sure about this, but honestly I don't really know much about IDirect3DTexture2::Load is supposed to work. When I ported ddraw.dll to use wined3d I only did routine adjustments to the code to cope with the wined3d objects. I think Lionel Ulmer wrote the original code, maybe he can comment on it?
I think we really need a few unit tests for this method. From the docs, it seems that this method is related to IDirect3DDevice9::LoadTexture, maybe we can map it to WineD3D's loadtexture.
Am Montag, 31. Dezember 2007 12:43:37 schrieb Alexander Dorofeyev:
The thing that it tries this copy palette thing at all (AVP1 doesn't depend on that BTW, it does SetPalette on both source and target) - that shouldn't be difficult to test, I think I could write it. Beyond that, I don't know, the documentation on these old interfaces seems non-existant. Is there a way to obtain DX6 docs? I suspect that we shouldn't copy the palette at all, I think Texture::Load and friends just copy the bytes over. A test could verify / falsify that. As for the docs, there were older dx docs available on some websites, but I lost the link. I'll see if I can find it.
OK, I'll see what I can do with the test. Speaking of tests, I submitted test for zero vertex rhw case (as suggested by H.Verbeet to avoid future regressions), but I think it didn't get accepted. Perhaps, you can look through it and provide some feedback? Would be good to hear the bad news now, before I submit more wrong stuff (esp. considering that I'm also trying to write a test for another issue with Aliens vs Predator 1 that I want to fix). I'm attaching the zero rhw test. Stefan Dösinger wrote:
Am Montag, 31. Dezember 2007 12:43:37 schrieb Alexander Dorofeyev:
The thing that it tries this copy palette thing at all (AVP1 doesn't depend on that BTW, it does SetPalette on both source and target) - that shouldn't be difficult to test, I think I could write it. Beyond that, I don't know, the documentation on these old interfaces seems non-existant. Is there a way to obtain DX6 docs? I suspect that we shouldn't copy the palette at all, I think Texture::Load and friends just copy the bytes over. A test could verify / falsify that. As for the docs, there were older dx docs available on some websites, but I lost the link. I'll see if I can find it.
diff --git a/dlls/ddraw/tests/visual.c b/dlls/ddraw/tests/visual.c index c9a3147..2269d70 100644 --- a/dlls/ddraw/tests/visual.c +++ b/dlls/ddraw/tests/visual.c @@ -761,6 +761,66 @@ static void alpha_test(IDirect3DDevice7 *device) if(backbuffer) IDirectDrawSurface7_Release(backbuffer); } +static void rhw_zero_test(IDirect3DDevice7 *device) +{ +/* Test if it will render a quad correctly when vertex rhw = 0 */ + HRESULT hr; + DWORD color; + + struct { + float x, y, z; + float rhw; + DWORD diffuse; + } quad1[] = + { + {0, 100, 0, 0, 0xffffffff}, + {0, 0, 0, 0, 0xffffffff}, + {100, 100, 0, 0, 0xffffffff}, + {100, 0, 0, 0, 0xffffffff}, + }; + + /* Clear to black */ + hr = IDirect3DDevice7_Clear(device, 0, NULL, D3DCLEAR_TARGET, 0, 0.0, 0); + ok(hr == D3D_OK, "Clear failed, hr = %08x\n", hr); + + /* Setup some states that may cause issues */ + hr = IDirect3DDevice7_SetRenderState(device, D3DRENDERSTATE_CLIPPING, FALSE); + ok(hr == D3D_OK, "IDirect3DDevice7_SetRenderState returned %08x\n", hr); + hr = IDirect3DDevice7_SetRenderState(device, D3DRENDERSTATE_ZENABLE, FALSE); + ok(hr == D3D_OK, "IDirect3DDevice7_SetRenderState returned %08x\n", hr); + hr = IDirect3DDevice7_SetRenderState(device, D3DRENDERSTATE_FOGENABLE, FALSE); + ok(hr == D3D_OK, "IDirect3DDevice7_SetRenderState returned %08x\n", hr); + hr = IDirect3DDevice7_SetRenderState(device, D3DRENDERSTATE_STENCILENABLE, FALSE); + ok(hr == D3D_OK, "IDirect3DDevice7_SetRenderState returned %08x\n", hr); + hr = IDirect3DDevice7_SetRenderState(device, D3DRENDERSTATE_ALPHATESTENABLE, FALSE); + ok(hr == D3D_OK, "IDirect3DDevice7_SetRenderState returned %08x\n", hr); + hr = IDirect3DDevice7_SetRenderState(device, D3DRENDERSTATE_ALPHABLENDENABLE, FALSE); + ok(hr == D3D_OK, "IDirect3DDevice7_SetRenderState returned %08x\n", hr); + hr = IDirect3DDevice7_SetRenderState(device, D3DRENDERSTATE_CULLMODE, D3DCULL_NONE); + ok(hr == D3D_OK, "IDirect3DDevice7_SetRenderState failed with %08x\n", hr); + hr = IDirect3DDevice7_SetTextureStageState(device, 0, D3DTSS_COLORARG1, D3DTA_CURRENT); + ok(hr == D3D_OK, "SetTextureStageState failed, hr = %08x\n", hr); + hr = IDirect3DDevice7_SetTextureStageState(device, 0, D3DTSS_COLOROP, D3DTOP_SELECTARG1); + ok(hr == D3D_OK, "SetTextureStageState failed, hr = %08x\n", hr); + + hr = IDirect3DDevice7_BeginScene(device); + ok(hr == D3D_OK, "IDirect3DDevice7_BeginScene failed with %08x\n", hr); + + if (hr == D3D_OK) { + hr = IDirect3DDevice7_DrawPrimitive(device, D3DPT_TRIANGLESTRIP, D3DFVF_XYZRHW | D3DFVF_DIFFUSE, quad1, 4, 0); + ok(hr == D3D_OK, "DrawPrimitive failed, hr = %08x\n", hr); + + hr = IDirect3DDevice7_EndScene(device); + ok(hr == D3D_OK, "IDirect3DDevice7_EndScene failed, hr = %08x\n", hr); + + color = (getPixelColor(device, 0, 0)) & 0xffffff; + ok(color == 0xffffff, "Got color %08x, expected 00ffffff\n", color); + + color = (getPixelColor(device, 101, 101)) & 0xffffff; + ok(color == 0, "Got color %08x, expected 00000000\n", color); + } +} + START_TEST(visual) { HRESULT hr; @@ -806,6 +866,7 @@ START_TEST(visual) fog_test(Direct3DDevice); offscreen_test(Direct3DDevice); alpha_test(Direct3DDevice); + rhw_zero_test(Direct3DDevice); cleanup: releaseObjects();
Am Dienstag, 1. Januar 2008 11:19:50 schrieb Alexander Dorofeyev:
OK, I'll see what I can do with the test. Speaking of tests, I submitted test for zero vertex rhw case (as suggested by H.Verbeet to avoid future regressions), but I think it didn't get accepted. Perhaps, you can look through it and provide some feedback? Would be good to hear the bad news now, before I submit more wrong stuff (esp. considering that I'm also trying to write a test for another issue with Aliens vs Predator 1 that I want to fix). I'm attaching the zero rhw test. It is likely that Alexandre was waiting for an OK from me or Henri. He is the only committer, but when someone he doesn't know sends a patch he usually waits for the OK from someone who is maintaining the part of Wine. I think I forgot to send an OK mail to the test when you submitted it.
A few minor suggestions for the test: * The vertex structure should be declared somewhere else already, as part of the lighting test I think. You can reuse that instead of redeclaring it. * You don't have to AND the color if 0xffffff, getPixelColor should do that already. * You don't have to set the render states unless you need a different setting than the previous tests leave behind * The "if(hr == D3D_OK)" after BeginScene should be changed to "if(SUCCEEDED(hr))"
On 01/01/2008, Stefan Dösinger <stefan(a)codeweavers.com> wrote:
It is likely that Alexandre was waiting for an OK from me or Henri. He is the only committer, but when someone he doesn't know sends a patch he usually waits for the OK from someone who is maintaining the part of Wine. I think I forgot to send an OK mail to the test when you submitted it. I didn't see anything obviously wrong with the test, except perhaps checking hr against D3D_OK, but ddraw is more your area.
* You don't have to set the render states unless you need a different setting than the previous tests leave behind
I don't think it's a bad thing to do though... it's pretty useful the be able to comment out the other tests without breaking the current test, especially when writing new tests. In general, we might want to consider using a new device for each test to prevent states from one test interfering with those from another test.
OK, thanks a lot. Regarding vertex structure - I'm afraid I can't reuse struct vertex/nvertex, because I need a very specific format for this test (D3DFVF_XYZRHW | D3DFVF_DIFFUSE). I could probably reuse sVertexT from fogtest, but then I'll also have to set specular, that I don't really need. I looks easier to declare a custom format that has just what I need. I'll make changes according to other suggestions. I've ran into some issues while trying to write a test for the old D3D TEXTUREMAPBLEND state AVP1 game uses. I hope you can help me a bit with it. The problem is, interfaces at least as recent as DX7 seem to be refusing this state (I can't make it work on XP). So I probably have to use something older. Unfortunately, so far I had no luck in creating interfaces that are older than IDirect3DDevice7, but less painful to use than the very first IDirect3DDevice: I don't have any DX6 docs and all working examples I can find including AVP1 use either IDirect3DDevice or something from DX7 or later, and trivial modifications to examples with IDirect3DDevice I've made fail on XP, usually on the step when I create IDirect3DDevice3 or something like that. So grudgingly I settled for IDirect3DDevice and I even have an almost complete test. But it's longer that it could be with DrawPrimitive calls, because of all that executebuffer horror. So the question is, is it ok if I submit it like this? From a certain point of view, there can even be benefits from using IDirect3DDevice, as it may also test some functions and codepaths not covered by other tests. Or do I have to continue to look for ways to write it with newer interfaces so that to make it more compact by getting rid of executebuffer? And, also, where to put it? ddraw/tests/visual.c seems mostly IDirect3DDevice7-centric, so, perhaps, visual tests involving earlier interfaces should go in a separate file? Or put it into visual.c and simply ignore / release DX7 interfaces? Stefan Dösinger wrote:
It is likely that Alexandre was waiting for an OK from me or Henri. He is the only committer, but when someone he doesn't know sends a patch he usually waits for the OK from someone who is maintaining the part of Wine. I think I forgot to send an OK mail to the test when you submitted it.
A few minor suggestions for the test: * The vertex structure should be declared somewhere else already, as part of the lighting test I think. You can reuse that instead of redeclaring it.
* You don't have to AND the color if 0xffffff, getPixelColor should do that already.
* You don't have to set the render states unless you need a different setting than the previous tests leave behind
* The "if(hr == D3D_OK)" after BeginScene should be changed to "if(SUCCEEDED(hr))"
Am Mittwoch, 2. Januar 2008 02:16:13 schrieb Alexander Dorofeyev:
OK, thanks a lot. Regarding vertex structure - I'm afraid I can't reuse struct vertex/nvertex, because I need a very specific format for this test (D3DFVF_XYZRHW | D3DFVF_DIFFUSE). I could probably reuse sVertexT from fogtest, but then I'll also have to set specular, that I don't really need. I looks easier to declare a custom format that has just what I need. Ah, that's ok then. I forgot that the other vertex has a specular color, so a new vertex type is fine.
I've ran into some issues while trying to write a test for the old D3D TEXTUREMAPBLEND state AVP1 game uses. I hope you can help me a bit with it. Yeah, IDirect3DDevice(1) and IDirect3DExecuteBuffer are a royal pain in the ass. Here's some popular rant about it to ease your pain:
http://www.exaflop.org/docs/d3dogl/d3dogl_jc_plan.html I think that a test using execute buffers is a great thing. This interface is terribly obscured and we have only one small test for it. You can add it to visual.c if you want, or a new file(maybe executebuffer.c, or something), I think both places are ok. You can also try to QueryInterface old IDirect3DDevice* versions from IDirect3DDevice7, but I think this doesn't work as the COM theory says on Windows. I haven't yet tested the restrictions and implemented them because I haven't seen a game yet that depends on them.
Stefan Dösinger wrote:
You can also try to QueryInterface old IDirect3DDevice* versions from IDirect3DDevice7, but I think this doesn't work as the COM theory says on Windows. I haven't yet tested the restrictions and implemented them because I haven't seen a game yet that depends on them.
Yes, I think it doesn't run on Windows, IIRC that was the first thing I tried. At least this is so for XP, which itself is partly broken in DX6 and less support though, from what I see (e.g. AVP1 3D engine is broken). Just one more question - I'm looking into this hr==D3D_OK vs SUCCEEDED(hr) thing and there appears to be some problems with consistency. In ddraw/tests/visual.c I see both hr==D3D_OK and SUCCEEDED, in one place even result of BeginScene are checked as hr==D3D_OK, so it's somewhat confusing. How do I tell, generally, what to use for ok(), if() etc, hr==D3D_OK or SUCCEDED? Do all checks ultimately need to be turned into SUCCEEDED or it's just that some of the calls have been found to return undocumented values other than D3D_OK, but are no errors?
On 02/01/2008, Alexander Dorofeyev <alexd4(a)inbox.lv> wrote:
Stefan Dösinger wrote:
You can also try to QueryInterface old IDirect3DDevice* versions from IDirect3DDevice7, but I think this doesn't work as the COM theory says on Windows. I haven't yet tested the restrictions and implemented them because I haven't seen a game yet that depends on them.
Yes, I think it doesn't run on Windows, IIRC that was the first thing I tried. At least this is so for XP, which itself is partly broken in DX6 and less support though, from what I see (e.g. AVP1 3D engine is broken).
Just one more question - I'm looking into this hr==D3D_OK vs SUCCEEDED(hr) thing and there appears to be some problems with consistency. In ddraw/tests/visual.c I see both hr==D3D_OK and SUCCEEDED, in one place even result of BeginScene are checked as hr==D3D_OK, so it's somewhat confusing. How do I tell, generally, what to use for ok(), if() etc, hr==D3D_OK or SUCCEDED? Do all checks ultimately need to be turned into SUCCEEDED or it's just that some of the calls have been found to return undocumented values other than D3D_OK, but are no errors?
Unless you actually want to test the exact return value, you should use SUCCEEDED / FAILED. Some of the tests do get this wrong, unfortunately.
Am Dienstag, 1. Januar 2008 21:53:02 schrieb H. Verbeet:
Unless you actually want to test the exact return value, you should use SUCCEEDED / FAILED. Some of the tests do get this wrong, unfortunately. I usually test against the exact return value(D3D_OK) in the ok() statements, and SUCCEEDED / FAILED when deciding the codepath the test takes(e.g. if(SUCCEEDED(BeginScene())) )
participants (3)
-
Alexander Dorofeyev -
H. Verbeet -
Stefan Dösinger