Re: [PATCH 2/2] d3drm: Implement IDirect3DRMViewport*::Clear.

[Aaryaman Vasishta]

On Sun, Aug 7, 2016 at 4:06 AM, Stefan Dösinger 
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