Re: [PATCH v2 0/2] MR3431: wined3d: Fix clears of the current render target in Vulkan

[Henri Verbeet (＠hverbeet)]

Mostly just a couple of style comments; feel free to disregard if you like:

The subject line, comments, and ok messages are missing full stops.

Personally, I'm not a fan of the switch/case style of tests. My preference would be to either just explicitly write out the tests, or to encode them using the "tests[]" construction we use in a couple of other tests.
> ```diff
> +    static const D3D11_VIEWPORT viewport = {0, 0, 2, 1, 0, 1};
>  ...
> +    texture_desc.Width = (UINT)viewport.Width;
> +    texture_desc.Height = (UINT)viewport.Height;
>  ...
> +        ID3D11DeviceContext_RSSetViewports(context, 1, &viewport);
> ```

That works, but more typically we'd initialise "texture_desc", and then call "set_viewport(..., texture_desc.Width, texture_desc.Height, ...)".
> ```diff
> +    int test;
> ```

"test" can't be negative, right?
> ```diff
> +        unsigned int i;
> +        static const UINT vertex_stride = sizeof(depth_quarter_quad_data[0]);
> +        static const UINT vertex_offset = 0;
> +        static const struct vec4 initial_color = {0, 0, 0, 1};
> +        static const float clear_color[] = {0, 1, 0, 1};
> +        DWORD expected_color[] = {0xff000000, 0xff000000};
> +        FLOAT expected_depth[] = {0.5f, 0.5f};
> ```

The choice for "FLOAT" seems odd, given that get_readback_float() returns a float. We don't tend to use UINT and DWORD a lot either.
> ```diff
> +        for (i = 0; i < ARRAYSIZE(expected_color); i++)
> ```

It's not wrong, but all the existing code uses ARRAY_SIZE...
> ```diff
> +                (!damavand && test == 3 && i == 0) /* Draw with no PS doesn't write color on D3D11 but does on OpenGL */
> ```

I don't think the comment is terribly clear, but it sounds like we're getting fixed-function with the GL backend, and things just happen to work with the Vulkan backend because we don't yet have a fixed-function implementation there. That likely won't last forever though. For comparison, vkd3d handles this explicitly in d3d12_pipeline_state_init_graphics().
> ```diff
> +            ok(depth == expected_depth[i], "Test %d: Got unexpected depth %.2f at %u\n", test, depth, i);
> ```

I don't think %.2f is quite sufficient to show what's going on if the result is only off by a couple of ulps. That's perhaps not terribly likely for clears with carefully chosen depth values, but it still looks a little sketchy to me.
> ```diff
> +static void wined3d_context_prepare_used_framebuffer(struct wined3d_rendertarget_view *rtv, struct wined3d_context *context)
> +{
> +    if (wined3d_rendertarget_view_get_locations(rtv) & WINED3D_LOCATION_CLEARED)
> +    {
> +        /* Need to rebind framebuffer or the clear won't happen */
> +        context_invalidate_state(context, STATE_FRAMEBUFFER);
> +        wined3d_rendertarget_view_prepare_location(rtv, context, rtv->resource->draw_binding);
> +    }
> +    else
> +    {
> +        wined3d_rendertarget_view_load_location(rtv, context, rtv->resource->draw_binding);
> +    }
> +}
> ```

The comment doesn't seem quite as helpful as it perhaps could be; if I understand the issue correctly, we don't actually care about rebinding the framebuffer, but instead about starting a new render pass, because that's where we'll clear these. That's also what the commit message seems to suggest.

By convention, "context" would be the first parameter for wined3d_context_*() functions. The function name is also slightly odd, because it's preparing/loading individual views, not complete framebuffers.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/3431#note_40852