[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?