[PATCH 3/6] d3dx9: Match native out of bounds array selector index handling.

[Matteo Bruni]

2017-05-15 23:25 GMT+02:00 Paul Gofman <gofmanp at gmail.com>:
> On 05/15/2017 09:21 PM, Matteo Bruni wrote:
>>
>> 2017-05-12 14:24 GMT+02:00 Paul Gofman <gofmanp at gmail.com>:
>>
>>> diff --git a/dlls/d3dx9_36/effect.c b/dlls/d3dx9_36/effect.c
>>> index 9c07f9c..21aa5d7 100644
>>> --- a/dlls/d3dx9_36/effect.c
>>> +++ b/dlls/d3dx9_36/effect.c
>>> @@ -1051,7 +1051,9 @@ static HRESULT d3dx9_get_param_value_ptr(struct
>>> d3dx_pass *pass, struct d3dx_sta
>>>                   FIXME("Preshader structure is null.\n");
>>>                   return D3DERR_INVALIDCALL;
>>>               }
>>> -            if (update_all ||
>>> is_param_eval_input_dirty(param->param_eval))
>>> +            /* According to the tests, native d3dx handles the case of
>>> array index evaluated to -1
>>> +             * in a specific way. */
>>> +            if (state->index == 0xffffffffu ||
>>> is_param_eval_input_dirty(param->param_eval, pass->update_version))
>>
>> Hmm, so that's the index value set at initialization, thus getting a
>> -1 in the first evaluation ends up breaking the "selector changed"
>> check in native? You might want to spell that out in some form in the
>> comment.
>
>     Well, it now looks like a leftover, it looks like now with the current
> version of the patch when parameter evaluation is fully transactional and
> actual recompute does not depend on update_all flag, this check
> 'state->index == 0xffffffffu' is redundant and I can just remove it, it does
> not break any test. I think it was needed on some of my previous versions of
> patches.
>
>     Now this part can be removed, it should be just
> if (is_param_eval_input_dirty(param->param_eval))
> ...
>
>     The check for array_idx == 0xffffffffu below is required though to match
> a special behaviour on -1 index value handling.
>
>>
>> AFAIU there is no other way a -1 is stored in state->index, correct?
>>
>>
> There is the other way to get -1 to index selector, it can be computed by
> preshader with corresponding parameters values set, and it is in the test.

Isn't array_idx set to 0 in that case before being stored in
state->index though?

>     The overall logic I could "interpolate" from the tests is:
>
> 1. Whenever BeginPass or GetPassDesc return a success while the actual index
> value should be out of bound, they return the value for previously computed
> "good" index;
>
> 2. Unlike BeginPass and GetPassDesc, CommitChanges never returns failure on
> out of bound index, though still does not set a state in the case when
> BeginPass would return an error. BeginPass and GetPassDesc can either return
> failulre or not with the same actual index value to be computed, depending
> on what was happening before.
>
> 3. Whenever index recompute is required (based on updated parameters), the
> out of bound check is performed and error is returned from BeginPass or
> GetPassDesc. When the recomputed index is out of bound stored index value is
> never updated.
>
> 4. When the index recompute is not required (there was BeginPass or
> CommitChanges since last index parameter update, even if it computed out of
> bound index), the array element corresponding to the previous ever
> successfully selected is used.
>
> 5. If the index value is '-1', first array element is selected, and no
> errors are returned in this case.
>
> 6. GetPassDesc() does not 'reset' the modified parameters, multiple
> successive calls to it consistently return failure. If the 'modified state'
> of index was reset by BeginPass() or CommitChanges(), GetPassDesc() will
> also return success with previous good selection, just like BeginPass().
>
>     All this looks a bit like dragon poker to me, but now with transactional
> update logic just a special treatment of '-1' value and allowing a sort of
> bug not modifying previous index on out of bound compute seems to match all
> this.

Okay, I went back and rechecked the tests. I see that I missed a few
bits and that -1 is actually weird.

Not that it really matters, I'm just trying to explain this as
accidental behavior rather than some weird kind of intentional
decision on MS's part. It's probably still the case but I fail to see
how it would happen. Oh well.