[PATCH 3/5] d3dx9: Partially implement D3DXComputeTangentFrameEx().

[Józef Kucia]

Hi,

On Mon, Jul 27, 2015 at 7:38 PM, Matteo Bruni <matteo.mystral at gmail.com> wrote:
>
> I've got a number of nitpicks but these patches are essentially okay with me.
>

Thanks for the review.

>> +
>> +    switch (declaration->Type)
>> +    {
>> +        case D3DDECLTYPE_FLOAT1:
>> +            vec3.x = src[0];
>> +            break;
>> +        case D3DDECLTYPE_FLOAT2:
>> +            vec3.x = src[0];
>> +            vec3.y = src[1];
>> +            break;
>> +        case D3DDECLTYPE_FLOAT3:
>> +        case D3DDECLTYPE_FLOAT4:
>> +            vec3.x = src[0];
>> +            vec3.y = src[1];
>> +            vec3.z = src[2];
>> +            break;
>> +        default:
>> +            ERR("Cannot read vec3\n");
>> +            break;
>> +    }
>
> You could sort the cases the other way around and fallthrough to make
> this part shorter. Again, this is also fine and it's up to you.
>

I was thinking about this but it seems slightly less readable to me.

>> +    }
>> +
>> +    if ((options & (D3DXTANGENT_WEIGHT_EQUAL | D3DXTANGENT_WEIGHT_BY_AREA)) == (D3DXTANGENT_WEIGHT_EQUAL | D3DXTANGENT_WEIGHT_BY_AREA))
>
> I would store the result of "options & (D3DXTANGENT_WEIGHT_EQUAL |
> D3DXTANGENT_WEIGHT_BY_AREA)" in a variable and use it here and below.
>

Definitely good suggestion.

>> +
>> +    for (i = 0; i < num_vertices; i++)
>> +    {
>> +        const D3DXVECTOR4 zero = {0.0f, 0.0f, 0.0f, 1.0f};
>
> That's a weird zero :) Not sure about a better name though, maybe
> "default_vector"? Also, please make it static.

I was considering "default_vector" but i left it as "zero". I guess I
have a bad taste ;)

>> +
>> +                if (isnan(weights[0])) weights[0] = 1.0f;
>> +                if (isnan(weights[1])) weights[1] = 1.0f;
>> +                if (isnan(weights[2])) weights[2] = 1.0f;
>> +                break;
>
> This might cause problems in the (unlikely) case float exceptions are
> enabled. Ideally this case would need to be tested with native and, if
> it doesn't throw exceptions, your code would need to take care of
> that. I don't think this is super critical though, it should be easy
> to figure it out later on if some application crashes here.

I'll test this case.