Re: d3dx9_36 [patch 1/2]: Implement D3DXSHEvalDirectionalLight

[Rico Schüller]

On 14.09.2012 11:07, Nozomi Kodama wrote:
> Hello
>
> one remark about this patch.
>
> Test in real windows shows that D3DXSHEvalDirectionalLight accepts order
> > D3DXSH_MAXORDER or order < D3DXSH_MINORDER.
> > But in these cases, the colour outputs are unpredictable. For the same
> > calling arguments, D3DSXHEvalDirectionalLight returns different colours.
>
> That explains the simplicity of the implementation of this function.
>
> Nozomi.

+    FLOAT s, temp[max( 1, order * order )];
Is this allowed in c and does it really work the way one would expect?


+    for (order = D3DXSH_MINORDER; order <= D3DXSH_MAXORDER; order++)
...
+            if ( ( order == 0 ) || ( j >= order * order ) )
Why would you check for order == 0? That's imho never the case that this 
is true. At least if you only test from D3DXSH_MINORDER onward.


+    unsigned int j, order, start[] = {0, 4, 13, 29, 54};
Using the start[] looks a bit ugly ... why don't you calculate the index?


+                expected = table[start[order - 2] + j];    !
You've again trailing white spaces.


What happens in the following test case? Which result is returned?
D3DXSHEvalDirectionalLight(order, &dir, 1.7f, 2.6f, 3.5f, rout, rout, rout);
Well the implementation seems to be correct, but it may be nice to test 
for this as well. Maybe in another patch.

Cheers
Rico

d3dx9_36 [patch 1/2]: Implement D3DXSHEvalDirectionalLight

[Henri Verbeet]

On 14 September 2012 14:04, Rico Schüller <kgbricola(a)web.de> wrote:
> +    FLOAT s, temp[max( 1, order * order )];
> Is this allowed in c and does it really work the way one would expect?

It is in C99 (look into VLAs), but Wine doesn't allow C99 features.