Hi,
On Mon, Jul 27, 2015 at 7:38 PM, Matteo Bruni matteo.mystral@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.