Re: [v2 2/6] d3dx9: implement simple effect states application.

[Matteo Bruni]

2016-03-01 18:09 GMT+01:00 Paul Gofman :
> Signed-off-by: Paul Gofman
>
> dlls/d3dx9_36/effect.c       | 111 +++++++++++++++++++++++++++++++++++++++----
>  dlls/d3dx9_36/tests/effect.c |  16 +++----
>  2 files changed, 110 insertions(+), 17 deletions(-)
>
> diff --git a/dlls/d3dx9_36/effect.c b/dlls/d3dx9_36/effect.c
> index f0bbd0b..7ccdb36 100644
> --- a/dlls/d3dx9_36/effect.c
> +++ b/dlls/d3dx9_36/effect.c
> @@ -2490,6 +2490,102 @@ static HRESULT d3dx9_base_effect_set_array_range(struct d3dx9_base_effect *base,
>      return E_NOTIMPL;
>  }
>
> +static HRESULT d3dx9_get_param_value_ptr(struct ID3DXEffectImpl *effect, struct d3dx_pass *pass, struct d3dx_state *state, void **param_value, struct d3dx_parameter **out_param)

Please break lines so they stay under 100 - 120 characters.
> +{
> - struct d3dx_parameter *param;
> - param = state->parameter.referenced_param ? state->parameter.referenced_param : &state->parameter;
> -
> - switch (state->type)
> - {
> -
>    case ST_CONSTANT:
> -
>    case ST_PARAMETER:
> -
>        *param_value = param->data;
> -
>        *out_param = param;
> -
>        return D3D_OK;
> -
>    case ST_ARRAY_SELECTOR:
> -
>        FIXME("Array selector.\n");
> -
>        break;
> -
>    case ST_FXLC:
> -
>        FIXME("FXLC not supported yet.\n");
> -
>        break;
> - }
> - *param_value = NULL;
> - *out_param = NULL;
> - return E_NOTIMPL;
>
> +}
> -
>
> +static HRESULT d3dx9_apply_state(struct ID3DXEffectImpl *effect, struct d3dx_pass *pass, struct d3dx_state *state)
> +{
> - IDirect3DDevice9 *device = effect->device;
> - struct d3dx_parameter *param;
> - void *param_value;
> - HRESULT hr;
> -
> - TRACE("operation %u, index %u, type %u.\n", state->operation, state->index, state->type);
> - hr = d3dx9_get_param_value_ptr(effect, pass, state, &param_value, &param);
> - if (FAILED(hr))
> -
>    return hr;
> -
> - switch (state_table[state->operation].class)
> - {
> -
>    case SC_RENDERSTATE:
> -
>        TRACE("%s: operation %u value %u.\n", state_table[state->operation].name, state_table[state->operation].op, *(DWORD *)param_value);

You're not being entirely consistent with those traces. I'd use the
style you're using here throughout the file but without the ':'.
> -
>        return IDirect3DDevice9_SetRenderState(device, state_table[state->operation].op, *(DWORD *)param_value);
> -
>    case SC_FVF:
> -
>        TRACE("%s: value %#x.\n", state_table[state->operation].name, *(DWORD *)param_value);
> -
>        return IDirect3DDevice9_SetFVF(device, *(DWORD *)param_value);
> -
>    case SC_TEXTURESTAGE:
> -
>        TRACE("%s: stage %u value %u.\n", state_table[state->operation].name, state->index, *(DWORD *)param_value);
> -
>        return IDirect3DDevice9_SetTextureStageState(device, state->index, state_table[state->operation].op, *(DWORD *)param_value);
> -
>    case SC_VERTEXSHADER:
> -
>        TRACE("%s vertex shader %p.\n", state_table[state->operation].name, *(IDirect3DVertexShader9 **)param_value);
> -
>        hr = IDirect3DDevice9_SetVertexShader(device, *(IDirect3DVertexShader9 **)param_value);
> -
>        if (FAILED(hr))
> -
>            ERR("Could not set vertex shader.\n");
> -
>        FIXME("Not executing preshader and not setting constants.\n");
> -
>        return hr;
> -
>    case SC_PIXELSHADER:
> -
>        TRACE("%s pixel shader %p.\n", state_table[state->operation].name, *(IDirect3DPixelShader9 **)param_value);
> -
>        hr = IDirect3DDevice9_SetPixelShader(device, *(IDirect3DPixelShader9 **)param_value);
> -
>        if (FAILED(hr))
> -
>            ERR("Could not set pixel shader.\n");
> -
>        FIXME("Not executing preshader and not setting constants.\n");
> -
>        return hr;
> -
>    case SC_TRANSFORM:
> -
>        TRACE("%s %u.\n", state_table[state->operation].name, state->index);
> -
>        return IDirect3DDevice9_SetTransform(device, state_table[state->operation].op + state->index, (D3DMATRIX *)param_value);
> -
>    case SC_NPATCHMODE:
> -
>        TRACE("%s %f.\n", state_table[state->operation].name, *(float *)param_value);
> -
>        return IDirect3DDevice9_SetNPatchMode(device, *(float *)param_value);
> -
>    default:
> -
>        FIXME("%s not handled.\n", state_table[state->operation].name);
> -
>        break;
> - }
> - return D3D_OK;
>
> +}
> -
>
> +static HRESULT d3dx9_apply_pass_states(struct ID3DXEffectImpl *effect, struct d3dx_pass *pass)
> +{
> - UINT i;

I'd prefer "unsigned int" for these kind of loop variables, here and
elsewhere in those patches.
> - HRESULT ret;
> -
> - TRACE("Effect %p, pass %p, state_count=%u.\n", effect, pass, pass->state_count);

Please drop the '='. While at it, use lowercase for "effect".
> -
> - ret = D3D_OK;
> - for (i = 0; i < pass->state_count; i++)
> - {
> -
>    HRESULT hr;
> -
> -
>    hr = d3dx9_apply_state(effect, pass, &pass->states[i]);
> -
>    if (FAILED(hr))
> -
>    {
> -
>        WARN("Error applying state: %#x.\n", hr);
> -
>        ret = hr;
> -
>    }
> - }
> - return ret;
>
> +}
> -
>
> static inline struct ID3DXEffectImpl *impl_from_ID3DXEffect(ID3DXEffect *iface)
>  {
>      return CONTAINING_RECORD(iface, struct ID3DXEffectImpl, ID3DXEffect_iface);
> @@ -3175,18 +3271,15 @@ static HRESULT WINAPI ID3DXEffectImpl_Begin(ID3DXEffect *iface, UINT *passes, DW
>
> static HRESULT WINAPI ID3DXEffectImpl_BeginPass(ID3DXEffect *iface, UINT pass)
>  {
> - struct ID3DXEffectImpl *This = impl_from_ID3DXEffect(iface);
> - struct d3dx_technique *technique = This->active_technique;
> - struct ID3DXEffectImpl *effect = impl_from_ID3DXEffect(iface);
> - struct d3dx_technique *technique = effect->active_technique;
> - TRACE("iface %p, pass %u\n", This, pass);
> - TRACE("iface %p, pass %u\n", effect, pass);
> - if (technique && pass < technique->pass_count && !This->active_pass)
> - if (technique && pass < technique->pass_count && !effect->active_pass)
> {
> -
>    This->active_pass = &technique->passes[pass];
> -
> -
>    FIXME("No states applied, yet!\n");
> -
> -
>    return D3D_OK;
> -
>    effect->active_pass = &technique->passes[pass];
> -
>    return d3dx9_apply_pass_states(effect, effect->active_pass);
>
> }
>
> WARN("Invalid argument supplied.\n");
>
> diff --git a/dlls/d3dx9_36/tests/effect.c b/dlls/d3dx9_36/tests/effect.c
> index 6dd3e00..e415a3c 100644
> --- a/dlls/d3dx9_36/tests/effect.c
> +++ b/dlls/d3dx9_36/tests/effect.c
> @@ -2875,11 +2875,11 @@ static void test_effect_states(IDirect3DDevice9 *device)
>      ok(mat.m[0][0] == test_mat.m[0][0], "Unexpected value: %f.\n", mat.m[0][0]);
>  hr = effect->lpVtbl->BeginPass(effect, 0);
> - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK).\n", hr);
> -
>
> todo_wine ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK).\n", hr);
>
> hr = IDirect3DDevice9_GetTransform(device, D3DTS_WORLDMATRIX(1), &mat);
> ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK).\n", hr);
> - todo_wine ok(!memcmp(mat.m, test_mat_world1.m, sizeof(mat)), "World matrix does not match.\n");
> -
>
> ok(!memcmp(mat.m, test_mat_world1.m, sizeof(mat)), "World matrix does not match.\n");
>
> hr = IDirect3DDevice9_GetTransform(device, D3DTS_VIEW, &mat);
> ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK).\n", hr);
>
> @@ -2887,11 +2887,11 @@ static void test_effect_states(IDirect3DDevice9 *device)
>  hr = IDirect3DDevice9_GetRenderState(device, D3DRS_BLENDOP, &value);
>  ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK).\n", hr);
> - todo_wine ok(value == 2, "Got result %u, expected %u\n", value, 2);
> -
>
> ok(value == 2, "Got result %u, expected %u\n", value, 2);
>
> hr = IDirect3DDevice9_GetVertexShader(device, &vshader);
> ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK).\n", hr);
> - todo_wine ok(vshader != NULL, "Got NULL vshader.\n");
> - ok(vshader != NULL, "Got NULL vshader.\n");
> if (vshader)
> {
>     byte_code_size = sizeof(test_effect_states_vshader_buf);
>
> @@ -2921,11 +2921,11 @@ static void test_effect_states(IDirect3DDevice9 *device)
>      ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK).\n", hr);
>      hr = IDirect3DDevice9_GetRenderState(device, D3DRS_BLENDOP, &value);
>      ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK).\n", hr);
> - todo_wine ok(value == 2, "Got result %u, expected %u\n", value, 2);
> -
>
> ok(value == 2, "Got result %u, expected %u\n", value, 2);
>
> hr = IDirect3DDevice9_GetRenderState(device, D3DRS_ZENABLE, &value);
> ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK).\n", hr);
> - todo_wine ok(value,"Got result %u, expected TRUE.\n", value);
> -
>
> ok(value,"Got result %u, expected TRUE.\n", value);
>
> hr = IDirect3DDevice9_GetSamplerState(device, 1, D3DSAMP_MIPFILTER, &value);
> ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK).\n", hr);
>
> @@ -2933,14 +2933,14 @@ static void test_effect_states(IDirect3DDevice9 *device)
>  hr = IDirect3DDevice9_GetTextureStageState(device, 3, D3DTSS_ALPHAOP, &value);
>  ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK).\n", hr);
> - todo_wine ok(value == 4, "Unexpected texture stage 3 AlphaOp %u.\n", value);
> -
>
> ok(value == 4, "Unexpected texture stage 3 AlphaOp %u.\n", value);
>
> hr = effect->lpVtbl->End(effect);
> ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK).\n", hr);
>
> hr = IDirect3DDevice9_GetTransform(device, D3DTS_WORLDMATRIX(1), &mat);
> ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK).\n", hr);
> - ok(!memcmp(mat.m, test_mat.m, sizeof(mat)), "World matrix not restored.\n");
> -
>
> todo_wine ok(!memcmp(mat.m, test_mat.m, sizeof(mat)), "World matrix not restored.\n");
>
> hr = IDirect3DDevice9_GetLightEnable(device, 2, &bval);
> todo_wine ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK).\n", hr);
>
> --
> 2.5.0