-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hi,
I missed a few things the last time. This list is exhaustive now, at least as far as I'm concerned. I have listed most things for d3d8, but they apply to the d3d9 test as well.
Am 2014-10-10 15:21, schrieb Joachim Priesner:
> + static const D3DMATRIX proj_mat =
> + {{{
> + 1.0f, 0.0f, 0.0f, 0.0f,
> + 0.0f, 1.0f, 0.0f, 0.0f,
> + 0.0f, 0.0f, -1.0f, 0.0f,
> + 0.0f, 0.0f, -0.0f, 1.0f
> + }}};
Minor detail: you left a redundant - in here. (4th row, third column, -0.0f)
> + /* Basic vertex shader with fog computation ("foggy").
> + * Using the vertex' x coordinate as fog coordinate so that all test cases that use vertex fog
> + * will have a gradient from fully fogged (green, left) to unfogged (red, right). */
This is a bit off-topic, but I'm still noting it here: We don't have any tests for the clamp(0.0, 1.0) we do on the oFog output. Hence it may be interesting to check the middle of the screen in case of the foggy shader. If it is half fogged, the clamp is correct. If it is fully fogged the clamp is wrong (keep in mind that "foggy shaders" have reverse fog with start = 1, end = 0).
This shouldn't be part of this patch, but it is an idea for a follow-up test if you or anyone else is interested.
> + {0, 0, D3DFOG_NONE, D3DFOG_LINEAR, COLOR_UNFOGGED, COLOR_FOGGED},
> + {0, 1, D3DFOG_NONE, D3DFOG_LINEAR, COLOR_UNFOGGED, COLOR_FOGGED},
> + {0, 0, D3DFOG_LINEAR, D3DFOG_LINEAR, COLOR_UNFOGGED, COLOR_FOGGED},
> + {0, 1, D3DFOG_LINEAR, D3DFOG_LINEAR, COLOR_UNFOGGED, COLOR_FOGGED},
> + {0, 0, D3DFOG_LINEAR, D3DFOG_NONE, COLOR_UNFOGGED, COLOR_FOGGED},
> + {0, 1, D3DFOG_LINEAR, D3DFOG_NONE, COLOR_UNFOGGED, COLOR_FOGGED},
Please add a D3DFOG_NONE, D3DFOG_NONE case for completeness. In case of the shaders I expect the same result as with table fog = NONE, vertex fog = LINEAR. In case of fixed function fragment processing I expect fully fogged because it uses the alpha component of the specular color, which defaults to 0. Fogstart = 255 and fogend = 0 in that case, similar to the vertex shader oFog case.
> + /* Note: Shaders expect matrices in column-major form, so they have
> + * to be transposed if they are not symmetrical (which is the case at the moment). */
> + hr = IDirect3DDevice8_SetVertexShaderConstant(device, 0, &view_mat, 4);
> + ok(SUCCEEDED(hr), "SetVertexShaderConstant (view matrix) failed (%08x)\n", hr);
> + hr = IDirect3DDevice8_SetVertexShaderConstant(device, 4, &proj_mat, 4);
> + ok(SUCCEEDED(hr), "SetVertexShaderConstant (projection matrix) failed (%08x)\n", hr);
I'd expect these calls to fail on hardware that does not support shaders. Same for the Set*Shader(NULL) calls later on.
Technically you do not need the matrix multiply inside the shader with the new matrices, but if you want to keep them in to make the fixed function and shader codepaths more similar that's OK with me.
The D3DPRASTERCAPS_FOGTABLE capability applies to d3d8/9 as well. I don't know if there's any GPU out there that doesn't support table fog other than my ancient ATI 3D Rage.
> + IDirect3DDevice8_Present(device, NULL, NULL, NULL, NULL);
You are not checking the return value here.
> + if (caps.PixelShaderVersion >= D3DPS_VERSION(1, 1))
> + {
> + hr = IDirect3DDevice9_CreateVertexShader(device, vertex_shader_code1, &vertex_shader[1]);
> + ok(SUCCEEDED(hr), "CreateVertexShader failed (%08x)\n", hr);
> + hr = IDirect3DDevice9_CreateVertexShader(device, vertex_shader_code2, &vertex_shader[2]);
> + ok(SUCCEEDED(hr), "CreateVertexShader failed (%08x)\n", hr);
> ...
> + }
Please check the vertex shader version separately from the pixel shader one.
> diff --git a/dlls/ddraw/tests/visual.c b/dlls/ddraw/tests/visual.c
> index 0385044..23542ff 100644
> --- a/dlls/ddraw/tests/visual.c
> +++ b/dlls/ddraw/tests/visual.c
New ddraw tests should go to dlls/ddraw/tests/ddraw*.c. I'm fine with just a ddraw7 test for this. The older ddraw interfaces have a different fog interface (D3DLIGHTSTATE_FOG*, and somehow there's a separate D3DRENDERSTATE_FOGTABLE*). While we could certainly use more tests for this interface it shouldn't be the concern of this patch.
> + if (IDirect3DDevice7_BeginScene(device) == D3D_OK)
This is not a big issue, but there's no need to have a code branch for BeginScene / EndScene, just let the draws in between fail like you do in d3d8/9. I have never seen a case where BeginScene fails unexpectedly. (Yeah, the existing code isn't consistent on this point.)
> + hr = IDirect3DDevice8_SetRenderState(device, D3DRS_FOGEND, end.i);
> + ok(SUCCEEDED(hr), "Setting fog end failed (%08x)\n", hr);
> ...
> + hr = IDirect3DDevice8_SetTransform(device, D3DTS_PROJECTION, &proj_mat);
> + ok(SUCCEEDED(hr), "Failed to set projection transform, hr %#x.\n", hr);
Another minor style issue: You're mixing %08x and %#x. The preferred one is %#x, but what mostly matters is consistency. Similar for punctuation at the end of the traces. SUCCEEDED(hr) is preferred over hr == D3D_OK.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQIcBAEBAgAGBQJUOFewAAoJEN0/YqbEcdMwxSYP/jFB5IT9a+7w8JQ7vlziVdGz
KZccLwVFSyJqLhEiTIs7B0yz5rWhkjI6HPxzWRPRMVbNTjlUvsiZRLl8FU6hR86A
1G6azwEnMs0/u9LUOfdphBLdj454+E/NJMr1EzkRjL+PMt49RZwSxdCmxPXK/unx
yrxLSPuxCjHGhN6K95/xV8fVkbJoa7va8IeKDHhfoFpdpaUqUqyGaSbNCfiyYj6l
cyA8ZBlQEEEYBDg8HopQgw99MmfU3BIL4rY984YnsscYMckI5qjOs+joMcUwHscU
Vaflkmh5udcj74sdgPguOwigJeLuLzY6zbWzhUGDFzdkXuwfuF0oe+ws/QcNOYX6
r2d/CGa7nYXfTFewR4SODJT+950P8CwuskLsslBAiPpy4uzVcbhKT2oNh7L7vhU1
+o6gYk98ceCaRdZxajf+j2hElg7N9AkWAK/RfLY25d3ALnKKVDbaidWKot3dVcHN
nicK89v6jKSmD/FdbVM9/YUi4QzOZM5IJAItG0OuhS0oU/Bfkpr0e7NXvORxdGPC
BpRvQrwIJXWiT43aqK8E1w67mE47PMmqoKjO3qVfWLojC0L7KPybDjGtUKj652Qr
ixqPlaERPLeRVyAGtpxHlIE2opyx3fOSy/ZSTq6/PzK4GNZ/hgfTLbUff2ixxa+M
cL3+mTtlQSBx9yt9BmUG
=ZDPP
-----END PGP SIGNATURE-----