On 2016-08-06 16:29, Aaryaman Vasishta wrote:
Signed-off-by: Aaryaman Vasishta jem456.vasishta@gmail.com
dlls/d3drm/tests/d3drm.c | 640 +++++++++++++++++++++++++++++++++++++++++++++++ dlls/d3drm/viewport.c | 71 +++++- 2 files changed, 704 insertions(+), 7 deletions(-)
diff --git a/dlls/d3drm/tests/d3drm.c b/dlls/d3drm/tests/d3drm.c index c4020e4..b9a83a0 100644 --- a/dlls/d3drm/tests/d3drm.c +++ b/dlls/d3drm/tests/d3drm.c @@ -5852,6 +5852,644 @@ static void test_viewport_qi(void) IDirect3DRM_Release(d3drm1); }
+static D3DCOLOR get_surface_color(IDirectDrawSurface *surface, UINT x, UINT y) +{
- RECT rect = { x, y, x + 1, y + 1 };
- DDSURFACEDESC surface_desc;
- D3DCOLOR color;
- HRESULT hr;
- memset(&surface_desc, 0, sizeof(surface_desc));
- surface_desc.dwSize = sizeof(surface_desc);
- hr = IDirectDrawSurface_Lock(surface, &rect, &surface_desc, DDLOCK_READONLY | DDLOCK_WAIT, NULL);
- ok(SUCCEEDED(hr), "Failed to lock surface, hr %#x.\n", hr);
- if (FAILED(hr))
return 0xdeadbeef;- color = *((DWORD *)surface_desc.lpSurface) & 0x00ffffff;
- hr = IDirectDrawSurface_Unlock(surface, NULL);
- ok(SUCCEEDED(hr), "Failed to unlock surface, hr %#x.\n", hr);
- return color;
+}
+static BOOL compare_color(D3DCOLOR c1, D3DCOLOR c2, BYTE max_diff) +{
- if ((c1 & 0xff) - (c2 & 0xff) > max_diff) return FALSE;
- c1 >>= 8; c2 >>= 8;
- if ((c1 & 0xff) - (c2 & 0xff) > max_diff) return FALSE;
- c1 >>= 8; c2 >>= 8;
- if ((c1 & 0xff) - (c2 & 0xff) > max_diff) return FALSE;
- c1 >>= 8; c2 >>= 8;
- if ((c1 & 0xff) - (c2 & 0xff) > max_diff) return FALSE;
- return TRUE;
+}
+static void clear_depth_surface(IDirectDrawSurface *surface, DWORD value) +{
- HRESULT hr;
- DDBLTFX fx;
- memset(&fx, 0, sizeof(fx));
- fx.dwSize = sizeof(fx);
- U5(fx).dwFillDepth = value;
- hr = IDirectDrawSurface_Blt(surface, NULL, NULL, NULL, DDBLT_DEPTHFILL | DDBLT_WAIT, &fx);
- ok(SUCCEEDED(hr), "Got unexpected hr %#x.\n", hr);
+}
+static void set_execute_data(IDirect3DExecuteBuffer *execute_buffer, UINT vertex_count, UINT offset, UINT len) +{
- D3DEXECUTEDATA exec_data;
- HRESULT hr;
- memset(&exec_data, 0, sizeof(exec_data));
- exec_data.dwSize = sizeof(exec_data);
- exec_data.dwVertexCount = vertex_count;
- exec_data.dwInstructionOffset = offset;
- exec_data.dwInstructionLength = len;
- hr = IDirect3DExecuteBuffer_SetExecuteData(execute_buffer, &exec_data);
- ok(SUCCEEDED(hr), "Failed to set execute data, hr %#x.\n", hr);
+}
+static void emit_set_ts(void **ptr, D3DTRANSFORMSTATETYPE state, DWORD value) +{
- D3DINSTRUCTION *inst = *ptr;
- D3DSTATE *ts = (D3DSTATE *)(inst + 1);
- inst->bOpcode = D3DOP_STATETRANSFORM;
- inst->bSize = sizeof(*ts);
- inst->wCount = 1;
- U1(*ts).dtstTransformStateType = state;
- U2(*ts).dwArg[0] = value;
- *ptr = ts + 1;
+}
+static void emit_set_rs(void **ptr, D3DRENDERSTATETYPE state, DWORD value) +{
- D3DINSTRUCTION *inst = *ptr;
- D3DSTATE *rs = (D3DSTATE *)(inst + 1);
- inst->bOpcode = D3DOP_STATERENDER;
- inst->bSize = sizeof(*rs);
- inst->wCount = 1;
- U1(*rs).drstRenderStateType = state;
- U2(*rs).dwArg[0] = value;
- *ptr = rs + 1;
+}
+static void emit_process_vertices(void **ptr, DWORD flags, WORD base_idx, DWORD vertex_count) +{
- D3DINSTRUCTION *inst = *ptr;
- D3DPROCESSVERTICES *pv = (D3DPROCESSVERTICES *)(inst + 1);
- inst->bOpcode = D3DOP_PROCESSVERTICES;
- inst->bSize = sizeof(*pv);
- inst->wCount = 1;
- pv->dwFlags = flags;
- pv->wStart = base_idx;
- pv->wDest = 0;
- pv->dwCount = vertex_count;
- pv->dwReserved = 0;
- *ptr = pv + 1;
+}
+static void emit_tquad(void **ptr, WORD base_idx) +{
- D3DINSTRUCTION *inst = *ptr;
- D3DTRIANGLE *tri = (D3DTRIANGLE *)(inst + 1);
- inst->bOpcode = D3DOP_TRIANGLE;
- inst->bSize = sizeof(*tri);
- inst->wCount = 2;
- U1(*tri).v1 = base_idx;
- U2(*tri).v2 = base_idx + 1;
- U3(*tri).v3 = base_idx + 2;
- tri->wFlags = D3DTRIFLAG_START;
- ++tri;
- U1(*tri).v1 = base_idx + 2;
- U2(*tri).v2 = base_idx + 1;
- U3(*tri).v3 = base_idx + 3;
- tri->wFlags = D3DTRIFLAG_ODD;
- ++tri;
- *ptr = tri;
+}
+static void emit_end(void **ptr) +{
- D3DINSTRUCTION *inst = *ptr;
- inst->bOpcode = D3DOP_EXIT;
- inst->bSize = 0;
- inst->wCount = 0;
- *ptr = inst + 1;
+}
+static void d3d_draw_quad1(IDirect3DDevice *device, IDirect3DViewport *viewport) +{
- IDirect3DExecuteBuffer *execute_buffer;
- D3DEXECUTEBUFFERDESC exec_desc;
- HRESULT hr;
- void *ptr;
- UINT inst_length;
- D3DMATRIXHANDLE world_handle, view_handle, proj_handle;
- static D3DMATRIX 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,- };
- static const D3DLVERTEX quad_strip[] =
- {
{{-1.0f}, {-1.0f}, {0.80f}, 0, {0xffbada55}},{{-1.0f}, { 1.0f}, {0.80f}, 0, {0xffbada55}},{{ 1.0f}, {-1.0f}, {0.80f}, 0, {0xffbada55}},{{ 1.0f}, { 1.0f}, {0.80f}, 0, {0xffbada55}},- };
You could make this
{{-1.0f}, {-1.0f}, {0.0f}, 0, {0xffbada55}}, {{-1.0f}, { 1.0f}, {0.0f}, 0, {0xffbada55}}, {{ 1.0f}, {-1.0f}, {1.0f}, 0, {0xffbada55}}, {{ 1.0f}, { 1.0f}, {1.0f}, 0, {0xffbada55}},
Then your currently discarded draws will draw to ca x = 250. That way you can make sure the draw is really discarded by the depth test and not some other reason.
- /* Check what happens if we release the depth surface that d3drm created, and clear the viewport */
...
- hr = IDirect3DRMViewport_Clear(viewport1);
- ok(SUCCEEDED(hr), "Cannot clear viewport (hr = %#x).\n", hr);
It's worth checking the color surface here to see if the clear was completely ignored or if it realized that there's no depth surface and it only cleared color. Also clear with a different color than what's left behind by the previous test.
...
- d3d_draw_quad1(d3d_device1, d3d_viewport);
- ret_color = get_surface_color(surface, 100, 200);
- todo_wine ok(compare_color(ret_color, 0x000000ff, 1), "Got unexpected color 0x%08x.\n", ret_color);
My reading of this is that the depth surface wasn't cleared (everything else would be very surprising). If the color surface was indeed cleared this shows that native at some place expects your ugly deed of removing the depth surface. If the color is not cleared it and the clear call is completely ignored I'd say something is broken somewhere and we shouldn't care too much about this corner case.
- hr = IDirect3DRM_CreateViewport(d3drm1, device1, dummy_frame1, 0, 0, rc.right,
rc.bottom, &viewport1);- ok(SUCCEEDED(hr), "Cannot get IDirect3DRMViewport interface (hr = %#x)\n", hr);
- /* Clear uses the scene/root frame's background color. */
Two newlines. Is that intentional?
- hr = IDirect3DRMFrame_SetSceneBackgroundRGB(frame1, 1.0f, 1.0f, 1.0f);
- ok(SUCCEEDED(hr), "Cannot set scene background RGB (hr = %#x)\n", hr);
- ret_color = IDirect3DRMFrame_GetSceneBackground(frame1);
- ok(ret_color == 0xffffffff, "Expected scene color returned == 0xffffffff, got %#x.\n", ret_color);
- hr = IDirect3DRMFrame_SetSceneBackgroundRGB(dummy_frame1, 0.0f, 1.0f, 0.0f);
- ok(SUCCEEDED(hr), "Cannot set scene background RGB (hr = %#x)\n", hr);
- ret_color = IDirect3DRMFrame_GetSceneBackground(dummy_frame1);
- ok(ret_color == 0xff00ff00, "Expected scene color returned == 0xff00ff00, got %#x.\n", ret_color);
- hr = IDirect3DRMFrame_SetSceneBackgroundRGB(dummy_frame1, 0.0f, 0.0f, 1.0f);
- ok(SUCCEEDED(hr), "Cannot set scene background RGB (hr = %#x)\n", hr);
- ret_color = IDirect3DRMFrame_GetSceneBackground(dummy_frame1);
- ok(ret_color == 0xff0000ff, "Expected scene color returned == 0xff0000ff, got %#x.\n", ret_color);
Can't the "where does the color come from" test be merged into the clear test at the start?
+static void draw_quad2(IDirect3DDevice2 *device, IDirect3DViewport *viewport) +{
Do you need to draw with IDirect3DDevice2 directly here? What about QI'ing IDirect3DDevice and calling d3d_draw_quad1?
diff --git a/dlls/d3drm/viewport.c b/dlls/d3drm/viewport.c index cb57fe0..371aae3 100644 --- a/dlls/d3drm/viewport.c +++ b/dlls/d3drm/viewport.c @@ -39,6 +39,42 @@ static inline struct d3drm_viewport *impl_from_IDirect3DRMViewport2(IDirect3DRMV return CONTAINING_RECORD(iface, struct d3drm_viewport, IDirect3DRMViewport2_iface); }
+static inline void d3drm_normalize_d3d_color(D3DCOLORVALUE *color_value, D3DCOLOR color) +{
- color_value->r = RGBA_GETRED(color) / 255.0f;
- color_value->g = RGBA_GETGREEN(color) / 255.0f;
- color_value->b = RGBA_GETBLUE(color) / 255.0f;
- color_value->a = RGBA_GETALPHA(color) / 255.0f;
+}
+static HRESULT d3drm_update_background_material(struct d3drm_viewport *viewport) +{
- IDirect3DRMFrame *root_frame;
- D3DCOLOR color;
- D3DMATERIAL mat;
- D3DMATERIALHANDLE hmat;
- HRESULT hr;
- if (FAILED(hr = IDirect3DRMFrame_GetScene(viewport->camera, &root_frame)))
return hr;- color = IDirect3DRMFrame_GetSceneBackground(root_frame);
You are leaking root_frame here.
- if (FAILED(hr = IDirect3DMaterial_GetHandle(viewport->material, viewport->device->device, &hmat)))
return hr;- if (FAILED(hr = IDirect3DViewport_SetBackground(viewport->d3d_viewport, hmat)))
return hr;
You can do these two calls during viewport creation, you don't have to repeat them every time.
@@ -344,10 +380,7 @@ static HRESULT WINAPI d3drm_viewport2_Init(IDirect3DRMViewport2 *iface, IDirect3
memset(&mat, 0, sizeof(mat)); mat.dwSize = sizeof(mat);
- mat.diffuse.r = RGBA_GETRED(color) / 255.0f;
- mat.diffuse.g = RGBA_GETGREEN(color) / 255.0f;
- mat.diffuse.b = RGBA_GETBLUE(color) / 255.0f;
- mat.diffuse.a = RGBA_GETALPHA(color) / 255.0f;
d3drm_normalize_d3d_color(&mat.diffuse, color);
if (FAILED(hr = IDirect3DMaterial_SetMaterial(material, &mat))) goto cleanup;
You can remove the IDirect3DMaterial::SetMaterial call from Init because you have to do it every time clear is called anyway.
Keep the GetHandle / SetBackground call in Init though to link the material with the viewport.
static HRESULT WINAPI d3drm_viewport2_Clear(IDirect3DRMViewport2 *iface, DWORD flags) ...
- if (flags & D3DRMCLEAR_ZBUFFER)
clear_flags |= D3DCLEAR_ZBUFFER;
I guess somewhere here or in the IDirect3DViewport::Clear implementation in ddraw you'll have to handle your missing depth buffer case.
On 2016-08-06 23:36, Stefan Dösinger wrote:
My reading of this is that the depth surface wasn't cleared (everything else would be very surprising). If the color surface was indeed cleared this shows that native at some place expects your ugly deed of removing the depth surface. If the color is not cleared it and the clear call is completely ignored I'd say something is broken somewhere and we shouldn't care too much about this corner case.
I guess I should have read the version 2 test before hitting "send"...
The v2 behavior is somewhat weird. What's even weirder is that you didn't need any todo_wine to make it work in Wine (whereas you need the todo_wine for the "sane" v1 behavior). Our DeleteAttachedSurface implementation sets the wined3d DS to NULL, at which point a D3DCLEAR_ZBUFFER clear should return an error. Do you have any idea why the depth buffer is still cleared on Wine?
On Sun, Aug 7, 2016 at 4:19 AM, Stefan Dösinger stefandoesinger@gmail.com wrote:
On 2016-08-06 23:36, Stefan Dösinger wrote:
My reading of this is that the depth surface wasn't cleared (everything else would be very surprising). If the color surface was indeed cleared this shows that native at some place expects your ugly deed of removing the depth surface. If the color is not cleared it and the clear call is completely ignored I'd say something is broken somewhere and we shouldn't care too much about this corner case.
I guess I should have read the version 2 test before hitting "send"...
The v2 behavior is somewhat weird. What's even weirder is that you didn't need any todo_wine to make it work in Wine (whereas you need the todo_wine for the "sane" v1 behavior). Our DeleteAttachedSurface implementation sets the wined3d DS to NULL, at which point a D3DCLEAR_ZBUFFER clear should return an error. Do you have any idea why the depth buffer is still cleared on Wine?
afaik wined3d should return a WARN in wined3d_device_clear showing that the depth surface is missing and fail on clear. But it seems that despite the depth surface being deleted that wined3d is still able to use that depth surface, which is definitely weird. I will dig more into this and post here as I find anything.
Cheers, Aaryaman
On Sun, Aug 7, 2016 at 4:06 AM, Stefan Dösinger stefandoesinger@gmail.com wrote:
You could make this
{{-1.0f}, {-1.0f}, {0.0f}, 0, {0xffbada55}}, {{-1.0f}, { 1.0f}, {0.0f}, 0, {0xffbada55}}, {{ 1.0f}, {-1.0f}, {1.0f}, 0, {0xffbada55}}, {{ 1.0f}, { 1.0f}, {1.0f}, 0, {0xffbada55}},
Then your currently discarded draws will draw to ca x = 250. That way you can make sure the draw is really discarded by the depth test and not some other reason.
- /* Check what happens if we release the depth surface that d3drm
created, and clear the viewport */
...
- hr = IDirect3DRMViewport_Clear(viewport1);
- ok(SUCCEEDED(hr), "Cannot clear viewport (hr = %#x).\n", hr);
It's worth checking the color surface here to see if the clear was completely ignored or if it realized that there's no depth surface and it only cleared color. Also clear with a different color than what's left behind by the previous test.
At that point the surface is already cleared to red color (see the test above clear_depth_surface call here) and the depth test fails while drawing the quad, so even if the clear was ignored, the color would remain the same as if it was cleared. Hmm but if I use the new z-coords that you specified above, then it's possible.
...
- d3d_draw_quad1(d3d_device1, d3d_viewport);
- ret_color = get_surface_color(surface, 100, 200);
- todo_wine ok(compare_color(ret_color, 0x000000ff, 1), "Got
unexpected color 0x%08x.\n", ret_color); My reading of this is that the depth surface wasn't cleared (everything else would be very surprising). If the color surface was indeed cleared this shows that native at some place expects your ugly deed of removing the depth surface. If the color is not cleared it and the clear call is completely ignored I'd say something is broken somewhere and we shouldn't care too much about this corner case.
(See my reply to your second email).
- hr = IDirect3DRMFrame_SetSceneBackgroundRGB(frame1, 1.0f, 1.0f,
1.0f);
- ok(SUCCEEDED(hr), "Cannot set scene background RGB (hr = %#x)\n",
hr);
- ret_color = IDirect3DRMFrame_GetSceneBackground(frame1);
- ok(ret_color == 0xffffffff, "Expected scene color returned ==
0xffffffff, got %#x.\n", ret_color);
- hr = IDirect3DRMFrame_SetSceneBackgroundRGB(dummy_frame1, 0.0f,
1.0f, 0.0f);
- ok(SUCCEEDED(hr), "Cannot set scene background RGB (hr = %#x)\n",
hr);
- ret_color = IDirect3DRMFrame_GetSceneBackground(dummy_frame1);
- ok(ret_color == 0xff00ff00, "Expected scene color returned ==
0xff00ff00, got %#x.\n", ret_color);
- hr = IDirect3DRMFrame_SetSceneBackgroundRGB(dummy_frame1, 0.0f,
0.0f, 1.0f);
- ok(SUCCEEDED(hr), "Cannot set scene background RGB (hr = %#x)\n",
hr);
- ret_color = IDirect3DRMFrame_GetSceneBackground(dummy_frame1);
- ok(ret_color == 0xff0000ff, "Expected scene color returned ==
0xff0000ff, got %#x.\n", ret_color); Can't the "where does the color come from" test be merged into the clear test at the start?
It can. I just wanted to keep it separate
+static void draw_quad2(IDirect3DDevice2 *device, IDirect3DViewport
*viewport)
+{
Do you need to draw with IDirect3DDevice2 directly here? What about QI'ing IDirect3DDevice and calling d3d_draw_quad1?
Not really. I just thought that since d3drm is giving us access to IDirect3DDevice2 here, might as well use it directly. I could QI version 1 and use d3d_draw_quad1 if you want, though. I also think that the "clear without depth buffer" behavior differs because of the differences within the drawing behavior of each version in native, maybe. (hence the todo_wine at the version 1 clear tests after the depth buffer is detached).
- if (FAILED(hr = IDirect3DMaterial_GetHandle(viewport->material,
viewport->device->device, &hmat)))
return hr;- if (FAILED(hr = IDirect3DViewport_SetBackground(viewport->d3d_viewport,
hmat)))
return hr;You can do these two calls during viewport creation, you don't have to repeat them every time.
Makes sense. So the handle is assigned to the interface, not the material itself?
static HRESULT WINAPI d3drm_viewport2_Clear(IDirect3DRMViewport2
*iface, DWORD flags)
...
- if (flags & D3DRMCLEAR_ZBUFFER)
clear_flags |= D3DCLEAR_ZBUFFER;I guess somewhere here or in the IDirect3DViewport::Clear implementation in ddraw you'll have to handle your missing depth buffer case.
I can probaly do that later on once an application depends on this behavior. Handling this case right now would require me to keep two separate implementations. But if you feel that it's important to handle this, I don't mind keeping them separate.
I also agree with the rest of your review, and will make fixes accordingly.
Thanks for the review!
Cheers, Aaryaman
On 2016-08-07 00:28, Aaryaman Vasishta wrote:
At that point the surface is already cleared to red color (see the test above clear_depth_surface call here) and the depth test fails while drawing the quad, so even if the clear was ignored, the color would remain the same as if it was cleared. Hmm but if I use the new z-coords that you specified above, then it's possible.
That's why I said "Also clear with a different color than what's left behind by the previous test".
Can't the "where does the color come from" test be merged into the clear test at the start?
It can. I just wanted to keep it separate
I'd prefer to merge them, it will keep the overall test shorter and improve readability IMO. It's a mild preference though, feel free to keep them separate if you feel otherwise.
Do you need to draw with IDirect3DDevice2 directly here? What about QI'ing IDirect3DDevice and calling d3d_draw_quad1?
Not really. I just thought that since d3drm is giving us access to IDirect3DDevice2 here, might as well use it directly. I could QI version 1 and use d3d_draw_quad1 if you want, though.
That's a reasonable point. An application may do something similar, so it's worth mixing IDirect3DDevice (d3drm internally) and IDirect3DDevice2 (test). An argument could be made to put such an interop test into dlls/ddraw/tests/ddraw1.c, but this is not a priority for me at the moment. d3drm.dll is the most likely "cause" of such a mix. (A particularly interesting question is how IDirect3DDevice2::{Get,Set}RenderState interacts with D3DOP_STATERENDER)
Makes sense. So the handle is assigned to the interface, not the material itself?
That's how I understand the interface, and that's how we implement it. Obviously you can write an explicit test for this in dlls/ddraw/tests :-) .
My understanding is that the handle refers to the IDirect3DMaterial object. The D3DMATERIAL struct is stored inside that object. So if change the D3DMATERIAL anything that stores a handle to the modified IDirect3DMaterial will use the new colors.
If that were not the case you'd see IDirect3DMaterial::GetHandle return a new handle after you call SetMaterial. It would also open a memory management can of worms because the old material handle has to be kept around somewhere together with the old color information.
I can probaly do that later on once an application depends on this behavior. Handling this case right now would require me to keep two separate implementations. But if you feel that it's important to handle this, I don't mind keeping them separate.
Well, your test kinda relies on it, but as we've noticed succeeds for some reason we don't understand :-) .
You can also try to create a d3drm device that doesn't have a depth buffer at creation and look at the return value of Viewport2::Clear(D3DRMCLEAR_ZBUFFER). I would expect an error.
My guess is that Viewport1::Clear shouldn't unconditionally use D3DRMCLEAR_ALL, but instead check if a Z surface is available. If yes, it uses D3DRMCLEAR_ALL, otherwise it uses TARGET | DIRTYRECTS. (It might not use DIRTYRECTS - we don't really know what it does.)
-
Aaryaman Vasishta -
Stefan Dösinger