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

[Paul Gofman]

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.

     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.