Re: [v2 2/6] d3dx9: Apply changed states only in CommitChanges.

[Matteo Bruni]

2017-03-14 13:46 GMT+01:00 Paul Gofman :
> Signed-off-by: Paul Gofman

The current subject might be slightly confusing, something like
"d3dx9: Don't apply unmodified states in CommitChanges." would be
probably better.

The patch looks generally good. Comments follow.
> v2:
>     - stored current_light and current_material in effect structure instead of pass;
>     - renamed clear_dirty_all() -> clear_dirty_params().
>
> +static BOOL param_clear_dirty_func(void *dummy, struct d3dx_parameter *param)
> +{
> - *param->dirty_flag_ptr &= ~PARAMETER_FLAG_DIRTY;
> - return FALSE;
>
> +}
> -
>
> +static void clear_dirty_params(struct d3dx9_base_effect *base)
> +{
> - unsigned int i;
> -
> - for (i = 0; i < base->parameter_count; ++i)
> -
>    walk_parameter_tree(&base->parameters[i], param_clear_dirty_func, NULL);
>
> +}

The walk seems unnecessary since we know that all the children
parameters share their dirty flag with their parents.
> -
>        param = param->referenced_param;
> -
>        TRACE("Array index %u.\n", array_idx);
> -
>        if (array_idx >= param->element_count)
> -
>        if (array_idx >= ref_param->element_count)
>        {
> -
>            ERR("Computed array index %u is out of bound %u.\n", array_idx, param->element_count);
> -
>            ERR("Computed array index %u is out of bound %u.\n", array_idx, ref_param->element_count);
>            return D3DERR_INVALIDCALL;
>        }
> -
>        param = &param->members[array_idx];
> -
>        param = &ref_param->members[array_idx];
> -
>        *param_dirty = state->index != array_idx || is_param_dirty(param);

This looked a bit weird to me at first because of reusing "param".
Maybe use a separate "selected_param" variable (or something to that
effect)?
> - if (FAILED(hr = d3dx9_get_param_value_ptr(effect, pass, state, &param_value, &param)))
> - if (FAILED(hr = d3dx9_get_param_value_ptr(effect, pass, state, &param_value, &param,
> -
>        update_all, &param_dirty)))
>    /* Native d3dx returns D3D_OK from BeginPass or Commit involving out of bounds array
> -
>     * access and does not touch affected state. */
> -
>     * access and does not touch affected state, except for BeginPass fails with E_FAIL if
> -
>     * out of bounds array index depends on dirty parameter(s) in BeginPass. The latter case
> -
>     * is TODO. */

Nitpicking on the comment, it sounds better to me with something like
"...except for BeginPass when the out of bounds array index depends on
dirty parameters. The latter case is supposed to return E_FAIL but is
currently TODO".

BTW, the ERR in d3dx9_get_param_value_ptr() should probably be a WARN
since it can be triggered by the application.
> -
>    TRACE("d3dx9_get_param_value_ptr error %#x.\n", hr);

This one shouldn't be necessary, I think there should have been a more
specific error / warning printed out by d3dx9_get_param_value_ptr() or
one of its callees.
Otherwise you could repurpose this to say that you're returning D3D_OK
notwithstanding the failure.
> -
>    return D3D_OK;
> - }

Actually, it's probably better to change the
d3dx9_get_param_value_ptr() return on out of bounds to E_FAIL and only
override the return value to D3D_OK in that specific case.
> +BOOL is_param_eval_input_dirty(struct d3dx_param_eval *peval)
> +{
> - return is_const_tab_input_dirty(&peval->pres.inputs)
> -
>        || is_const_tab_input_dirty(&peval->shader_inputs);
>
> +}

If I'm not mistaken this only needs to check pres.inputs, given that
it's only reasonably going to be called on FXLC and ARRAY_SELECTOR
preshaders (not on actual shaders' preshaders). It doesn't matter much
though.
> - set_constants(rs, &pres->inputs);
> - if (FAILED(hr = execute_preshader(pres)))
> -
>    return hr;
> - if (update_all || is_const_tab_input_dirty(&pres->inputs))
> - {
> -
>    set_constants(rs, &pres->inputs, TRUE);
> -
>    if (FAILED(hr = execute_preshader(pres)))
> -
>        return hr;
> -
>    regstore_reset_table(rs, PRES_REGTAB_CONST);
> - }

This is technically a separate bugfix which would be better as a
separate patch, before this one.