[v2 3/6] d3dx9: implement light and material effect states application.

Matteo Bruni matteo.mystral at gmail.com
Wed Mar 2 10:09:40 CST 2016


2016-03-01 18:09 GMT+01:00 Paul Gofman <gofmanp at gmail.com>:
> Signed-off-by: Paul Gofman <gofmanp at gmail.com>
> ---
>  dlls/d3dx9_36/effect.c       | 139 +++++++++++++++++++++++++++++++++++++++++++
>  dlls/d3dx9_36/tests/effect.c |   8 +--
>  2 files changed, 143 insertions(+), 4 deletions(-)
>

More nitpicks on this one...

> diff --git a/dlls/d3dx9_36/effect.c b/dlls/d3dx9_36/effect.c
> index 7ccdb36..16110c5 100644
> --- a/dlls/d3dx9_36/effect.c
> +++ b/dlls/d3dx9_36/effect.c
> @@ -2514,6 +2514,114 @@ static HRESULT d3dx9_get_param_value_ptr(struct ID3DXEffectImpl *effect, struct
>      return E_NOTIMPL;
>  }
>
> +static void d3dx9_set_light_parameter(enum LIGHT_TYPE op, D3DLIGHT9 *light, void *value)
> +{
> +    static const struct
> +    {
> +        int off;

I'd prefer "unsigned int offset;".

> +        const char *name;
> +    } light_tbl[] =

I think the wined3d style is to put the closing '}' on its own line.

> +    {
> +       {FIELD_OFFSET(D3DLIGHT9, Type),         "LC_TYPE"},
> +       {FIELD_OFFSET(D3DLIGHT9, Diffuse),      "LT_DIFFUSE"},
> +       {FIELD_OFFSET(D3DLIGHT9, Specular),     "LT_SPECULAR"},
> +       {FIELD_OFFSET(D3DLIGHT9, Ambient),      "LT_AMBIENT"},
> +       {FIELD_OFFSET(D3DLIGHT9, Position),     "LT_POSITION"},
> +       {FIELD_OFFSET(D3DLIGHT9, Direction),    "LT_DIRECTION"},
> +       {FIELD_OFFSET(D3DLIGHT9, Range),        "LT_RANGE"},
> +       {FIELD_OFFSET(D3DLIGHT9, Falloff),      "LT_FALLOFF"},
> +       {FIELD_OFFSET(D3DLIGHT9, Attenuation0), "LT_ATTENUATION0"},
> +       {FIELD_OFFSET(D3DLIGHT9, Attenuation1), "LT_ATTENUATION1"},
> +       {FIELD_OFFSET(D3DLIGHT9, Attenuation2), "LT_ATTENUATION2"},
> +       {FIELD_OFFSET(D3DLIGHT9, Theta),        "LT_THETA"},
> +       {FIELD_OFFSET(D3DLIGHT9, Phi),          "LT_PHI"}
> +    };
> +    switch (op)
> +    {
> +        case LT_TYPE:
> +            TRACE("LT_TYPE %u.\n", *(D3DLIGHTTYPE *)value);
> +            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 %u.\n",op);
> +            break;
> +    }
> +}
> +
> +static void d3dx9_set_material_parameter(enum MATERIAL_TYPE op, D3DMATERIAL9 *material, void *value)
> +{
> +    static const struct
> +    {
> +        int off;
> +        const char *name;
> +    } material_tbl[] =

Same as above.

> +    {
> +       {FIELD_OFFSET(D3DMATERIAL9, Diffuse),  "MT_DIFFUSE"},
> +       {FIELD_OFFSET(D3DMATERIAL9, Ambient),  "MT_AMBIENT"},
> +       {FIELD_OFFSET(D3DMATERIAL9, Specular), "MT_SPECULAR"},
> +       {FIELD_OFFSET(D3DMATERIAL9, Emissive), "MT_EMISSIVE"},
> +       {FIELD_OFFSET(D3DMATERIAL9, Power),    "MT_POWER"}
> +    };
> +
> +    if (op < 0 || op > MT_POWER)
> +    {
> +        FIXME("Unknown material parameter %u.\n",op);
> +        return;
> +    }
> +
> +    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;
> +        }
> +    }
> +}

I probably prefer if you write this in the same fashion as with the
light parameter above (explicit cases for the 4 color values, error
message in the default case of the switch). Actually the error message
should probably be a WARN() instead since I don't think values
different from the ones you're handling are ever legitimate so there
is nothing to fix for us (but the value comes from the application so
it's not an ERR).

> +
>  static HRESULT d3dx9_apply_state(struct ID3DXEffectImpl *effect, struct d3dx_pass *pass, struct d3dx_state *state)
>  {
>      IDirect3DDevice9 *device = effect->device;
> @@ -2554,6 +2662,37 @@ static HRESULT d3dx9_apply_state(struct ID3DXEffectImpl *effect, struct d3dx_pas
>          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_LIGHTENABLE:
> +            TRACE("%s index %u %u.\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 %u index %u.\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);

Please parenthesize the sizeof argument.

> +            }
> +            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 %u index %u.\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);
> diff --git a/dlls/d3dx9_36/tests/effect.c b/dlls/d3dx9_36/tests/effect.c
> index e415a3c..3eaddb7 100644
> --- a/dlls/d3dx9_36/tests/effect.c
> +++ b/dlls/d3dx9_36/tests/effect.c
> @@ -2903,11 +2903,11 @@ static void test_effect_states(IDirect3DDevice9 *device)
>      }
>
>      hr = IDirect3DDevice9_GetLightEnable(device, 2, &bval);
> -    todo_wine ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK).\n", hr);
> +    ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK).\n", hr);
>      if (hr == D3D_OK)
>          ok(bval,"Got result %u, expected TRUE.\n", bval);
>      hr = IDirect3DDevice9_GetLight(device, 2, &light);
> -    todo_wine ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK).\n", hr);
> +    ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK).\n", hr);
>      if (hr == D3D_OK)
>          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);
> @@ -2943,9 +2943,9 @@ static void test_effect_states(IDirect3DDevice9 *device)
>      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);
> +    ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK).\n", hr);
>      if (hr == D3D_OK)
> -        ok(!bval,"Got result %u, expected 0.\n", bval);
> +        todo_wine ok(!bval,"Got result %u, expected 0.\n", bval);
>
>      if (effect)
>          effect->lpVtbl->Release(effect);
> --
> 2.5.0



More information about the wine-devel mailing list