d3drm patch feedback

[Stefan Dösinger]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

Here's the feedback for the patch you posted on IRC:
> - GUID driver = IID_IDirect3DRGBDevice;

Do you need this? Can't you just pass &IID_IDirect3DRGBDevice to CreateDeviceFromClipper? It may generate a const warning though.
> - hr = DirectDrawCreateClipper(0, &clipper, NULL);
> - ok(hr == DD_OK, "Cannot get IDirectDrawClipper interface (hr = %x).\n", hr);
> - hr = IDirectDrawClipper_SetHWnd(clipper, 0, window);
> - ok(hr == DD_OK, "Cannot set HWnd to Clipper (hr = %x).\n", hr);

Can you pass a NULL clipper? If this crashes, just put a comment in the test.

What if the clipper has no HWND and no clip list?

Call GetClipper on the render target and see if the clipper is assigned.
> - hr = IDirect3DRM2_CreateDeviceFromClipper(d3drm2, clipper, &driver, 300, 200, &device2);

For the sake of completeness pass 0 width and height. I expect one of the two results: Either it returns an error, or d3drm starts to use the window dimensions for the render target now.
> - todo_wine ok(ref2 > ref1, "expected ref2 > ref1, got ref1 = %d , ref2 = %d\n", ref1, ref2);

Minor detail: Use %u for unsigned.
> - ok((surface_desc.dwWidth == 300) && (surface_desc.dwHeight == 200), "Expected surface dimentions = 300, 200, got %d, %d.\n",
> -
>    surface_desc.dwWidth, surface_desc.dwHeight);
> - ok((surface_desc.ddsCaps.dwCaps & (DDSCAPS_OFFSCREENPLAIN | DDSCAPS_3DDEVICE)) == (DDSCAPS_OFFSCREENPLAIN | DDSCAPS_3DDEVICE),
> -
>    "Expected caps containing %x, got %x.\n", DDSCAPS_OFFSCREENPLAIN | DDSCAPS_3DDEVICE, surface_desc.ddsCaps.dwCaps);
> - expected_flags = DDSD_PIXELFORMAT | DDSD_CAPS | DDSD_WIDTH | DDSD_HEIGHT | DDSD_PITCH;
> - ok(surface_desc.dwFlags == expected_flags, "Expected %x for flags, got %x.\n", expected_flags, surface_desc.dwFlags);

Test the pixel format here. First query the current bpp with EnumDisplaySettings and check that the pixel format matches the screen properties.
> - /* Check if ddraw supports change of display mode */

The comment is somewhat outdated. Either remove it or say something like "Test if the render target format follows the screen format".
> - ok(surface_desc.ddpfPixelFormat.dwRGBBitCount == 16, "Expected 16bpp, got %dbpp.\n",
> -
>        surface_desc.ddpfPixelFormat.dwRGBBitCount);

Similar to the %u -> %u above

Once these are fixed the patch can go to wine-patches as far as I'm concerned.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBAgAGBQJVgt8QAAoJEN0/YqbEcdMwOTYP/jXwDY+gUs/zmAyqB5hJp8by
FO0YSZsi0Z3QMMzfqenCy9+/PmqjMHyRPQvhM8CFpcdWsUi8bH6PzTiooAYOcvzf
GXEnj46T1O0wiJfLK+RnzfvYNzITLWf2XK6ek1OtP1z6Y3JhADXwoLW/tSM+kKp7
Crih31t9ewiSMG5iebcSdviI59WrFtUHmS9Mdp5UnWVQB8smZII8c+VAOf/SmWZ1
cGKosFncJVNuXpF0fq5gWiy3m1VcJ1xoWsG1rXbrOzVFvpw+20twa1FgZm9Z6I3c
Pxj2prH2Lz9u7tDZUr75dg3ucRPcUe2QP2bTwwBDDq4oaUZYKgXBLrz94AVZ/Y+j
datcVFY7KwY2byGmxmO+0I+iWyj26CIg13jo20gqfRfjt92DFlfi5/Mm/e97rCa1
0V38s0kqN5o4rvU+QbUwHfvHIgr4kAFWZPjDmZd3YececQuoeC7FszND/43UD0tK
MTd1Na3hFa5m6zO7oe4fPfGYX55tVhEzphiRkH4K0R0ZTxRsZGGqQoSFAHeAJDH6
0hPd3XjVHxSpHQNd8/8aFeOMJ8pBR+4MRUMcFBDn0XlDUYw0kUdvRkttKoNVCSGw
H0BS00wj/fs/G4D/pCLEFEX7wF9qDM5O6xKQRoyXG3+i9oN+zdzCNMs6SV1J2Bci
Jedz7Aw7+yAByiH/GePR
=aa6D
-----END PGP SIGNATURE-----

[Aaryaman Vasishta]

On Thu, Jun 18, 2015 at 8:39 PM, Stefan Dösinger 
wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hi,
>
> Here's the feedback for the patch you posted on IRC:
> > - GUID driver = IID_IDirect3DRGBDevice;
>
> Do you need this? Can't you just pass &IID_IDirect3DRGBDevice to
> CreateDeviceFromClipper? It may generate a const warning though.

It does generate the const warning :)
> What if the clipper has no HWND and no clip list?

It will still create the device, though nothing will be shown on the window
which is displayed (just checked on a sample program on windows).

I'll do the rest of the changes and post to wine-patches for final review.

Thank you!