Re: [PATCH] wined3d: Restore WINEDDBLT_ALPHATEST support.

[Henri Verbeet]

On 7 December 2015 at 23:36, Stefan Dösinger  wrote:
> -static void arbfp_blit_surface(struct wined3d_device *device, DWORD filter,
> +static void arbfp_blit_surface(enum wined3d_blit_op op, struct wined3d_device *device, DWORD filter,

I think I'd prefer "device" to stay the first parameter for
consistency with the other blitter ops. It's probably not a big deal
though.
> @@ -7881,6 +7885,9 @@ static void arbfp_blit_surface(struct wined3d_device *device, DWORD filter,
>      /* Leave the opengl state valid for blitting */
>      arbfp_blit_unset(context->gl_info);
> - if (op == WINED3D_BLIT_OP_COLOR_BLIT_ALPHATEST)
> -
>    glDisable(GL_ALPHA_TEST);
> -

You shouldn't need this. context_apply_blit_state() invalidates
WINED3D_RS_ALPHATESTENABLE, but even if it didn't, disabling the alpha
test isn't necessarily the right thing to do. Not that
arbfp_blit_surface() necessarily needs all the invalidation
context_apply_blit_state() does though.
> @@ -4335,8 +4336,16 @@ static void ffp_blit_blit_surface(struct wined3d_device *device, DWORD filter,
>      wined3d_texture_set_color_key(src_surface->container, WINED3D_CKEY_SRC_BLT, color_key);
>  context = context_acquire(device, dst_surface);
> -
> - if (op == WINED3D_BLIT_OP_COLOR_BLIT_ALPHATEST)
> -
>    glEnable(GL_ALPHA_TEST);
> - surface_blt_to_drawable(device, context, filter,
>            !!color_key, src_surface, src_rect, dst_surface, dst_rect);
> -
> - if (op == WINED3D_BLIT_OP_COLOR_BLIT_ALPHATEST)
> -
>    glDisable(GL_ALPHA_TEST);
> -

Like above.

Some basic tests probably wouldn't hurt. If we had had any the
regression might not have happened in the first place.

[PATCH] wined3d: Restore WINEDDBLT_ALPHATEST support.

[Stefan Dösinger]

> Am 08.12.2015 um 10:42 schrieb Henri Verbeet :
>
> You shouldn't need this. context_apply_blit_state() invalidates
> WINED3D_RS_ALPHATESTENABLE, but even if it didn't, disabling the alpha
> test isn't necessarily the right thing to do. Not that
> arbfp_blit_surface() necessarily needs all the invalidation
> context_apply_blit_state() does though.

Notice the last_was_blit check. Future blits might break if we keep the alpha test on.

We could discuss more fine-grained state tracking for blits, but I don't think 1.8 is the time for that, and even beyond that I don't think it's worth it. GLSL would just swap most of them by selecting its shader. I can do the same in ARB, but I don't think it matters either way.
> Some basic tests probably wouldn't hurt. If we had had any the
> regression might not have happened in the first place.

Agreed, but I probably won't get around to writing some before the weekend. The nature of the software cursor makes it a bit difficult. You can spot it via GetDC(NULL), but not through GetFrontBufferData on native. I'll see if I can find a way to write the test that doesn't fail on OSX by design. Also, Fullscreen only, windows doesn't do SW cursors in windowed mode. Alexandre will love another fullscreen test :-) .

[PATCH] wined3d: Restore WINEDDBLT_ALPHATEST support.

[Henri Verbeet]

On 9 December 2015 at 00:39, Stefan Dösinger  wrote:
> > Am 08.12.2015 um 10:42 schrieb Henri Verbeet :
> > You shouldn't need this. context_apply_blit_state() invalidates
> > WINED3D_RS_ALPHATESTENABLE, but even if it didn't, disabling the alpha
> > test isn't necessarily the right thing to do. Not that
> > arbfp_blit_surface() necessarily needs all the invalidation
> > context_apply_blit_state() does though.
>
> Notice the last_was_blit check. Future blits might break if we keep the alpha test on.

Right. Can't say I particularly like it, but that's how it works.
> We could discuss more fine-grained state tracking for blits, but I don't think 1.8 is the time for that, and even beyond that I don't think it's worth it. GLSL would just swap most of them by selecting its shader. I can do the same in ARB, but I don't think it matters either way.

Yeah. I think context_apply_blit_state() only makes sense for the
fixed function blitter, but it's not a big issue.
> > Some basic tests probably wouldn't hurt. If we had had any the
> > regression might not have happened in the first place.
>
> Agreed, but I probably won't get around to writing some before the weekend. The nature of the software cursor makes it a bit difficult. You can spot it via GetDC(NULL), but not through GetFrontBufferData on native. I'll see if I can find a way to write the test that doesn't fail on OSX by design. Also, Fullscreen only, windows doesn't do SW cursors in windowed mode. Alexandre will love another fullscreen test :-) .

Oh, I somehow thought DDBLT_ALPHATEST was a real thing, in which case
it would have been easy to write some basic ddraw tests. Since it
isn't though, I have to wonder if you wouldn't get pretty much the
same effect by just using a color keyed blit and setting the color key
values to 0x00000000 and 0x00ffffff?

[PATCH] wined3d: Restore WINEDDBLT_ALPHATEST support.

[Stefan Dösinger]

> Am 09.12.2015 um 10:09 schrieb Henri Verbeet :
>
> Oh, I somehow thought DDBLT_ALPHATEST was a real thing, in which case
> it would have been easy to write some basic ddraw tests.

There's DDBLT_ALPHASRC and stuff, but I haven't seen it supported anywhere.
> Since it
> isn't though, I have to wonder if you wouldn't get pretty much the
> same effect by just using a color keyed blit and setting the color key
> values to 0x00000000 and 0x00ffffff?

Right, that will work inside the ARB blitter and would avoid the glEnable / glDisable there. It won't work for the fixed function blitter because it doesn't support range color keys, so we'll need the special WINEDDBLT_ALPHATEST. (And no, I think supporting range color keys inside the fixed function blitter to generate a no-op conversion and so on for the pointer is fragile overkill.)