Incorrect order of parameters for mAtan passed into atan2
by Lateral Punk · in Torque Game Engine · 11/27/2006 (10:41 pm) · 8 replies
It seems that the wrapper function to the standard C library atan function, mAtan, is passing in the parameters in the wrong order. Let's start with what C says the defintion of atan2 is www.cplusplus.com/ref/cmath/atan2.html
Hence, atan2 expects y first and then x (check out the sample program in the above example).
Now look at what mAtan is doing.
It's passing x for Y and y for X! So mAtan doesn't actually give you the arctangent it gives you the arc cotangent (basically the opposite), but apparently nobody noticed so now everything in torque is coded backwards! To fix it, just:
Lateral Punk
Hence, atan2 expects y first and then x (check out the sample program in the above example).
Now look at what mAtan is doing.
inline F64 mAtan(const F64 x, const F64 y)
{
return (F64) atan2(x, y);
}It's passing x for Y and y for X! So mAtan doesn't actually give you the arctangent it gives you the arc cotangent (basically the opposite), but apparently nobody noticed so now everything in torque is coded backwards! To fix it, just:
inline F64 mAtan(const F64 x, const F64 y)
{
return (F64) atan2(y, x);
}Lateral Punk
#2
fixing it would probably break existing code which is relying on the incorrect behaviour.
you'd have to find all existing calls to mAtan() and decide whether or not to swap the params.
.. i count about 24 calls to mAtan() in our current codebase.
another option would be to create a new function named something like mAtanGood() which handles the parameters correctly, and use that for new code.
11/29/2006 (12:04 pm)
Ooh, tough call on this.fixing it would probably break existing code which is relying on the incorrect behaviour.
you'd have to find all existing calls to mAtan() and decide whether or not to swap the params.
.. i count about 24 calls to mAtan() in our current codebase.
another option would be to create a new function named something like mAtanGood() which handles the parameters correctly, and use that for new code.
#3
11/29/2006 (12:10 pm)
24 calls (is that both engine & script) isn't all too bad to fix. Yeah you'll have to decypher the surrounding code and figure out what represents X and what is Y, which might not always be easy. This is a logical bug that should be fixed in head for GG as it definitely can be a source of confusion for future developers.
#4
still, it's a dangerous change to put into head
because that means anyone who updates to say TGE 1.5.1 from TGE 1.5 will find their script code executing differently.
11/29/2006 (12:21 pm)
I count about 24 (+/- a few) calls in C++, and one in script. actually the stock script codebase probably has none in script.still, it's a dangerous change to put into head
because that means anyone who updates to say TGE 1.5.1 from TGE 1.5 will find their script code executing differently.
#5
11/29/2006 (1:25 pm)
Yeah it's dangerous, but also misleading for 'future' developers. Being backwards compatible is one thing, but relying on incorrect behaviour is just dangerous. GG dudes, any input?
#6
I've had to update plenty of code for php in the past as they updated to be more secure. It wasnt a big deal and I learned something new from it, and my old code got cleaned up in the process. Makes a bad impression when stuff doesnt work right.
What script code would this affect? If its just the mAtan I say fix it.
11/29/2006 (1:43 pm)
I have never used that function and probably never will, but my two cents is to fix it. Broken code can be fixed easily by the developer, and they can always use the older versions if they dont want to update. I've had to update plenty of code for php in the past as they updated to be more secure. It wasnt a big deal and I learned something new from it, and my old code got cleaned up in the process. Makes a bad impression when stuff doesnt work right.
What script code would this affect? If its just the mAtan I say fix it.
#7
This is one of those things that wont show up much, but if it does it would be crushing trying to debug.
My concern with my fixing it locally is that everytime I integrate an update, I have to go through and make sure that new calls to mAtan havent' been added. (Assuming I remember this issue four months from now).
Todd
11/29/2006 (5:37 pm)
Yeah, I'd rather see this fixed in head as well.This is one of those things that wont show up much, but if it does it would be crushing trying to debug.
My concern with my fixing it locally is that everytime I integrate an update, I have to go through and make sure that new calls to mAtan havent' been added. (Assuming I remember this issue four months from now).
Todd
#8
11/29/2006 (5:42 pm)
Quote:I couldn't have agreed more...
My concern with my fixing it locally is that everytime I integrate an update, I have to go through and make sure that new calls to mAtan havent' been added. (Assuming I remember this issue four months from now).
Torque 3D Owner Lateral Punk
Default Studio Name
ConsoleFunction( mAtan, F32, 3, 3, "(float rise, float run) Returns the slope in radians (the arc-tangent) of a line with the given rise and run.") { return(mAtan(dAtof(argv[1]), dAtof(argv[2]))); }hence, rise would imply y and run would imply x. So if the careful scripter was using this, and called it like
then they would have been ok. But I've seen much script code that calls it with (x,y). Moreover, engine code that directly uses mAtan is calling it with (x,y) all over the places: