<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Aug 7, 2016 at 4:06 AM, Stefan Dösinger <span dir="ltr"><<a href="mailto:stefandoesinger@gmail.com" target="_blank">stefandoesinger@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">You could make this<br>
<br>
{{-1.0f}, {-1.0f}, {0.0f}, 0, {0xffbada55}},<br>
{{-1.0f}, { 1.0f}, {0.0f}, 0, {0xffbada55}},<br>
{{ 1.0f}, {-1.0f}, {1.0f}, 0, {0xffbada55}},<br>
{{ 1.0f}, { 1.0f}, {1.0f}, 0, {0xffbada55}},<br>
<br>
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.<br>
<span class=""><br>
> + /* Check what happens if we release the depth surface that d3drm created, and clear the viewport */<br>
</span>> ...<br>
<span class="">> + hr = IDirect3DRMViewport_Clear(<wbr>viewport1);<br>
> + ok(SUCCEEDED(hr), "Cannot clear viewport (hr = %#x).\n", hr);<br>
</span>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.<br></blockquote><div>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.<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> ...<br>
<span class="">> + d3d_draw_quad1(d3d_device1, d3d_viewport);<br>
> +<br>
> + ret_color = get_surface_color(surface, 100, 200);<br>
> + todo_wine ok(compare_color(ret_color, 0x000000ff, 1), "Got unexpected color 0x%08x.\n", ret_color);<br>
</span>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.<br></blockquote><div>(See my reply to your second email). <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> + hr = IDirect3DRMFrame_<wbr>SetSceneBackgroundRGB(frame1, 1.0f, 1.0f, 1.0f);<br>
> + ok(SUCCEEDED(hr), "Cannot set scene background RGB (hr = %#x)\n", hr);<br>
> + ret_color = IDirect3DRMFrame_<wbr>GetSceneBackground(frame1);<br>
> + ok(ret_color == 0xffffffff, "Expected scene color returned == 0xffffffff, got %#x.\n", ret_color);<br>
> + hr = IDirect3DRMFrame_<wbr>SetSceneBackgroundRGB(dummy_<wbr>frame1, 0.0f, 1.0f, 0.0f);<br>
> + ok(SUCCEEDED(hr), "Cannot set scene background RGB (hr = %#x)\n", hr);<br>
> + ret_color = IDirect3DRMFrame_<wbr>GetSceneBackground(dummy_<wbr>frame1);<br>
> + ok(ret_color == 0xff00ff00, "Expected scene color returned == 0xff00ff00, got %#x.\n", ret_color);<br>
> + hr = IDirect3DRMFrame_<wbr>SetSceneBackgroundRGB(dummy_<wbr>frame1, 0.0f, 0.0f, 1.0f);<br>
> + ok(SUCCEEDED(hr), "Cannot set scene background RGB (hr = %#x)\n", hr);<br>
> + ret_color = IDirect3DRMFrame_<wbr>GetSceneBackground(dummy_<wbr>frame1);<br>
> + ok(ret_color == 0xff0000ff, "Expected scene color returned == 0xff0000ff, got %#x.\n", ret_color);<br>
</span>Can't the "where does the color come from" test be merged into the clear test at the start?<br></blockquote><div>It can. I just wanted to keep it separate <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> +static void draw_quad2(IDirect3DDevice2 *device, IDirect3DViewport *viewport)<br>
> +{<br>
</span>Do you need to draw with IDirect3DDevice2 directly here? What about QI'ing IDirect3DDevice and calling d3d_draw_quad1? <br></blockquote><div>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.<br></div><div>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).<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><span class=""></span><br><span class=""></span></div></div><span class="">
> + if (FAILED(hr = IDirect3DMaterial_GetHandle(<wbr>viewport->material, viewport->device->device, &hmat)))<br>
> + return hr;<br>
> +<br>
> + if (FAILED(hr = IDirect3DViewport_<wbr>SetBackground(viewport->d3d_<wbr>viewport, hmat)))<br>
> + return hr;<br>
</span>You can do these two calls during viewport creation, you don't have to repeat them every time.<br></blockquote><div>Makes sense. So the handle is assigned to the interface, not the material itself? <span class=""></span><span class=""></span><br><span class=""></span></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> static HRESULT WINAPI d3drm_viewport2_Clear(<wbr>IDirect3DRMViewport2 *iface, DWORD flags)<br>
</span>> ...<br>
<span class="">> + if (flags & D3DRMCLEAR_ZBUFFER)<br>
> + clear_flags |= D3DCLEAR_ZBUFFER;<br>
</span>I guess somewhere here or in the IDirect3DViewport::Clear implementation in ddraw you'll have to handle your missing depth buffer case. <br></blockquote><div>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.<br><br></div><div>I also agree with the rest of your review, and will make fixes accordingly.<br><br></div><div>Thanks for the review!<br><br></div><div>Cheers,<br></div><div>Aaryaman <br></div></div><br></div></div>