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

[Paul Gofman]

On 03/16/2017 01:39 AM, Matteo Bruni wrote:
> > -
> >        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)?

Yes, sure.
> > - 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.

The reason I left ERR in d3dx9_get_param_value_ptr is now our 
implementation for out of bounds selectors errors handling is different 
from native implementation for certain cases. Should I still avoid ERR 
or FIXME, or maybe change TRACE("d3dx9_get_param_value_ptr error 
%#x.\n", hr); to ERR("d3dx9_get_param_value_ptr error %#x, returning 
D3D_OK.\n", hr); when returning OK status only?
> > -
> >    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.

If it is FXLC or ARRAY_SELECTOR peval->shader_inputs is an empty table 
with zero count, and I thought that doing a check in 
is_const_tab_input_dirty() with zero count in table is not much 
different from explicitly checking the type of param_eval, while it is 
shorter/simpler and more generic. Maybe I could leave it this way?
> > - 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.

This chunk actually avoids redundant preshader recompute. I don't quite 
understand how can I do it as a separate previous patch, as this patch 
introduce "update_all" parameter, is_const_tab_input_dirty() function 
and all the other logic around it. Or can it be that you were thinking 
of changes on light and material state update, and copy pasted different 
chunk?