d3drm patch feedback

Thu Jun 18 10:09:04 CDT 2015

-----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.