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

[Matteo Bruni]

2017-03-16 10:52 GMT+01:00 Paul Gofman <gofmanp at gmail.com>:
> On 03/16/2017 01:39 AM, Matteo Bruni wrote:

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

I think WARN is what you want to use in d3dx9_get_param_value_ptr(). A
FIXME for the broken case only would be okay but at that point you
could just fix it properly as well so I don't think you need anything
else. The todo_wine in the tests are enough.
You can keep the TRACE when returning D3D_OK (definitely not ERR) but
I think it's better to only override the return on out-of-bounds, so
the thing with using a different HRESULT for that 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?

Yeah, it's fine.

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

I just meant the regstore_reset_table() part.