fix computeFaceNormal: use corner whose angle is closest to right angle for cross product

[henkson]

This change provides a fix for issue #130.

• adding a method angle that calculates the angle between 2 Vector3 objects
• modifying the Mesh.computeFaceNormal method: verify the angles of all face vertices, and use the vertex whose angle is the closest to a right angle (90°). This makes the cross product numerically more stable.

[henkson]

Benchmarking the code with over 5000 test meshes doesn't show any performance impact.

[siliconvoodoo]

if you want you can try using a polynomial fit for this. it musnt be precise. look how cute it is when I try the montreal university fit (Sreeraman Rajan et al.)

[henkson]

Not so sure, atan2 should give results in the range [-pi, pi], while atan gives results in the range [-pi/2, pi/2].
Apart from that, that smells like premature optimization imo ("no visible slowdown").

[siliconvoodoo]

yes it is premature optimization. don't touch it, the test showed it's not necessary

[siliconvoodoo]

bunch of warnings you could cleanup

1>xatlas\source\xatlas\xatlas.cpp(821,12): warning C4244: 'return': conversion from 'double' to 'float', possible loss of data
1>xatlas\source\xatlas\xatlas.cpp(2726,26): warning C4244: 'initializing': conversion from 'double' to 'float', possible loss of data
1>xatlas\source\xatlas\xatlas.cpp(2726,21): warning C4244: 'initializing': conversion from 'double' to 'const float', possible loss of data
1>xatlas\source\xatlas\xatlas.cpp(2727,26): warning C4244: 'initializing': conversion from 'double' to 'float', possible loss of data
1>xatlas\source\xatlas\xatlas.cpp(2727,21): warning C4244: 'initializing': conversion from 'double' to 'const float', possible loss of data
1>xatlas\source\xatlas\xatlas.cpp(2728,26): warning C4244: 'initializing': conversion from 'double' to 'float', possible loss of data
1>xatlas\source\xatlas\xatlas.cpp(2728,21): warning C4244: 'initializing': conversion from 'double' to 'const float', possible loss of data

[siliconvoodoo]

I tried it. no visible slowdown. LGTM

[mifth]

Should Vectors be normalized before the cross product?

[mifth]

@siliconvoodoo @henkson
I've found that there is kPi2 variable which I think you should use instead of M_PI_2. The point of the lib is to use all internal things.

[mifth]

And you probably need to remove the #define _USE_MATH_DEFINES.

[henkson]

@siliconvoodoo @henkson I've found that there is kPi2 variable which I think you should use instead of M_PI_2. The point of the lib is to use all internal things.

@mifth
We can't use kPi2 because that is equal to 2 * PI and we need PI / 2 here.
I do understand your "internal things" remark though, I can add a new variable. Do you have a suggestion for its name? (I have no idea where the k prefixes comes from. M_ probably stands for "math"?)

And you probably need to remove the #define _USE_MATH_DEFINES.

That was probably only needed because I used those M_PI and M_PI_2 variables. (Correct? I'm not really a C/C++ developer, and it's almost a year since I wrote this...)

[mifth]

Oh, right! A new variable is needed. About the name I would leave it to you if you don't mind. I believe something like kPi_YourNewName. :)

Yeah it seems the #define _USE_MATH_DEFINES was used for the M_PI_2 only,

[mifth]

Just have got in my mind. What do you think about kPi_Half?

[siliconvoodoo]

the k prefix is just one of usual conventions to denote a hard constant. (usually the type that get inlined at code generation and they have no ODR existence)

[mifth]

I've tested your code with a 200000 triangles mesh. It looks pretty healty. 15-17 seconds both versions. Your version sometimes works even faster (-1 second). I guess it's because of less negative triangles.