Re: [v2 2/3] d3dx9: Implement IsParameterUsed function in effect.

[Matteo Bruni]

2016-05-23 12:40 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
> Signed-off-by: Paul Gofman <gofmanp(a)gmail.com>

It looks generally good, I only have minor "complaints" (and sorry for
the delay in reviewing...).
> ---
>  dlls/d3dx9_36/effect.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 110 insertions(+), 3 deletions(-)
>
> diff --git a/dlls/d3dx9_36/effect.c b/dlls/d3dx9_36/effect.c
> index 31aa2d6..cdfc494 100644
> --- a/dlls/d3dx9_36/effect.c
> +++ b/dlls/d3dx9_36/effect.c
> @@ -3545,15 +3545,122 @@ static HRESULT WINAPI ID3DXEffectImpl_FindNextValidTechnique(ID3DXEffect* iface,
>      return E_NOTIMPL;
>  }
>
> +typedef BOOL (*walk_parameter_dep_func)(void *data, struct d3dx_parameter *param);
> +static BOOL walk_state_dep(struct d3dx_state *state, walk_parameter_dep_func param_func,
> +        void *data);
> +static BOOL walk_param_eval_dep(struct d3dx_param_eval *param_eval, walk_parameter_dep_func param_func,
> +        void *data);

I guess you intend to reuse all these functions down the line for some
other API, what are your plans?
> +static BOOL compare_param_ptr(void *param_comp, struct d3dx_parameter *param)
> +{
> +    return param_comp == (void *)param;

I think the cast is unnecessary here.
> +}
> +
>  static BOOL WINAPI ID3DXEffectImpl_IsParameterUsed(ID3DXEffect* iface, D3DXHANDLE parameter, D3DXHANDLE technique)
>  {
>      struct ID3DXEffectImpl *effect = impl_from_ID3DXEffect(iface);
> +    unsigned int i, j;
>      struct d3dx_parameter *param = get_valid_parameter(&effect->base_effect, parameter);
> +    struct d3dx_technique *tech = get_valid_technique(&effect->base_effect, technique);
> +    struct d3dx_pass *pass;
>
> -    FIXME("iface %p, parameter %p, technique %p stub.\n", iface, parameter, technique);
> -    TRACE("param %p (%s).\n", param, param ? debugstr_a(param->name) : "");
> +    TRACE("iface %p, parameter %p, technique %p.\n", iface, parameter, technique);
> +    TRACE("param %p, name %s, tech %p.\n", param, param ? debugstr_a(param->name) : "", tech);
> +    if (!tech || !param)
> +        return FALSE;
>
> -    return TRUE;
> +    for (i = 0; i < tech->pass_count; ++i)
> +    {
> +        pass = &tech->passes[i];
> +        for (j = 0; j < pass->state_count; ++j)
> +        {
> +            if (walk_state_dep(&pass->states[j], compare_param_ptr, param))
> +            {
> +                TRACE("technique %p, parameter %s, TRUE.\n", tech, param ? debugstr_a(param->name) : "");

I don't think you need to trace technique and parameter again, they
are in the immediately preceding ID3DXEffectImpl_IsParameterUsed()
TRACE anyway. Maybe the message could simply be "Returning TRUE.\n" or
similar.
> +                return TRUE;
> +            }
> +        }
> +    }
> +    TRACE("technique %p, parameter %s, FALSE.\n", tech, param ? debugstr_a(param->name) : "");

Same here.
> +    return FALSE;
>  }
>
> static HRESULT WINAPI ID3DXEffectImpl_Begin(ID3DXEffect *iface, UINT *passes, DWORD flags)
> --
> 2.5.5

[v2 2/3] d3dx9: Implement IsParameterUsed function in effect.

[Paul Gofman]

On 05/26/2016 06:10 PM, Matteo Bruni wrote:
> 2016-05-23 12:40 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
> > Signed-off-by: Paul Gofman <gofmanp(a)gmail.com>
> > It looks generally good, I only have minor "complaints" (and sorry for
> > the delay in reviewing...).
> > ---
> >  dlls/d3dx9_36/effect.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 110 insertions(+), 3 deletions(-)
> >
> > diff --git a/dlls/d3dx9_36/effect.c b/dlls/d3dx9_36/effect.c
> > index 31aa2d6..cdfc494 100644
> > --- a/dlls/d3dx9_36/effect.c
> > +++ b/dlls/d3dx9_36/effect.c
> > @@ -3545,15 +3545,122 @@ static HRESULT WINAPI ID3DXEffectImpl_FindNextValidTechnique(ID3DXEffect* iface,
> >      return E_NOTIMPL;
> >  }
> >
> > +typedef BOOL (*walk_parameter_dep_func)(void *data, struct d3dx_parameter *param);
> > +static BOOL walk_state_dep(struct d3dx_state *state, walk_parameter_dep_func param_func,
> > +        void *data);
> > +static BOOL walk_param_eval_dep(struct d3dx_param_eval *param_eval, walk_parameter_dep_func param_func,
> > +        void *data);
> > I guess you intend to reuse all these functions down the line for some
> > other API, what are your plans?
> > Well, I intended to reuse, but later realized that the other "walk"
> > function I need in 'dirty' param tracking on CommitChanges
> > implementation are quite different as walk parameter members tree and
> > not dependencies. Now I am reusing just typedef for the tree mapped
> > function, I will remove one prototype and move another one near to the
> > function (I will still need one as there are recursive calls between the
> > two functions).