Re: [PATCH] d3dx9: implement effect state management.

[Matteo Bruni]

2016-02-29 19:08 GMT+01:00 Paul Gofman :
> Preshaders (and FXLC-type constants requiring preshaders) are TODOs (on the way).
> Signed-off-by: Paul Gofman
>
> dlls/d3dx9_36/effect.c       | 375 +++++++++++++++++++++++++++++++++++++++++--
>  dlls/d3dx9_36/tests/effect.c | 241 +++++++++++++++++++++++++++
>  2 files changed, 606 insertions(+), 10 deletions(-)

You should split this patch into smaller pieces. One way to do that is
to introduce the various state classes one at a time into
d3dx9_apply_state (or, not everything at once, anyway). The state
saving / restoring should also be a separate patch.
WRT the tests, they should probably be split out too. You can put them
before the implementation, inserting and removing "todo_wine" where
necessary, or after. If it's not too complicated the first option is
generally preferred.
BTW it turns out I have a similar test somewhere in my queue. I'll
have another look and I might send it if it still makes sense to.

I'm leaving a few comments inline in the patch, below, mostly style
issues I noticed at a quick look. Many of them apply multiple times
through the patch. In general, make sure to be consistent with code
style.

Oh, and thank you for working on this!
> diff --git a/dlls/d3dx9_36/effect.c b/dlls/d3dx9_36/effect.c
> index f0bbd0b..33e063a 100644
> --- a/dlls/d3dx9_36/effect.c
> +++ b/dlls/d3dx9_36/effect.c
> @@ -162,6 +162,8 @@ struct d3dx_technique
>  struct d3dx_parameter *annotations;
>  struct d3dx_pass *passes;
> -
> - struct IDirect3DStateBlock9 *saved_state;
>
> };
>
> struct param_table
> @@ -642,6 +644,12 @@ static void free_technique(struct d3dx_technique *technique)
>      if (!technique)
>          return;
> - if (technique->saved_state)
> - {
> -
>    IDirect3DStateBlock9_Release(technique->saved_state);
> -
>    technique->saved_state = NULL;
> - }
> - if (technique->annotations)
>    {
>        for (i = 0; i < technique->annotation_count; ++i)
>
> @@ -2490,6 +2498,332 @@ 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 *This, struct d3dx_pass *pass, struct d3dx_state *state, void **param_value, struct d3dx_parameter **out_param)
> +{
> - struct d3dx_parameter *param;
> - param = state->parameter.referenced_param ? state->parameter.referenced_param : &state->parameter;
> -
> - switch(state->type)

Space after the switch keyword.
> - {
> -
>    case ST_CONSTANT:
> -
>    case ST_PARAMETER:
> -
>        *param_value = param->data;
> -
>        *out_param = param;
> -
>        return D3D_OK;
> -
>    case ST_ARRAY_SELECTOR:
> -
>        FIXME("array selector\n");

Uppercase at the start and period at the end of the message.
> -
>        break;
> -
>    case ST_FXLC:
> -
>        FIXME("FXLC not supported yet\n");
> -
>        break;
> - }
> - *param_value = NULL;
> - *out_param = NULL;
> - return E_NOTIMPL;
>
> +}
> -
>
> +void d3dx9_set_light_parameter(enum LIGHT_TYPE op, D3DLIGHT9 *light, void *value)
> +{
> - static struct
> - {
> -
>    int off;
> -
>    const char *name;
> - } light_tbl[] =
> - {
> -
>   {offsetof(D3DLIGHT9, Type),         "LC_TYPE"},

You probably want to use FIELD_OFFSET instead.
> -
>   {offsetof(D3DLIGHT9, Diffuse),      "LT_DIFFUSE"},
> -
>   {offsetof(D3DLIGHT9, Specular),     "LT_SPECULAR"},
> -
>   {offsetof(D3DLIGHT9, Ambient),      "LT_AMBIENT"},
> -
>   {offsetof(D3DLIGHT9, Position),     "LT_POSITION"},
> -
>   {offsetof(D3DLIGHT9, Direction),    "LT_DIRECTION"},
> -
>   {offsetof(D3DLIGHT9, Range),        "LT_RANGE"},
> -
>   {offsetof(D3DLIGHT9, Falloff),      "LT_FALLOFF"},
> -
>   {offsetof(D3DLIGHT9, Attenuation0), "LT_ATTENUATION0"},
> -
>   {offsetof(D3DLIGHT9, Attenuation1), "LT_ATTENUATION1"},
> -
>   {offsetof(D3DLIGHT9, Attenuation2), "LT_ATTENUATION2"},
> -
>   {offsetof(D3DLIGHT9, Theta),        "LT_THETA"},
> -
>   {offsetof(D3DLIGHT9, Phi),          "LT_PHI"}
> - };
> - switch(op)
> - {
> -
>    case LT_TYPE:
> -
>        TRACE("LT_TYPE %d\n", *(D3DLIGHTTYPE*)value);

Please put a space between the type and the '*' in the cast.
> -
>        light->Type = *(D3DLIGHTTYPE*)value;
> -
>        break;
> -
>    case LT_DIFFUSE:
> -
>    case LT_SPECULAR:
> -
>    case LT_AMBIENT:
> -
>    {
> -
>        D3DCOLORVALUE c = *(D3DCOLORVALUE*)value;
> -
> -
>        TRACE("%s (%f %f %f %f)\n", light_tbl[op].name, c.r, c.g, c.b, c.a);
> -
>        *(D3DCOLORVALUE*)((char*)light + light_tbl[op].off) = c;
> -
>        break;
> -
>    }
> -
>    case LT_POSITION:
> -
>    case LT_DIRECTION:
> -
>    {
> -
>        D3DVECTOR v = *(D3DVECTOR*)value;
> -
> -
>        TRACE("%s (%f %f %f)\n", light_tbl[op].name, v.x, v.y, v.z);
> -
>        *(D3DVECTOR*)((char*)light + light_tbl[op].off) = v;
> -
>        break;
> -
>    }
> -
>    case LT_RANGE:
> -
>    case LT_FALLOFF:
> -
>    case LT_ATTENUATION0:
> -
>    case LT_ATTENUATION1:
> -
>    case LT_ATTENUATION2:
> -
>    case LT_THETA:
> -
>    case LT_PHI:
> -
>    {
> -
>        float v = *(float*)value;
> -
>        TRACE("%s %f\n", light_tbl[op].name, v);
> -
>        *(float*)((char*)light + light_tbl[op].off) = v;
> -
>        break;
> -
>    }
> -
>    default:
> -
>        FIXME("unknown light parameter %d\n",op);
> -
>        break;
> - }
>
> +}
> -
>
> +void d3dx9_set_material_parameter(enum MATERIAL_TYPE op, D3DMATERIAL9 *material, void *value)
> +{
> - static struct

Please make it const too, if that works (I guess it might not).
> - {
> -
>    int off;
> -
>    const char *name;
> - } material_tbl[] =
> - {
> -
>   {offsetof(D3DMATERIAL9, Diffuse),  "MT_DIFFUSE"},
> -
>   {offsetof(D3DMATERIAL9, Ambient),  "MT_AMBIENT"},
> -
>   {offsetof(D3DMATERIAL9, Specular), "MT_SPECULAR"},
> -
>   {offsetof(D3DMATERIAL9, Emissive), "MT_EMISSIVE"},
> -
>   {offsetof(D3DMATERIAL9, Power),    "MT_POWER"}
> - };
> -
> - if (op < 0 || op > MT_POWER)
> - {
> -
>    FIXME("unknown material parameter %d\n",op);
> -
>    return ;

Stray whitespace.
> - }
> -
> - switch(op)
> - {
> -
>    case MT_POWER:
> -
>    {
> -
>        float v = *(float*)value;
> -
> -
>        TRACE("%s %f\n", material_tbl[op].name, v);
> -
>        material->Power = v;
> -
>        break;
> -
>    }
> -
>    default:
> -
>    {
> -
>        D3DCOLORVALUE c = *(D3DCOLORVALUE*)value;
> -
> -
>        TRACE("%s (%f %f %f %f)\n", material_tbl[op].name, c.r, c.g, c.b, c.a);
> -
>        *(D3DCOLORVALUE*)((char*)material + material_tbl[op].off) = c;
> -
>        break;
> -
>    }
> - }
>
> +}
> -
>
> +HRESULT d3dx_set_shader_const_state(IDirect3DDevice9 *device, enum SHADER_CONSTANT_TYPE op, int index, struct d3dx_parameter *param, void *value_ptr)

Probably index should be unsigned int.
> +{
> - static struct {

'{' should be on its own line.
> -
>    D3DXPARAMETER_TYPE type;
> -
>    int elem_size;
> -
>    const char *name;
> - } const_tbl[] =
> - {
> -
>    {D3DXPT_FLOAT, sizeof(float)*4, "SCT_VSFLOAT"},

Space before and after the binary operator.
> -
>    {D3DXPT_BOOL, sizeof(BOOL),     "SCT_VSBOOL"},
> -
>    {D3DXPT_INT, sizeof(int)*4,     "SCT_VSINT"},
> -
>    {D3DXPT_FLOAT, sizeof(float)*4, "SCT_PSFLOAT"},
> -
>    {D3DXPT_BOOL, sizeof(BOOL),     "SCT_PSBOOL"},
> -
>    {D3DXPT_INT, sizeof(int)*4,     "SCT_PSINT"},
> - };
> - int nelem;
> -
> - if (op < 0 || op > SCT_PSINT)
> - {
> -
>    FIXME("unknown op %d\n", op);
> -
>    return D3DERR_INVALIDCALL;
> - }
> - nelem = param->bytes / const_tbl[op].elem_size;
> - TRACE("setting %s index %d, %d elements\n", const_tbl[op].name, index, nelem);
> - if (param->type != const_tbl[op].type)
> - {
> -
>    FIXME("unexpected param type: %d\n", param->type);
> -
>    return D3DERR_INVALIDCALL;
> - }
> - if (param->bytes % const_tbl[op].elem_size != 0)
> - {
> -
>    FIXME("unexpected param size: %d (rows=%d, cols=%d)\n", param->bytes, param->rows, param->columns);

%u, also we usually do without the '=' i.e. "rows %u, cols %u".
> -
>    return D3DERR_INVALIDCALL;
> - }
> -
> - switch(op)
> - {
> -
>    case SCT_VSFLOAT:
> -
>        return IDirect3DDevice9_SetVertexShaderConstantF(device, index, (const float*)value_ptr,nelem);
> -
>    case SCT_VSBOOL:
> -
>        return IDirect3DDevice9_SetVertexShaderConstantB(device, index, (const BOOL*)value_ptr,nelem);
> -
>    case SCT_VSINT:
> -
>        return IDirect3DDevice9_SetVertexShaderConstantI(device, index, (const int*)value_ptr,nelem);
> -
>    case SCT_PSFLOAT:
> -
>        return IDirect3DDevice9_SetPixelShaderConstantF(device, index, (const float*)value_ptr,nelem);
> -
>    case SCT_PSBOOL:
> -
>        return IDirect3DDevice9_SetPixelShaderConstantB(device, index, (const BOOL*)value_ptr,nelem);
> -
>    case SCT_PSINT:
> -
>        return IDirect3DDevice9_SetPixelShaderConstantI(device, index, (const int*)value_ptr,nelem);
> - }
> - return D3D_OK;
>
> +}
> -
>
> +static HRESULT d3dx9_apply_state(struct ID3DXEffectImpl *This, struct d3dx_pass *pass, struct d3dx_state *state, int parent_index)
> +{
> - IDirect3DDevice9 *device = This->device;
> - struct d3dx_parameter *param;
> - void *param_value;
> - HRESULT hr;
> -
> - TRACE("operation=%d index=%d type=%u\n", state->operation, state->index, state->type);
> - hr = d3dx9_get_param_value_ptr(This, pass, state, &param_value, &param);
> - if (FAILED(hr))
> -
>    return hr;
> -
> - switch(state_table[state->operation].class)
> - {
> -
>    case SC_RENDERSTATE:
> -
>        TRACE("%s: operation %d value %u\n", state_table[state->operation].name, state_table[state->operation].op, *(DWORD*)param_value);
> -
>        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_TEXTURE:
> -
>    {
> -
>        int sampler;
> -
> -
>        sampler = parent_index == -1 ? state->index : parent_index;
> -
>        TRACE("%s: sampler %d value %p\n", state_table[state->operation].name, sampler, *(IDirect3DBaseTexture9 **)param_value);
> -
>        return IDirect3DDevice9_SetTexture(device, sampler, *(IDirect3DBaseTexture9 **)param_value);
> -
>    }
> -
>    case SC_TEXTURESTAGE:
> -
>        TRACE("%s: stage %d 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_SETSAMPLER:
> -
>    {
> -
>        struct d3dx_sampler *sampler;
> -
>        HRESULT ret, hr;
> -
>        int i;
> -
> -
>        sampler = (struct d3dx_sampler *)param_value;
> -
>        TRACE("%s: sampler %d applying %d states\n", state_table[state->operation].name, state->index, sampler->state_count);
> -
>        ret = D3D_OK;
> -
>        for (i = 0; i < sampler->state_count; i++)
> -
>        {
> -
>            hr = d3dx9_apply_state(This, pass, &sampler->states[i], state->index);
> -
>            if (FAILED(hr))
> -
>                ret = hr;
> -
>        }
> -
>        return ret;
> -
>    }
> -
>    case SC_SAMPLERSTATE:
> -
>    {
> -
>        int sampler;
> -
> -
>        sampler = parent_index == -1 ? state->index : parent_index;
> -
>        TRACE("%s sampler %d:%u\n", state_table[state->operation].name, sampler, *(DWORD*)param_value);
> -
>        return IDirect3DDevice9_SetSamplerState(device, sampler, 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 %d\n", state_table[state->operation].name, state->index);
> -
>        return IDirect3DDevice9_SetTransform(device, state_table[state->operation].op + state->index, (D3DMATRIX*)param_value);
> -
>    case SC_LIGHTENABLE:
> -
>        TRACE("%s index %d %d\n", state_table[state->operation].name, state->index, *(BOOL*)param_value);
> -
>        return IDirect3DDevice9_LightEnable(device, state->index, *(BOOL*)param_value);
> -
>    case SC_LIGHT:
> -
>    {
> -
>        D3DLIGHT9 light;
> -
> -
>        TRACE("%s op %d index %d\n", state_table[state->operation].name, state_table[state->operation].op, state->index);
> -
>        hr = IDirect3DDevice9_GetLight(device, state->index, &light);
> -
>        if (FAILED(hr))
> -
>        {
> -
>            WARN("could not get light: %#x\n", hr);
> -
>            memset(&light, 0, sizeof light);
> -
>        }
> -
>        d3dx9_set_light_parameter(state_table[state->operation].op, &light, param_value);
> -
>        return IDirect3DDevice9_SetLight(device, state->index, &light);
> -
>    }
> -
>    case SC_MATERIAL:
> -
>    {
> -
>        D3DMATERIAL9 material;
> -
> -
>        TRACE("%s op %d index %d\n", state_table[state->operation].name, state_table[state->operation].op, state->index);
> -
>        hr = IDirect3DDevice9_GetMaterial(device, &material);
> -
>        if (FAILED(hr))
> -
>        {
> -
>            WARN("could not get material: %#x\n", hr);
> -
>            memset(&material, 0, sizeof material);
> -
>        }
> -
>        d3dx9_set_material_parameter(state_table[state->operation].op, &material, param_value);
> -
>        return IDirect3DDevice9_SetMaterial(device, &material);
> -
>    }
> -
>    case SC_NPATCHMODE:
> -
>        TRACE("%s %f\n", state_table[state->operation].name, *(float*)param_value);
> -
>        return IDirect3DDevice9_SetNPatchMode(device, *(float*)param_value);
> -
>    break;

Break is unnecessary after the return (also it's wrongly indented).
> -
>    case SC_SHADERCONST:
> -
>        TRACE("%s op %d index %d\n", state_table[state->operation].name, state_table[state->operation].op, state->index);
> -
>        return d3dx_set_shader_const_state(device, state_table[state->operation].op, state->index, param, param_value);
> -
>    break;
> -
>    default:
> -
>        FIXME("%s not handled\n", state_table[state->operation].name);
> -
>    break;
> - }
> - return D3D_OK;
>
> +}
> -
>
> +static HRESULT d3dx9_apply_pass_states(struct ID3DXEffectImpl *This, struct d3dx_pass *pass)
> +{
> - int istate;

Unsigned int, also you could just call it "i" (or "state_idx").
> - HRESULT ret;
> -
> - TRACE("This %p, pass %p, state_count=%d\n", This, pass, pass->state_count);
> -
> - ret = D3D_OK;
> - for (istate = 0; istate < pass->state_count; istate++)
> - {
> -
>    HRESULT hr;
> -
> -
>    hr = d3dx9_apply_state(This, pass, &pass->states[istate], -1);
> -
>    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);
> @@ -3158,7 +3492,23 @@ static HRESULT WINAPI ID3DXEffectImpl_Begin(ID3DXEffect *iface, UINT *passes, DW
>          }
>          else
>          {
> -
>        FIXME("State capturing not supported, yet!\n");
> -
>        HRESULT hr;
> -
>        int i;
> -
> -
>        if (!technique->saved_state)
> -
>        {
> -
>            hr = IDirect3DDevice9_BeginStateBlock(This->device);
> -
>            if (FAILED(hr))
> -
>                ERR("BeginStateBlock failed: %#x\n",hr);
> -
>            for (i = 0; i < technique->pass_count; i++)
> -
>                d3dx9_apply_pass_states(This, &technique->passes[i]);
> -
>            hr = IDirect3DDevice9_EndStateBlock(This->device, &technique->saved_state);
> -
>            if (FAILED(hr))
> -
>                ERR("EndStateBlock failed: %#x\n",hr);
> -
>        }
> -
>        hr = IDirect3DStateBlock9_Capture(technique->saved_state);
> -
>        if (FAILED(hr))
> -
>            ERR("StateBlock Capture failed: %#x\n",hr);
>    }
>
>    *passes = technique->pass_count;
>
> @@ -3183,10 +3533,7 @@ static HRESULT WINAPI ID3DXEffectImpl_BeginPass(ID3DXEffect *iface, UINT pass)
>      if (technique && pass < technique->pass_count && !This->active_pass)
>      {
>          This->active_pass = &technique->passes[pass];
> -
> -
>    FIXME("No states applied, yet!\n");
> -
> -
>    return D3D_OK;
> -
>    return d3dx9_apply_pass_states(This, This->active_pass);
>
> }
>
> WARN("Invalid argument supplied.\n");
>
> @@ -3198,15 +3545,13 @@ static HRESULT WINAPI ID3DXEffectImpl_CommitChanges(ID3DXEffect* iface)
>  {
>      struct ID3DXEffectImpl *This = impl_from_ID3DXEffect(iface);
> - FIXME("(%p)->(): stub\n", This);
> - if (!This->active_pass)
>    {
>        WARN("Called without an active pass.\n");
>        return D3D_OK;
>    }
> -
> - return E_NOTIMPL;
> - /* TODO: apply only changed states */
> - return d3dx9_apply_pass_states(This, This->active_pass);
>
> }
>
> static HRESULT WINAPI ID3DXEffectImpl_EndPass(ID3DXEffect *iface)
> @@ -3229,6 +3574,7 @@ static HRESULT WINAPI ID3DXEffectImpl_EndPass(ID3DXEffect *iface)
>  static HRESULT WINAPI ID3DXEffectImpl_End(ID3DXEffect *iface)
>  {
>      struct ID3DXEffectImpl *This = impl_from_ID3DXEffect(iface);
> -
>
> struct d3dx_technique *technique = This->active_technique;
>
> TRACE("iface %p.\n", iface);
>
> @@ -3241,7 +3587,16 @@ static HRESULT WINAPI ID3DXEffectImpl_End(ID3DXEffect *iface)
>      }
>      else
>      {
> -
>    FIXME("State restoring not supported, yet!\n");
> -
>    HRESULT hr;
> -
> -
>    if (technique && technique->saved_state)
> -
>    {
> -
>        hr = IDirect3DStateBlock9_Apply(technique->saved_state);
> -
>        if (FAILED(hr))
> -
>            ERR("stateblock apply failed: %#x\n",hr);
> -
>    }
> -
>    else
> -
>        ERR("no saved state");
>
> }
>
> This->started = FALSE;
>
> diff --git a/dlls/d3dx9_36/tests/effect.c b/dlls/d3dx9_36/tests/effect.c
> index 482f064..a1b35b7 100644
> --- a/dlls/d3dx9_36/tests/effect.c
> +++ b/dlls/d3dx9_36/tests/effect.c
> @@ -2692,6 +2692,246 @@ static void test_effect_compilation_errors(IDirect3DDevice9 *device)
>      effect->lpVtbl->Release(effect);
>  }
>
> +/* fxc.exe /T fx_2_0
> +vertexshader vs_arr1[2] =
> +{
> - asm
> - {
> - vs_1_1
> - def c0, 1, 1, 1, 1
> - mov oPos, c0
> - },
> - asm
> - {
> - vs_2_0
> - def c0, 2, 2, 2, 2
> - mov oPos, c0
> - }
>
> +};

Good idea with the shader array.
> -
>
> +sampler sampler1 =
> +sampler_state
> +{
> - MipFilter = LINEAR;
>
> +};
> -
>
> +float4x4 camera : VIEW = {4.0,0.0,0.0,0.0, 0.0,0.0,0.0,0.0, 0.0,0.0,0.0,0.0, 0.0,0.0,0.0,6.0};
> +technique tech0
> +{
> - pass p0
> - {
> - vertexshader = vs_arr1[1];

Idea for a future preshader test, see what happens if you put an
expression using parameters as the array index. Not a priority by any
means.
> - VertexShaderConstant1[3] = {2,2,2,2};
> - BlendOp = 2;
> - AlphaOp[3] = 4;
> - ZEnable = true;
> - WorldTransform[1]={2.0,0.0,0.0,0.0, 0.0,0.0,0.0,0.0, 0.0,0.0,0.0,0.0, 0.0,0.0,0.0,4.0};
> - ViewTransform=(camera);
> - LightEnable[2] = TRUE;
> - LightType[2] = POINT;
> - LightPosition[2] = {4.0f, 5.0f, 6.0f};
> - Sampler[1] = sampler1;
> - }
>
> +}

I know this is just for reference but it would be better if this was
formatted properly.
> +*/
> +static const DWORD test_effect_states_effect_blob[] =
> +{
> +0xfeff0901, 0x000002e8, 0x00000000, 0x00000010, 0x00000004, 0x00000020, 0x00000000, 0x00000002,
> +0x00000001, 0x00000002, 0x00000008, 0x615f7376, 0x00317272, 0x0000000a, 0x00000004, 0x00000074,
> +0x00000000, 0x00000000, 0x00000002, 0x00000002, 0x00000002, 0x00000000, 0x00000000, 0x00000000,
> +0x00000001, 0x00000001, 0x00000001, 0x000000ab, 0x00000100, 0x00000044, 0x00000040, 0x00000009,
> +0x706d6173, 0x3172656c, 0x00000000, 0x00000003, 0x00000002, 0x000000e0, 0x000000ec, 0x00000000,
> +0x00000004, 0x00000004, 0x40800000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000,
> +0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000,
> +0x00000000, 0x40c00000, 0x00000007, 0x656d6163, 0x00006172, 0x00000005, 0x57454956, 0x00000000,
> +0x00000003, 0x00000010, 0x00000004, 0x00000000, 0x00000000, 0x00000000, 0x40000000, 0x40000000,
> +0x40000000, 0x40000000, 0x00000003, 0x00000003, 0x00000000, 0x00000000, 0x00000000, 0x00000004,
> +0x00000001, 0x00000002, 0x00000002, 0x00000002, 0x00000000, 0x00000000, 0x00000000, 0x00000001,
> +0x00000001, 0x00000004, 0x00000002, 0x00000002, 0x00000000, 0x00000000, 0x00000000, 0x00000001,
> +0x00000001, 0x00000001, 0x00000002, 0x00000002, 0x00000000, 0x00000000, 0x00000000, 0x00000001,
> +0x00000001, 0x40000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000,
> +0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000,
> +0x40800000, 0x00000003, 0x00000002, 0x00000000, 0x00000000, 0x00000000, 0x00000010, 0x00000001,
> +0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000,
> +0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000,
> +0x00000003, 0x00000002, 0x00000000, 0x00000000, 0x00000000, 0x00000004, 0x00000004, 0x00000001,
> +0x00000002, 0x00000002, 0x00000000, 0x00000000, 0x00000000, 0x00000001, 0x00000001, 0x00000001,
> +0x00000002, 0x00000002, 0x00000000, 0x00000000, 0x00000000, 0x00000001, 0x00000001, 0x40800000,
> +0x40a00000, 0x40c00000, 0x00000003, 0x00000002, 0x00000000, 0x00000000, 0x00000000, 0x00000003,
> +0x00000001, 0x00000000, 0x0000000a, 0x00000004, 0x00000000, 0x00000000, 0x00000000, 0x00000003,
> +0x00003070, 0x00000006, 0x68636574, 0x00000030, 0x00000003, 0x00000001, 0x00000005, 0x00000004,
> +0x00000004, 0x00000018, 0x00000000, 0x00000000, 0x0000002c, 0x00000060, 0x00000000, 0x00000000,
> +0x00000084, 0x000000a0, 0x00000000, 0x00000000, 0x000002dc, 0x00000000, 0x00000001, 0x000002d4,
> +0x00000000, 0x0000000b, 0x00000092, 0x00000000, 0x000000fc, 0x000000f8, 0x00000098, 0x00000003,
> +0x00000120, 0x00000110, 0x0000004b, 0x00000000, 0x00000140, 0x0000013c, 0x0000006b, 0x00000003,
> +0x00000160, 0x0000015c, 0x00000000, 0x00000000, 0x00000180, 0x0000017c, 0x0000007d, 0x00000001,
> +0x000001dc, 0x0000019c, 0x0000007c, 0x00000000, 0x00000238, 0x000001f8, 0x00000091, 0x00000002,
> +0x00000258, 0x00000254, 0x00000084, 0x00000002, 0x00000278, 0x00000274, 0x00000088, 0x00000002,
> +0x000002a0, 0x00000294, 0x000000b2, 0x00000001, 0x000002c0, 0x000002bc, 0x00000002, 0x00000003,
> +0x00000001, 0x0000002c, 0xfffe0101, 0x00000051, 0xa00f0000, 0x3f800000, 0x3f800000, 0x3f800000,
> +0x3f800000, 0x00000001, 0xc00f0000, 0xa0e40000, 0x0000ffff, 0x00000002, 0x0000002c, 0xfffe0200,
> +0x05000051, 0xa00f0000, 0x40000000, 0x40000000, 0x40000000, 0x40000000, 0x02000001, 0xc00f0000,
> +0xa0e40000, 0x0000ffff, 0x00000000, 0x00000000, 0xffffffff, 0x0000000a, 0x00000001, 0x00000009,
> +0x706d6173, 0x3172656c, 0x00000000, 0x00000000, 0x00000000, 0xffffffff, 0x00000006, 0x00000000,
> +0x0000016c, 0x46580200, 0x0030fffe, 0x42415443, 0x0000001c, 0x0000008b, 0x46580200, 0x00000001,
> +0x0000001c, 0x20000100, 0x00000088, 0x00000030, 0x00000002, 0x00000004, 0x00000038, 0x00000048,
> +0x656d6163, 0xab006172, 0x00030003, 0x00040004, 0x00000001, 0x00000000, 0x40800000, 0x00000000,
> +0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000,
> +0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x40c00000, 0x4d007874, 0x6f726369,
> +0x74666f73, 0x29522820, 0x534c4820, 0x6853204c, 0x72656461, 0x6d6f4320, 0x656c6970, 0x2e392072,
> +0x392e3932, 0x332e3235, 0x00313131, 0x0002fffe, 0x54494c43, 0x00000000, 0x0024fffe, 0x434c5846,
> +0x00000004, 0x10000004, 0x00000001, 0x00000000, 0x00000002, 0x00000000, 0x00000000, 0x00000004,
> +0x00000000, 0x10000004, 0x00000001, 0x00000000, 0x00000002, 0x00000004, 0x00000000, 0x00000004,
> +0x00000004, 0x10000004, 0x00000001, 0x00000000, 0x00000002, 0x00000008, 0x00000000, 0x00000004,
> +0x00000008, 0x10000004, 0x00000001, 0x00000000, 0x00000002, 0x0000000c, 0x00000000, 0x00000004,
> +0x0000000c, 0xf0f0f0f0, 0x0f0f0f0f, 0x0000ffff, 0x00000000, 0x00000000, 0xffffffff, 0x00000000,
> +0x00000001, 0x0000000b, 0x615f7376, 0x5b317272, 0x00005d31,
> +};
> -
>
> +static DWORD test_effect_states_vshader_buf[sizeof test_effect_states_effect_blob / sizeof (DWORD)];

Please use parentheses for the argument of sizeof consistently (always
use them, no space).
> -
>
> +static void test_effect_states(IDirect3DDevice9 *device)
> +{
> - D3DMATRIX test_mat =
> - {
> -
>    {
> -
>    -1.0f, 0.0f, 0.0f, 0.0f,
> -
>    0.0f, 0.0f, 0.0f, 0.0f,
> -
>    0.0f, 0.0f, 0.0f, 0.0f,
> -
>    0.0f, 0.0f, 0.0f, 0.0f
> -
>    }
> - };
> - D3DMATRIX mat;
> - HRESULT hr;
> - ID3DXEffect *effect;
> - UINT cPasses;

No camelcase or similar, just lowercase with underscores.
> - DWORD value;
> - IDirect3DVertexShader9 *vshader;
> - UINT byte_code_size;
> - BOOL bval;
> - D3DLIGHT9 light;
> - float float_data[4];
> -
> - hr = D3DXCreateEffect(device, test_effect_states_effect_blob, sizeof(test_effect_states_effect_blob),
> -
>    NULL, NULL, 0, NULL, &effect, NULL);
> - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> -
> - /* 1. State affected in passes saved/restored even if no pass
> -
>   was performed. States not present in passes are not saved &
> -
>   restored
> - */

The "*/" goes on the previous line.
> - hr = IDirect3DDevice9_SetRenderState(device, D3DRS_BLENDOP, 1);
> - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> - hr = IDirect3DDevice9_SetRenderState(device, D3DRS_ALPHAFUNC, 1);
> - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> -
> - hr = effect->lpVtbl->Begin(effect, &cPasses, 0);
> - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> - ok(cPasses == 1, "Expected 1 pass, got %d\n", cPasses);
> -
> - hr = IDirect3DDevice9_SetRenderState(device, D3DRS_BLENDOP, 3);
> - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> - hr = IDirect3DDevice9_SetRenderState(device, D3DRS_ALPHAFUNC, 2);
> - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> -
> - hr = effect->lpVtbl->End(effect);
> - 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);
> - ok(value == 1, "Got result %d, expected %d\n", value, 1);
> - hr = IDirect3DDevice9_GetRenderState(device, D3DRS_ALPHAFUNC, &value);
> - ok(value == 2, "Got result %d, expected %d\n", value, 2);
> -
> - /* 2. Test states application in BeginPass. No states are restored
> -
>   on EndPass.
> - */
> - hr = IDirect3DDevice9_SetSamplerState(device, 1, D3DSAMP_MIPFILTER, 0);
> - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> - hr = IDirect3DDevice9_SetRenderState(device, D3DRS_ZENABLE, 0);
> - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> -
> - hr = IDirect3DDevice9_GetLightEnable(device, 2, &bval);
> - ok(!bval,"Got result %d, expected 0", bval);
> -
> - hr = IDirect3DDevice9_SetTransform(device, D3DTS_WORLDMATRIX(1), &test_mat);
> - hr = effect->lpVtbl->Begin(effect, &cPasses, 0);
> - 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(mat.m[0][0] == test_mat.m[0][0], "Unexpected value: %f\n", mat.m[0][0]);

I would check the entire matrix with memcmp().
> -
> - hr = effect->lpVtbl->BeginPass(effect, 0);
> - 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(mat.m[0][0] == 2.0f && mat.m[3][3] == 4.0f, "World matrix not applied (%f %f)\n", mat.m[0][0], mat.m[3][3]);
> -
> - hr = IDirect3DDevice9_GetTransform(device, D3DTS_VIEW, &mat);
> - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> - todo_wine ok(mat.m[0][0] == 4.0f && mat.m[3][3] == 6.0f, "View matrix not applied (%f %f)\n", mat.m[0][0], mat.m[3][3]);
> -
> - hr = IDirect3DDevice9_GetRenderState(device, D3DRS_BLENDOP, &value);
> - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> - ok(value == 2, "Got result %d, expected %d\n", value, 2);
> -
> - hr = IDirect3DDevice9_GetVertexShader(device, &vshader);
> - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> - byte_code_size = sizeof test_effect_states_vshader_buf;
> - hr = IDirect3DVertexShader9_GetFunction(vshader, test_effect_states_vshader_buf,&byte_code_size);
> - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> - ok(byte_code_size > 1, "Got unexpected byte code size %d\n", byte_code_size);
> - ok(test_effect_states_vshader_buf[0] == 0xfffe0200, "Incorrect shader selected: %#x\n", test_effect_states_vshader_buf[0]);
> - IDirect3DVertexShader9_Release(vshader);
> -
> - hr = IDirect3DDevice9_GetLightEnable(device, 2, &bval);
> - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> - ok(bval,"Got result %d, expected TRUE", bval);
> - hr = IDirect3DDevice9_GetLight(device, 2, &light);
> - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> - ok(light.Position.x == 4.0f && light.Position.y == 5.0f && light.Position.z == 6.0f,
> -
>    "Got unexpected light position (%f, %f, %f)\n", light.Position.x, light.Position.y, light.Position.z);
> - hr = IDirect3DDevice9_GetVertexShaderConstantF(device, 3, float_data, 1);
> - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> - ok(float_data[0] == 2.0f && float_data[1] == 2.0f && float_data[2] == 2.0f && float_data[3] == 2.0f,
> -
>    "Got unexpected vertex shader floats: (%f %f %f %f)\n",
> -
>    float_data[0], float_data[1], float_data[2], float_data[3]);

We use 8 spaces for line continuations in new code.
> -
> - hr = effect->lpVtbl->EndPass(effect);
> - 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);
> - ok(value == 2, "Got result %d, expected %d\n", value, 2);
> -
> - hr = IDirect3DDevice9_GetRenderState(device, D3DRS_ZENABLE, &value);
> - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> - ok(value,"Got result %d, 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);
> - ok(value == D3DTEXF_LINEAR, "Unexpected sampler 1 mipfilter %d\n", value);
> -
> - hr = IDirect3DDevice9_GetTextureStageState(device, 3, D3DTSS_ALPHAOP, &value);
> - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> - ok(value == 4, "Unexpected texture stage 3 AlphaOp %d\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(mat.m[0][0] == test_mat.m[0][0] && mat.m[3][3] == test_mat.m[3][3],
> -
>    "World matrix not restored (%f %f)\n", mat.m[0][0], mat.m[3][3]);
> -
> - hr = IDirect3DDevice9_GetLightEnable(device, 2, &bval);
> - ok(!bval,"Got result %d, expected 0", bval);
> -
> - if (effect)
> -
>    effect->lpVtbl->Release(effect);
>
> +}
> -
>
> START_TEST(effect)
>  {
>      HWND wnd;
> @@ -2730,6 +2970,7 @@ START_TEST(effect)
>      test_effect_parameter_value(device);
>      test_effect_variable_names(device);
>      test_effect_compilation_errors(device);
> -
>
> test_effect_states(device);
>
> count = IDirect3DDevice9_Release(device);
> ok(count == 0, "The device was not properly freed: refcount %u\n", count);
>
> --
> 2.5.0

[PATCH] d3dx9: implement effect state management.

[Henri Verbeet]

> > +static void test_effect_states(IDirect3DDevice9 *device)
> > +{
> > - D3DMATRIX test_mat =
> > - {
> > -
> >    {
> > -
> >    -1.0f, 0.0f, 0.0f, 0.0f,
> > -
> >    0.0f, 0.0f, 0.0f, 0.0f,
> > -
> >    0.0f, 0.0f, 0.0f, 0.0f,
> > -
> >    0.0f, 0.0f, 0.0f, 0.0f
> > -
> >    }
> > - };
> > - D3DMATRIX mat;
> > - HRESULT hr;
> > - ID3DXEffect *effect;
> > - UINT cPasses;
>
> No camelcase or similar, just lowercase with underscores.

Somewhat related, "This" really isn't a great variable name.
Unfortunately it's fairly common in Wine source, I suppose it's
something people picked up from C++ and then propagated.

[PATCH] d3dx9: implement effect state management.

[Paul Gofman]

Matteo,

thank you for the review, I will be following up.
    Regarding splitting the test, do you suggest introducing separate
test functions or just adding tests into the same one step by step?

Regards,
    Paul.

On 03/01/2016 04:15 PM, Matteo Bruni wrote:
> 2016-02-29 19:08 GMT+01:00 Paul Gofman :
> > Preshaders (and FXLC-type constants requiring preshaders) are TODOs (on the way).
> > Signed-off-by: Paul Gofman
> >
> > dlls/d3dx9_36/effect.c       | 375 +++++++++++++++++++++++++++++++++++++++++--
> >  dlls/d3dx9_36/tests/effect.c | 241 +++++++++++++++++++++++++++
> >  2 files changed, 606 insertions(+), 10 deletions(-)
>
> You should split this patch into smaller pieces. One way to do that is
> to introduce the various state classes one at a time into
> d3dx9_apply_state (or, not everything at once, anyway). The state
> saving / restoring should also be a separate patch.
> WRT the tests, they should probably be split out too. You can put them
> before the implementation, inserting and removing "todo_wine" where
> necessary, or after. If it's not too complicated the first option is
> generally preferred.
> BTW it turns out I have a similar test somewhere in my queue. I'll
> have another look and I might send it if it still makes sense to.
>
> I'm leaving a few comments inline in the patch, below, mostly style
> issues I noticed at a quick look. Many of them apply multiple times
> through the patch. In general, make sure to be consistent with code
> style.
>
> Oh, and thank you for working on this!
> > diff --git a/dlls/d3dx9_36/effect.c b/dlls/d3dx9_36/effect.c
> > index f0bbd0b..33e063a 100644
> > --- a/dlls/d3dx9_36/effect.c
> > +++ b/dlls/d3dx9_36/effect.c
> > @@ -162,6 +162,8 @@ struct d3dx_technique
> >  struct d3dx_parameter *annotations;
> >  struct d3dx_pass *passes;
> > -
> > - struct IDirect3DStateBlock9 *saved_state;
> >
> > };
> >
> > struct param_table
> > @@ -642,6 +644,12 @@ static void free_technique(struct d3dx_technique *technique)
> >      if (!technique)
> >          return;
> > - if (technique->saved_state)
> > - {
> > -
> >    IDirect3DStateBlock9_Release(technique->saved_state);
> > -
> >    technique->saved_state = NULL;
> > - }
> > - if (technique->annotations)
> >    {
> >        for (i = 0; i < technique->annotation_count; ++i)
> >
> > @@ -2490,6 +2498,332 @@ 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 *This, struct d3dx_pass *pass, struct d3dx_state *state, void **param_value, struct d3dx_parameter **out_param)
> > +{
> > - struct d3dx_parameter *param;
> > - param = state->parameter.referenced_param ? state->parameter.referenced_param : &state->parameter;
> > -
> > - switch(state->type)
>
> Space after the switch keyword.
> > - {
> > -
> >    case ST_CONSTANT:
> > -
> >    case ST_PARAMETER:
> > -
> >        *param_value = param->data;
> > -
> >        *out_param = param;
> > -
> >        return D3D_OK;
> > -
> >    case ST_ARRAY_SELECTOR:
> > -
> >        FIXME("array selector\n");
>
> Uppercase at the start and period at the end of the message.
> > -
> >        break;
> > -
> >    case ST_FXLC:
> > -
> >        FIXME("FXLC not supported yet\n");
> > -
> >        break;
> > - }
> > - *param_value = NULL;
> > - *out_param = NULL;
> > - return E_NOTIMPL;
> >
> > +}
> > -
> >
> > +void d3dx9_set_light_parameter(enum LIGHT_TYPE op, D3DLIGHT9 *light, void *value)
> > +{
> > - static struct
> > - {
> > -
> >    int off;
> > -
> >    const char *name;
> > - } light_tbl[] =
> > - {
> > -
> >   {offsetof(D3DLIGHT9, Type),         "LC_TYPE"},
>
> You probably want to use FIELD_OFFSET instead.
> > -
> >   {offsetof(D3DLIGHT9, Diffuse),      "LT_DIFFUSE"},
> > -
> >   {offsetof(D3DLIGHT9, Specular),     "LT_SPECULAR"},
> > -
> >   {offsetof(D3DLIGHT9, Ambient),      "LT_AMBIENT"},
> > -
> >   {offsetof(D3DLIGHT9, Position),     "LT_POSITION"},
> > -
> >   {offsetof(D3DLIGHT9, Direction),    "LT_DIRECTION"},
> > -
> >   {offsetof(D3DLIGHT9, Range),        "LT_RANGE"},
> > -
> >   {offsetof(D3DLIGHT9, Falloff),      "LT_FALLOFF"},
> > -
> >   {offsetof(D3DLIGHT9, Attenuation0), "LT_ATTENUATION0"},
> > -
> >   {offsetof(D3DLIGHT9, Attenuation1), "LT_ATTENUATION1"},
> > -
> >   {offsetof(D3DLIGHT9, Attenuation2), "LT_ATTENUATION2"},
> > -
> >   {offsetof(D3DLIGHT9, Theta),        "LT_THETA"},
> > -
> >   {offsetof(D3DLIGHT9, Phi),          "LT_PHI"}
> > - };
> > - switch(op)
> > - {
> > -
> >    case LT_TYPE:
> > -
> >        TRACE("LT_TYPE %d\n", *(D3DLIGHTTYPE*)value);
>
> Please put a space between the type and the '*' in the cast.
> > -
> >        light->Type = *(D3DLIGHTTYPE*)value;
> > -
> >        break;
> > -
> >    case LT_DIFFUSE:
> > -
> >    case LT_SPECULAR:
> > -
> >    case LT_AMBIENT:
> > -
> >    {
> > -
> >        D3DCOLORVALUE c = *(D3DCOLORVALUE*)value;
> > -
> > -
> >        TRACE("%s (%f %f %f %f)\n", light_tbl[op].name, c.r, c.g, c.b, c.a);
> > -
> >        *(D3DCOLORVALUE*)((char*)light + light_tbl[op].off) = c;
> > -
> >        break;
> > -
> >    }
> > -
> >    case LT_POSITION:
> > -
> >    case LT_DIRECTION:
> > -
> >    {
> > -
> >        D3DVECTOR v = *(D3DVECTOR*)value;
> > -
> > -
> >        TRACE("%s (%f %f %f)\n", light_tbl[op].name, v.x, v.y, v.z);
> > -
> >        *(D3DVECTOR*)((char*)light + light_tbl[op].off) = v;
> > -
> >        break;
> > -
> >    }
> > -
> >    case LT_RANGE:
> > -
> >    case LT_FALLOFF:
> > -
> >    case LT_ATTENUATION0:
> > -
> >    case LT_ATTENUATION1:
> > -
> >    case LT_ATTENUATION2:
> > -
> >    case LT_THETA:
> > -
> >    case LT_PHI:
> > -
> >    {
> > -
> >        float v = *(float*)value;
> > -
> >        TRACE("%s %f\n", light_tbl[op].name, v);
> > -
> >        *(float*)((char*)light + light_tbl[op].off) = v;
> > -
> >        break;
> > -
> >    }
> > -
> >    default:
> > -
> >        FIXME("unknown light parameter %d\n",op);
> > -
> >        break;
> > - }
> >
> > +}
> > -
> >
> > +void d3dx9_set_material_parameter(enum MATERIAL_TYPE op, D3DMATERIAL9 *material, void *value)
> > +{
> > - static struct
>
> Please make it const too, if that works (I guess it might not).
> > - {
> > -
> >    int off;
> > -
> >    const char *name;
> > - } material_tbl[] =
> > - {
> > -
> >   {offsetof(D3DMATERIAL9, Diffuse),  "MT_DIFFUSE"},
> > -
> >   {offsetof(D3DMATERIAL9, Ambient),  "MT_AMBIENT"},
> > -
> >   {offsetof(D3DMATERIAL9, Specular), "MT_SPECULAR"},
> > -
> >   {offsetof(D3DMATERIAL9, Emissive), "MT_EMISSIVE"},
> > -
> >   {offsetof(D3DMATERIAL9, Power),    "MT_POWER"}
> > - };
> > -
> > - if (op < 0 || op > MT_POWER)
> > - {
> > -
> >    FIXME("unknown material parameter %d\n",op);
> > -
> >    return ;
>
> Stray whitespace.
> > - }
> > -
> > - switch(op)
> > - {
> > -
> >    case MT_POWER:
> > -
> >    {
> > -
> >        float v = *(float*)value;
> > -
> > -
> >        TRACE("%s %f\n", material_tbl[op].name, v);
> > -
> >        material->Power = v;
> > -
> >        break;
> > -
> >    }
> > -
> >    default:
> > -
> >    {
> > -
> >        D3DCOLORVALUE c = *(D3DCOLORVALUE*)value;
> > -
> > -
> >        TRACE("%s (%f %f %f %f)\n", material_tbl[op].name, c.r, c.g, c.b, c.a);
> > -
> >        *(D3DCOLORVALUE*)((char*)material + material_tbl[op].off) = c;
> > -
> >        break;
> > -
> >    }
> > - }
> >
> > +}
> > -
> >
> > +HRESULT d3dx_set_shader_const_state(IDirect3DDevice9 *device, enum SHADER_CONSTANT_TYPE op, int index, struct d3dx_parameter *param, void *value_ptr)
>
> Probably index should be unsigned int.
> > +{
> > - static struct {
>
> '{' should be on its own line.
> > -
> >    D3DXPARAMETER_TYPE type;
> > -
> >    int elem_size;
> > -
> >    const char *name;
> > - } const_tbl[] =
> > - {
> > -
> >    {D3DXPT_FLOAT, sizeof(float)*4, "SCT_VSFLOAT"},
>
> Space before and after the binary operator.
> > -
> >    {D3DXPT_BOOL, sizeof(BOOL),     "SCT_VSBOOL"},
> > -
> >    {D3DXPT_INT, sizeof(int)*4,     "SCT_VSINT"},
> > -
> >    {D3DXPT_FLOAT, sizeof(float)*4, "SCT_PSFLOAT"},
> > -
> >    {D3DXPT_BOOL, sizeof(BOOL),     "SCT_PSBOOL"},
> > -
> >    {D3DXPT_INT, sizeof(int)*4,     "SCT_PSINT"},
> > - };
> > - int nelem;
> > -
> > - if (op < 0 || op > SCT_PSINT)
> > - {
> > -
> >    FIXME("unknown op %d\n", op);
> > -
> >    return D3DERR_INVALIDCALL;
> > - }
> > - nelem = param->bytes / const_tbl[op].elem_size;
> > - TRACE("setting %s index %d, %d elements\n", const_tbl[op].name, index, nelem);
> > - if (param->type != const_tbl[op].type)
> > - {
> > -
> >    FIXME("unexpected param type: %d\n", param->type);
> > -
> >    return D3DERR_INVALIDCALL;
> > - }
> > - if (param->bytes % const_tbl[op].elem_size != 0)
> > - {
> > -
> >    FIXME("unexpected param size: %d (rows=%d, cols=%d)\n", param->bytes, param->rows, param->columns);
>
> %u, also we usually do without the '=' i.e. "rows %u, cols %u".
> > -
> >    return D3DERR_INVALIDCALL;
> > - }
> > -
> > - switch(op)
> > - {
> > -
> >    case SCT_VSFLOAT:
> > -
> >        return IDirect3DDevice9_SetVertexShaderConstantF(device, index, (const float*)value_ptr,nelem);
> > -
> >    case SCT_VSBOOL:
> > -
> >        return IDirect3DDevice9_SetVertexShaderConstantB(device, index, (const BOOL*)value_ptr,nelem);
> > -
> >    case SCT_VSINT:
> > -
> >        return IDirect3DDevice9_SetVertexShaderConstantI(device, index, (const int*)value_ptr,nelem);
> > -
> >    case SCT_PSFLOAT:
> > -
> >        return IDirect3DDevice9_SetPixelShaderConstantF(device, index, (const float*)value_ptr,nelem);
> > -
> >    case SCT_PSBOOL:
> > -
> >        return IDirect3DDevice9_SetPixelShaderConstantB(device, index, (const BOOL*)value_ptr,nelem);
> > -
> >    case SCT_PSINT:
> > -
> >        return IDirect3DDevice9_SetPixelShaderConstantI(device, index, (const int*)value_ptr,nelem);
> > - }
> > - return D3D_OK;
> >
> > +}
> > -
> >
> > +static HRESULT d3dx9_apply_state(struct ID3DXEffectImpl *This, struct d3dx_pass *pass, struct d3dx_state *state, int parent_index)
> > +{
> > - IDirect3DDevice9 *device = This->device;
> > - struct d3dx_parameter *param;
> > - void *param_value;
> > - HRESULT hr;
> > -
> > - TRACE("operation=%d index=%d type=%u\n", state->operation, state->index, state->type);
> > - hr = d3dx9_get_param_value_ptr(This, pass, state, &param_value, &param);
> > - if (FAILED(hr))
> > -
> >    return hr;
> > -
> > - switch(state_table[state->operation].class)
> > - {
> > -
> >    case SC_RENDERSTATE:
> > -
> >        TRACE("%s: operation %d value %u\n", state_table[state->operation].name, state_table[state->operation].op, *(DWORD*)param_value);
> > -
> >        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_TEXTURE:
> > -
> >    {
> > -
> >        int sampler;
> > -
> > -
> >        sampler = parent_index == -1 ? state->index : parent_index;
> > -
> >        TRACE("%s: sampler %d value %p\n", state_table[state->operation].name, sampler, *(IDirect3DBaseTexture9 **)param_value);
> > -
> >        return IDirect3DDevice9_SetTexture(device, sampler, *(IDirect3DBaseTexture9 **)param_value);
> > -
> >    }
> > -
> >    case SC_TEXTURESTAGE:
> > -
> >        TRACE("%s: stage %d 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_SETSAMPLER:
> > -
> >    {
> > -
> >        struct d3dx_sampler *sampler;
> > -
> >        HRESULT ret, hr;
> > -
> >        int i;
> > -
> > -
> >        sampler = (struct d3dx_sampler *)param_value;
> > -
> >        TRACE("%s: sampler %d applying %d states\n", state_table[state->operation].name, state->index, sampler->state_count);
> > -
> >        ret = D3D_OK;
> > -
> >        for (i = 0; i < sampler->state_count; i++)
> > -
> >        {
> > -
> >            hr = d3dx9_apply_state(This, pass, &sampler->states[i], state->index);
> > -
> >            if (FAILED(hr))
> > -
> >                ret = hr;
> > -
> >        }
> > -
> >        return ret;
> > -
> >    }
> > -
> >    case SC_SAMPLERSTATE:
> > -
> >    {
> > -
> >        int sampler;
> > -
> > -
> >        sampler = parent_index == -1 ? state->index : parent_index;
> > -
> >        TRACE("%s sampler %d:%u\n", state_table[state->operation].name, sampler, *(DWORD*)param_value);
> > -
> >        return IDirect3DDevice9_SetSamplerState(device, sampler, 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 %d\n", state_table[state->operation].name, state->index);
> > -
> >        return IDirect3DDevice9_SetTransform(device, state_table[state->operation].op + state->index, (D3DMATRIX*)param_value);
> > -
> >    case SC_LIGHTENABLE:
> > -
> >        TRACE("%s index %d %d\n", state_table[state->operation].name, state->index, *(BOOL*)param_value);
> > -
> >        return IDirect3DDevice9_LightEnable(device, state->index, *(BOOL*)param_value);
> > -
> >    case SC_LIGHT:
> > -
> >    {
> > -
> >        D3DLIGHT9 light;
> > -
> > -
> >        TRACE("%s op %d index %d\n", state_table[state->operation].name, state_table[state->operation].op, state->index);
> > -
> >        hr = IDirect3DDevice9_GetLight(device, state->index, &light);
> > -
> >        if (FAILED(hr))
> > -
> >        {
> > -
> >            WARN("could not get light: %#x\n", hr);
> > -
> >            memset(&light, 0, sizeof light);
> > -
> >        }
> > -
> >        d3dx9_set_light_parameter(state_table[state->operation].op, &light, param_value);
> > -
> >        return IDirect3DDevice9_SetLight(device, state->index, &light);
> > -
> >    }
> > -
> >    case SC_MATERIAL:
> > -
> >    {
> > -
> >        D3DMATERIAL9 material;
> > -
> > -
> >        TRACE("%s op %d index %d\n", state_table[state->operation].name, state_table[state->operation].op, state->index);
> > -
> >        hr = IDirect3DDevice9_GetMaterial(device, &material);
> > -
> >        if (FAILED(hr))
> > -
> >        {
> > -
> >            WARN("could not get material: %#x\n", hr);
> > -
> >            memset(&material, 0, sizeof material);
> > -
> >        }
> > -
> >        d3dx9_set_material_parameter(state_table[state->operation].op, &material, param_value);
> > -
> >        return IDirect3DDevice9_SetMaterial(device, &material);
> > -
> >    }
> > -
> >    case SC_NPATCHMODE:
> > -
> >        TRACE("%s %f\n", state_table[state->operation].name, *(float*)param_value);
> > -
> >        return IDirect3DDevice9_SetNPatchMode(device, *(float*)param_value);
> > -
> >    break;
>
> Break is unnecessary after the return (also it's wrongly indented).
> > -
> >    case SC_SHADERCONST:
> > -
> >        TRACE("%s op %d index %d\n", state_table[state->operation].name, state_table[state->operation].op, state->index);
> > -
> >        return d3dx_set_shader_const_state(device, state_table[state->operation].op, state->index, param, param_value);
> > -
> >    break;
> > -
> >    default:
> > -
> >        FIXME("%s not handled\n", state_table[state->operation].name);
> > -
> >    break;
> > - }
> > - return D3D_OK;
> >
> > +}
> > -
> >
> > +static HRESULT d3dx9_apply_pass_states(struct ID3DXEffectImpl *This, struct d3dx_pass *pass)
> > +{
> > - int istate;
>
> Unsigned int, also you could just call it "i" (or "state_idx").
> > - HRESULT ret;
> > -
> > - TRACE("This %p, pass %p, state_count=%d\n", This, pass, pass->state_count);
> > -
> > - ret = D3D_OK;
> > - for (istate = 0; istate < pass->state_count; istate++)
> > - {
> > -
> >    HRESULT hr;
> > -
> > -
> >    hr = d3dx9_apply_state(This, pass, &pass->states[istate], -1);
> > -
> >    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);
> > @@ -3158,7 +3492,23 @@ static HRESULT WINAPI ID3DXEffectImpl_Begin(ID3DXEffect *iface, UINT *passes, DW
> >          }
> >          else
> >          {
> > -
> >        FIXME("State capturing not supported, yet!\n");
> > -
> >        HRESULT hr;
> > -
> >        int i;
> > -
> > -
> >        if (!technique->saved_state)
> > -
> >        {
> > -
> >            hr = IDirect3DDevice9_BeginStateBlock(This->device);
> > -
> >            if (FAILED(hr))
> > -
> >                ERR("BeginStateBlock failed: %#x\n",hr);
> > -
> >            for (i = 0; i < technique->pass_count; i++)
> > -
> >                d3dx9_apply_pass_states(This, &technique->passes[i]);
> > -
> >            hr = IDirect3DDevice9_EndStateBlock(This->device, &technique->saved_state);
> > -
> >            if (FAILED(hr))
> > -
> >                ERR("EndStateBlock failed: %#x\n",hr);
> > -
> >        }
> > -
> >        hr = IDirect3DStateBlock9_Capture(technique->saved_state);
> > -
> >        if (FAILED(hr))
> > -
> >            ERR("StateBlock Capture failed: %#x\n",hr);
> >    }
> >
> >    *passes = technique->pass_count;
> >
> > @@ -3183,10 +3533,7 @@ static HRESULT WINAPI ID3DXEffectImpl_BeginPass(ID3DXEffect *iface, UINT pass)
> >      if (technique && pass < technique->pass_count && !This->active_pass)
> >      {
> >          This->active_pass = &technique->passes[pass];
> > -
> > -
> >    FIXME("No states applied, yet!\n");
> > -
> > -
> >    return D3D_OK;
> > -
> >    return d3dx9_apply_pass_states(This, This->active_pass);
> >
> > }
> >
> > WARN("Invalid argument supplied.\n");
> >
> > @@ -3198,15 +3545,13 @@ static HRESULT WINAPI ID3DXEffectImpl_CommitChanges(ID3DXEffect* iface)
> >  {
> >      struct ID3DXEffectImpl *This = impl_from_ID3DXEffect(iface);
> > - FIXME("(%p)->(): stub\n", This);
> > - if (!This->active_pass)
> >    {
> >        WARN("Called without an active pass.\n");
> >        return D3D_OK;
> >    }
> > -
> > - return E_NOTIMPL;
> > - /* TODO: apply only changed states */
> > - return d3dx9_apply_pass_states(This, This->active_pass);
> >
> > }
> >
> > static HRESULT WINAPI ID3DXEffectImpl_EndPass(ID3DXEffect *iface)
> > @@ -3229,6 +3574,7 @@ static HRESULT WINAPI ID3DXEffectImpl_EndPass(ID3DXEffect *iface)
> >  static HRESULT WINAPI ID3DXEffectImpl_End(ID3DXEffect *iface)
> >  {
> >      struct ID3DXEffectImpl *This = impl_from_ID3DXEffect(iface);
> > -
> >
> > struct d3dx_technique *technique = This->active_technique;
> >
> > TRACE("iface %p.\n", iface);
> >
> > @@ -3241,7 +3587,16 @@ static HRESULT WINAPI ID3DXEffectImpl_End(ID3DXEffect *iface)
> >      }
> >      else
> >      {
> > -
> >    FIXME("State restoring not supported, yet!\n");
> > -
> >    HRESULT hr;
> > -
> > -
> >    if (technique && technique->saved_state)
> > -
> >    {
> > -
> >        hr = IDirect3DStateBlock9_Apply(technique->saved_state);
> > -
> >        if (FAILED(hr))
> > -
> >            ERR("stateblock apply failed: %#x\n",hr);
> > -
> >    }
> > -
> >    else
> > -
> >        ERR("no saved state");
> >
> > }
> >
> > This->started = FALSE;
> >
> > diff --git a/dlls/d3dx9_36/tests/effect.c b/dlls/d3dx9_36/tests/effect.c
> > index 482f064..a1b35b7 100644
> > --- a/dlls/d3dx9_36/tests/effect.c
> > +++ b/dlls/d3dx9_36/tests/effect.c
> > @@ -2692,6 +2692,246 @@ static void test_effect_compilation_errors(IDirect3DDevice9 *device)
> >      effect->lpVtbl->Release(effect);
> >  }
> >
> > +/* fxc.exe /T fx_2_0
> > +vertexshader vs_arr1[2] =
> > +{
> > - asm
> > - {
> > - vs_1_1
> > - def c0, 1, 1, 1, 1
> > - mov oPos, c0
> > - },
> > - asm
> > - {
> > - vs_2_0
> > - def c0, 2, 2, 2, 2
> > - mov oPos, c0
> > - }
> >
> > +};
>
> Good idea with the shader array.
> > -
> >
> > +sampler sampler1 =
> > +sampler_state
> > +{
> > - MipFilter = LINEAR;
> >
> > +};
> > -
> >
> > +float4x4 camera : VIEW = {4.0,0.0,0.0,0.0, 0.0,0.0,0.0,0.0, 0.0,0.0,0.0,0.0, 0.0,0.0,0.0,6.0};
> > +technique tech0
> > +{
> > - pass p0
> > - {
> > - vertexshader = vs_arr1[1];
>
> Idea for a future preshader test, see what happens if you put an
> expression using parameters as the array index. Not a priority by any
> means.
> > - VertexShaderConstant1[3] = {2,2,2,2};
> > - BlendOp = 2;
> > - AlphaOp[3] = 4;
> > - ZEnable = true;
> > - WorldTransform[1]={2.0,0.0,0.0,0.0, 0.0,0.0,0.0,0.0, 0.0,0.0,0.0,0.0, 0.0,0.0,0.0,4.0};
> > - ViewTransform=(camera);
> > - LightEnable[2] = TRUE;
> > - LightType[2] = POINT;
> > - LightPosition[2] = {4.0f, 5.0f, 6.0f};
> > - Sampler[1] = sampler1;
> > - }
> >
> > +}
>
> I know this is just for reference but it would be better if this was
> formatted properly.
> > +*/
> > +static const DWORD test_effect_states_effect_blob[] =
> > +{
> > +0xfeff0901, 0x000002e8, 0x00000000, 0x00000010, 0x00000004, 0x00000020, 0x00000000, 0x00000002,
> > +0x00000001, 0x00000002, 0x00000008, 0x615f7376, 0x00317272, 0x0000000a, 0x00000004, 0x00000074,
> > +0x00000000, 0x00000000, 0x00000002, 0x00000002, 0x00000002, 0x00000000, 0x00000000, 0x00000000,
> > +0x00000001, 0x00000001, 0x00000001, 0x000000ab, 0x00000100, 0x00000044, 0x00000040, 0x00000009,
> > +0x706d6173, 0x3172656c, 0x00000000, 0x00000003, 0x00000002, 0x000000e0, 0x000000ec, 0x00000000,
> > +0x00000004, 0x00000004, 0x40800000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000,
> > +0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000,
> > +0x00000000, 0x40c00000, 0x00000007, 0x656d6163, 0x00006172, 0x00000005, 0x57454956, 0x00000000,
> > +0x00000003, 0x00000010, 0x00000004, 0x00000000, 0x00000000, 0x00000000, 0x40000000, 0x40000000,
> > +0x40000000, 0x40000000, 0x00000003, 0x00000003, 0x00000000, 0x00000000, 0x00000000, 0x00000004,
> > +0x00000001, 0x00000002, 0x00000002, 0x00000002, 0x00000000, 0x00000000, 0x00000000, 0x00000001,
> > +0x00000001, 0x00000004, 0x00000002, 0x00000002, 0x00000000, 0x00000000, 0x00000000, 0x00000001,
> > +0x00000001, 0x00000001, 0x00000002, 0x00000002, 0x00000000, 0x00000000, 0x00000000, 0x00000001,
> > +0x00000001, 0x40000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000,
> > +0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000,
> > +0x40800000, 0x00000003, 0x00000002, 0x00000000, 0x00000000, 0x00000000, 0x00000010, 0x00000001,
> > +0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000,
> > +0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000,
> > +0x00000003, 0x00000002, 0x00000000, 0x00000000, 0x00000000, 0x00000004, 0x00000004, 0x00000001,
> > +0x00000002, 0x00000002, 0x00000000, 0x00000000, 0x00000000, 0x00000001, 0x00000001, 0x00000001,
> > +0x00000002, 0x00000002, 0x00000000, 0x00000000, 0x00000000, 0x00000001, 0x00000001, 0x40800000,
> > +0x40a00000, 0x40c00000, 0x00000003, 0x00000002, 0x00000000, 0x00000000, 0x00000000, 0x00000003,
> > +0x00000001, 0x00000000, 0x0000000a, 0x00000004, 0x00000000, 0x00000000, 0x00000000, 0x00000003,
> > +0x00003070, 0x00000006, 0x68636574, 0x00000030, 0x00000003, 0x00000001, 0x00000005, 0x00000004,
> > +0x00000004, 0x00000018, 0x00000000, 0x00000000, 0x0000002c, 0x00000060, 0x00000000, 0x00000000,
> > +0x00000084, 0x000000a0, 0x00000000, 0x00000000, 0x000002dc, 0x00000000, 0x00000001, 0x000002d4,
> > +0x00000000, 0x0000000b, 0x00000092, 0x00000000, 0x000000fc, 0x000000f8, 0x00000098, 0x00000003,
> > +0x00000120, 0x00000110, 0x0000004b, 0x00000000, 0x00000140, 0x0000013c, 0x0000006b, 0x00000003,
> > +0x00000160, 0x0000015c, 0x00000000, 0x00000000, 0x00000180, 0x0000017c, 0x0000007d, 0x00000001,
> > +0x000001dc, 0x0000019c, 0x0000007c, 0x00000000, 0x00000238, 0x000001f8, 0x00000091, 0x00000002,
> > +0x00000258, 0x00000254, 0x00000084, 0x00000002, 0x00000278, 0x00000274, 0x00000088, 0x00000002,
> > +0x000002a0, 0x00000294, 0x000000b2, 0x00000001, 0x000002c0, 0x000002bc, 0x00000002, 0x00000003,
> > +0x00000001, 0x0000002c, 0xfffe0101, 0x00000051, 0xa00f0000, 0x3f800000, 0x3f800000, 0x3f800000,
> > +0x3f800000, 0x00000001, 0xc00f0000, 0xa0e40000, 0x0000ffff, 0x00000002, 0x0000002c, 0xfffe0200,
> > +0x05000051, 0xa00f0000, 0x40000000, 0x40000000, 0x40000000, 0x40000000, 0x02000001, 0xc00f0000,
> > +0xa0e40000, 0x0000ffff, 0x00000000, 0x00000000, 0xffffffff, 0x0000000a, 0x00000001, 0x00000009,
> > +0x706d6173, 0x3172656c, 0x00000000, 0x00000000, 0x00000000, 0xffffffff, 0x00000006, 0x00000000,
> > +0x0000016c, 0x46580200, 0x0030fffe, 0x42415443, 0x0000001c, 0x0000008b, 0x46580200, 0x00000001,
> > +0x0000001c, 0x20000100, 0x00000088, 0x00000030, 0x00000002, 0x00000004, 0x00000038, 0x00000048,
> > +0x656d6163, 0xab006172, 0x00030003, 0x00040004, 0x00000001, 0x00000000, 0x40800000, 0x00000000,
> > +0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000,
> > +0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x40c00000, 0x4d007874, 0x6f726369,
> > +0x74666f73, 0x29522820, 0x534c4820, 0x6853204c, 0x72656461, 0x6d6f4320, 0x656c6970, 0x2e392072,
> > +0x392e3932, 0x332e3235, 0x00313131, 0x0002fffe, 0x54494c43, 0x00000000, 0x0024fffe, 0x434c5846,
> > +0x00000004, 0x10000004, 0x00000001, 0x00000000, 0x00000002, 0x00000000, 0x00000000, 0x00000004,
> > +0x00000000, 0x10000004, 0x00000001, 0x00000000, 0x00000002, 0x00000004, 0x00000000, 0x00000004,
> > +0x00000004, 0x10000004, 0x00000001, 0x00000000, 0x00000002, 0x00000008, 0x00000000, 0x00000004,
> > +0x00000008, 0x10000004, 0x00000001, 0x00000000, 0x00000002, 0x0000000c, 0x00000000, 0x00000004,
> > +0x0000000c, 0xf0f0f0f0, 0x0f0f0f0f, 0x0000ffff, 0x00000000, 0x00000000, 0xffffffff, 0x00000000,
> > +0x00000001, 0x0000000b, 0x615f7376, 0x5b317272, 0x00005d31,
> > +};
> > -
> >
> > +static DWORD test_effect_states_vshader_buf[sizeof test_effect_states_effect_blob / sizeof (DWORD)];
>
> Please use parentheses for the argument of sizeof consistently (always
> use them, no space).
> > -
> >
> > +static void test_effect_states(IDirect3DDevice9 *device)
> > +{
> > - D3DMATRIX test_mat =
> > - {
> > -
> >    {
> > -
> >    -1.0f, 0.0f, 0.0f, 0.0f,
> > -
> >    0.0f, 0.0f, 0.0f, 0.0f,
> > -
> >    0.0f, 0.0f, 0.0f, 0.0f,
> > -
> >    0.0f, 0.0f, 0.0f, 0.0f
> > -
> >    }
> > - };
> > - D3DMATRIX mat;
> > - HRESULT hr;
> > - ID3DXEffect *effect;
> > - UINT cPasses;
>
> No camelcase or similar, just lowercase with underscores.
> > - DWORD value;
> > - IDirect3DVertexShader9 *vshader;
> > - UINT byte_code_size;
> > - BOOL bval;
> > - D3DLIGHT9 light;
> > - float float_data[4];
> > -
> > - hr = D3DXCreateEffect(device, test_effect_states_effect_blob, sizeof(test_effect_states_effect_blob),
> > -
> >    NULL, NULL, 0, NULL, &effect, NULL);
> > - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> > -
> > - /* 1. State affected in passes saved/restored even if no pass
> > -
> >   was performed. States not present in passes are not saved &
> > -
> >   restored
> > - */
>
> The "*/" goes on the previous line.
> > - hr = IDirect3DDevice9_SetRenderState(device, D3DRS_BLENDOP, 1);
> > - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> > - hr = IDirect3DDevice9_SetRenderState(device, D3DRS_ALPHAFUNC, 1);
> > - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> > -
> > - hr = effect->lpVtbl->Begin(effect, &cPasses, 0);
> > - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> > - ok(cPasses == 1, "Expected 1 pass, got %d\n", cPasses);
> > -
> > - hr = IDirect3DDevice9_SetRenderState(device, D3DRS_BLENDOP, 3);
> > - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> > - hr = IDirect3DDevice9_SetRenderState(device, D3DRS_ALPHAFUNC, 2);
> > - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> > -
> > - hr = effect->lpVtbl->End(effect);
> > - 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);
> > - ok(value == 1, "Got result %d, expected %d\n", value, 1);
> > - hr = IDirect3DDevice9_GetRenderState(device, D3DRS_ALPHAFUNC, &value);
> > - ok(value == 2, "Got result %d, expected %d\n", value, 2);
> > -
> > - /* 2. Test states application in BeginPass. No states are restored
> > -
> >   on EndPass.
> > - */
> > - hr = IDirect3DDevice9_SetSamplerState(device, 1, D3DSAMP_MIPFILTER, 0);
> > - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> > - hr = IDirect3DDevice9_SetRenderState(device, D3DRS_ZENABLE, 0);
> > - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> > -
> > - hr = IDirect3DDevice9_GetLightEnable(device, 2, &bval);
> > - ok(!bval,"Got result %d, expected 0", bval);
> > -
> > - hr = IDirect3DDevice9_SetTransform(device, D3DTS_WORLDMATRIX(1), &test_mat);
> > - hr = effect->lpVtbl->Begin(effect, &cPasses, 0);
> > - 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(mat.m[0][0] == test_mat.m[0][0], "Unexpected value: %f\n", mat.m[0][0]);
>
> I would check the entire matrix with memcmp().
> > -
> > - hr = effect->lpVtbl->BeginPass(effect, 0);
> > - 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(mat.m[0][0] == 2.0f && mat.m[3][3] == 4.0f, "World matrix not applied (%f %f)\n", mat.m[0][0], mat.m[3][3]);
> > -
> > - hr = IDirect3DDevice9_GetTransform(device, D3DTS_VIEW, &mat);
> > - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> > - todo_wine ok(mat.m[0][0] == 4.0f && mat.m[3][3] == 6.0f, "View matrix not applied (%f %f)\n", mat.m[0][0], mat.m[3][3]);
> > -
> > - hr = IDirect3DDevice9_GetRenderState(device, D3DRS_BLENDOP, &value);
> > - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> > - ok(value == 2, "Got result %d, expected %d\n", value, 2);
> > -
> > - hr = IDirect3DDevice9_GetVertexShader(device, &vshader);
> > - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> > - byte_code_size = sizeof test_effect_states_vshader_buf;
> > - hr = IDirect3DVertexShader9_GetFunction(vshader, test_effect_states_vshader_buf,&byte_code_size);
> > - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> > - ok(byte_code_size > 1, "Got unexpected byte code size %d\n", byte_code_size);
> > - ok(test_effect_states_vshader_buf[0] == 0xfffe0200, "Incorrect shader selected: %#x\n", test_effect_states_vshader_buf[0]);
> > - IDirect3DVertexShader9_Release(vshader);
> > -
> > - hr = IDirect3DDevice9_GetLightEnable(device, 2, &bval);
> > - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> > - ok(bval,"Got result %d, expected TRUE", bval);
> > - hr = IDirect3DDevice9_GetLight(device, 2, &light);
> > - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> > - ok(light.Position.x == 4.0f && light.Position.y == 5.0f && light.Position.z == 6.0f,
> > -
> >    "Got unexpected light position (%f, %f, %f)\n", light.Position.x, light.Position.y, light.Position.z);
> > - hr = IDirect3DDevice9_GetVertexShaderConstantF(device, 3, float_data, 1);
> > - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> > - ok(float_data[0] == 2.0f && float_data[1] == 2.0f && float_data[2] == 2.0f && float_data[3] == 2.0f,
> > -
> >    "Got unexpected vertex shader floats: (%f %f %f %f)\n",
> > -
> >    float_data[0], float_data[1], float_data[2], float_data[3]);
>
> We use 8 spaces for line continuations in new code.
> > -
> > - hr = effect->lpVtbl->EndPass(effect);
> > - 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);
> > - ok(value == 2, "Got result %d, expected %d\n", value, 2);
> > -
> > - hr = IDirect3DDevice9_GetRenderState(device, D3DRS_ZENABLE, &value);
> > - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> > - ok(value,"Got result %d, 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);
> > - ok(value == D3DTEXF_LINEAR, "Unexpected sampler 1 mipfilter %d\n", value);
> > -
> > - hr = IDirect3DDevice9_GetTextureStageState(device, 3, D3DTSS_ALPHAOP, &value);
> > - ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr);
> > - ok(value == 4, "Unexpected texture stage 3 AlphaOp %d\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(mat.m[0][0] == test_mat.m[0][0] && mat.m[3][3] == test_mat.m[3][3],
> > -
> >    "World matrix not restored (%f %f)\n", mat.m[0][0], mat.m[3][3]);
> > -
> > - hr = IDirect3DDevice9_GetLightEnable(device, 2, &bval);
> > - ok(!bval,"Got result %d, expected 0", bval);
> > -
> > - if (effect)
> > -
> >    effect->lpVtbl->Release(effect);
> >
> > +}
> > -
> >
> > START_TEST(effect)
> >  {
> >      HWND wnd;
> > @@ -2730,6 +2970,7 @@ START_TEST(effect)
> >      test_effect_parameter_value(device);
> >      test_effect_variable_names(device);
> >      test_effect_compilation_errors(device);
> > -
> >
> > test_effect_states(device);
> >
> > count = IDirect3DDevice9_Release(device);
> > ok(count == 0, "The device was not properly freed: refcount %u\n", count);
> >
> > --
> > 2.5.0

[PATCH] d3dx9: implement effect state management.

[Matteo Bruni]

2016-03-01 14:49 GMT+01:00 Paul Gofman :
> Matteo,
> thank you for the review, I will be following up.
> Regarding splitting the test, do you suggest introducing separate
>
> test functions or just adding tests into the same one step by step?

I just meant to split the test out of the implementation patch(es),
not necessarily to split the test over a number of patches, although I
guess the question is also valid for future extensions to the test.
The answer is "it depends", it's usually fine to add more stuff into
the same test as long as it's adding related stuff, it's easy to
integrate and it doesn't make the test too large.
> Regards,
>     Paul.