[script]Pitch broken for REAL now.
Moderator: GZDoom Developers
Forum rules
Please don't bump threads here if you have a problem - it will often be forgotten about if you do. Instead, make a new thread here.
Please don't bump threads here if you have a problem - it will often be forgotten about if you do. Instead, make a new thread here.
- Major Cooke
- Posts: 8176
- Joined: Sun Jan 28, 2007 3:55 pm
- Preferred Pronouns: He/Him
- Location: QZDoom Maintenance Team
[script]Pitch broken for REAL now.
http://zdoom.org/Changelog/e3bf1dd/files
Turns out this commit was actually doing the inverse of what it's supposed to do. Weapons above aiming flat out straight ahead on up will kick the pitch all the way to the top.
Turns out this commit was actually doing the inverse of what it's supposed to do. Weapons above aiming flat out straight ahead on up will kick the pitch all the way to the top.
- Major Cooke
- Posts: 8176
- Joined: Sun Jan 28, 2007 3:55 pm
- Preferred Pronouns: He/Him
- Location: QZDoom Maintenance Team
Re: [script]Pitch broken for REAL now.
https://github.com/rheit/zdoom/pull/549
Taking a look back, angle_t parameter turns out to have been the same the entire time even before the scripting merge.
Taking a look back, angle_t parameter turns out to have been the same the entire time even before the scripting merge.
-
-
- Posts: 3144
- Joined: Wed Nov 24, 2004 12:59 pm
- Graphics Processor: ATI/AMD with Vulkan/Metal Support
- Contact:
Re: [script]Pitch broken for REAL now.
So apparently x86_64 has quirky double->int behavior as well: https://social.msdn.microsoft.com/Forum ... lcppnative
Looks like code is needed to make sure that the value is in range [0-360) before casting.
Looks like code is needed to make sure that the value is in range [0-360) before casting.
Re: [script]Pitch broken for REAL now.
What about double->int64 like it shows in your link?
-
-
- Posts: 3144
- Joined: Wed Nov 24, 2004 12:59 pm
- Graphics Processor: ATI/AMD with Vulkan/Metal Support
- Contact:
Re: [script]Pitch broken for REAL now.
If you're not concerned about the performance of always doing that then go for it. As long as the input value is within range of the destination type then it should be a defined conversion.
Re: [script]Pitch broken for REAL now.
This basically means the upper range of a valid conversion from Double to int64 is about... 0x7FFFFFFFFFFFF000?
- Graf Zahl
- Lead GZDoom+Raze Developer
- Posts: 49073
- Joined: Sat Jul 19, 2003 10:19 am
- Location: Germany
Re: [script]Pitch broken for REAL now.
Crap. I think this all should be refactored into a FLOAT2ANGLE macro that can be implemented differently for the respective platforms. If each one is different, it makes no sense to write out a one-size-fits-all conversion each time.
- Graf Zahl
- Lead GZDoom+Raze Developer
- Posts: 49073
- Joined: Sat Jul 19, 2003 10:19 am
- Location: Germany
Re: [script]Pitch broken for REAL now.
Ok, one question before all go mad here:
Is this only for compiler provided float to int conversions or are the xs_RoundToInt functions also affected? My guess would be no, because they store the float as an actual float and then read out parts of that with an integer instruction - without any hardware weirdness.
Is this only for compiler provided float to int conversions or are the xs_RoundToInt functions also affected? My guess would be no, because they store the float as an actual float and then read out parts of that with an integer instruction - without any hardware weirdness.
- Graf Zahl
- Lead GZDoom+Raze Developer
- Posts: 49073
- Joined: Sat Jul 19, 2003 10:19 am
- Location: Germany
Re: [script]Pitch broken for REAL now.
BTW, here's something interesting I just found. Look what the moving camera does with the pitch:
I'd guess this can be filed under 'worked by accident'...
Code: Select all
if (args[2] & 1)
{ // linear
if (args[2] & 4)
{ // interpolate pitch
pitch = FLOAT2FIXED(Lerp (FIXED2FLOAT(CurrNode->pitch), FIXED2FLOAT(CurrNode->Next->pitch)));
}
}
else
{ // spline
if (args[2] & 4)
{ // interpolate pitch
pitch = FLOAT2FIXED(Splerp (FIXED2FLOAT(PrevNode->pitch), FIXED2FLOAT(CurrNode->pitch),
FIXED2FLOAT(CurrNode->Next->pitch), FIXED2FLOAT(CurrNode->Next->Next->pitch)));
}
}
- Graf Zahl
- Lead GZDoom+Raze Developer
- Posts: 49073
- Joined: Sat Jul 19, 2003 10:19 am
- Location: Germany
Re: [script]Pitch broken for REAL now.
More info on this mess.
I added these:
and this testing CCMD
And here's the output:
That was on 32bit Windows with a debug compile. (Release is identical with Visual Studio 2013)
So, xs_CRoundToInt works correctly, so does the unsigned cast, but the signed int cast doesn't. Why?
Actually quite simple:
The unsigned cast uses an inlined intrinsic that performs x87 math specially tailored to that case.
The signed cast calls a function 'ftol_sse'. Which performs
Yes, SSE math!
So, to summarize:
- we cannot use unsigned casts, because ARM does not like them.
- we cannot use signed casts because x86-SSE and x64 do not like them.
- xs_CRoundToInt completely circumvents the floating point math units and only depends on the bit ordering in a double. And that's identical everywhere.
I'm going to test this on ARM, too, but I have used xs_Float on iOS and Android before and it worked fine there, so this seems indeed to be the solution.
I think ZDBSP needs to have this addressed as well...
I added these:
Code: Select all
#define ANGLE2DBL(f) ((f) * (180./ANGLE_180))
#define ANGLE2FLOAT(f) (float((f) * (180./ANGLE_180)))
#define FLOAT2ANGLE(f) ((angle_t)xs_CRoundToInt((f) * (ANGLE_180/180.)))
#define ANGLE2RAD(f) ((f) * (M_PI/ANGLE_180))
#define ANGLE2RADF(f) ((f) * float(M_PI/ANGLE_180))
#define RAD2ANGLE(f) ((angle_t)xs_CRoundToInt((f) * (ANGLE_180/M_PI)))
Code: Select all
CCMD(angleconvtest)
{
Printf("Testing degrees to angle conversion:\n");
for (double ang = -5 * 180.; ang < 5 * 180.; ang += 45.)
{
angle_t ang1 = FLOAT2ANGLE(ang);
angle_t ang2 = (angle_t)(ang * (ANGLE_180 / 180.));
angle_t ang3 = (angle_t)(int)(ang * (ANGLE_180 / 180.));
Printf("Angle = %.5f: xs_RoundToInt = %08x, unsigned cast = %08x, signed cast = %08x\n",
ang, ang1, ang2, ang3);
}
}
Code: Select all
Testing degrees to angle conversion:
Angle = -900.00000: xs_RoundToInt = 80000000, unsigned cast = 80000000, signed cast = 80000000
Angle = -855.00000: xs_RoundToInt = a0000000, unsigned cast = a0000000, signed cast = 80000000
Angle = -810.00000: xs_RoundToInt = c0000000, unsigned cast = c0000000, signed cast = 80000000
Angle = -765.00000: xs_RoundToInt = e0000000, unsigned cast = e0000000, signed cast = 80000000
Angle = -720.00000: xs_RoundToInt = 00000000, unsigned cast = 00000000, signed cast = 80000000
Angle = -675.00000: xs_RoundToInt = 20000000, unsigned cast = 20000000, signed cast = 80000000
Angle = -630.00000: xs_RoundToInt = 40000000, unsigned cast = 40000000, signed cast = 80000000
Angle = -585.00000: xs_RoundToInt = 60000000, unsigned cast = 60000000, signed cast = 80000000
Angle = -540.00000: xs_RoundToInt = 80000000, unsigned cast = 80000000, signed cast = 80000000
Angle = -495.00000: xs_RoundToInt = a0000000, unsigned cast = a0000000, signed cast = 80000000
Angle = -450.00000: xs_RoundToInt = c0000000, unsigned cast = c0000000, signed cast = 80000000
Angle = -405.00000: xs_RoundToInt = e0000000, unsigned cast = e0000000, signed cast = 80000000
Angle = -360.00000: xs_RoundToInt = 00000000, unsigned cast = 00000000, signed cast = 80000000
Angle = -315.00000: xs_RoundToInt = 20000000, unsigned cast = 20000000, signed cast = 80000000
Angle = -270.00000: xs_RoundToInt = 40000000, unsigned cast = 40000000, signed cast = 80000000
Angle = -225.00000: xs_RoundToInt = 60000000, unsigned cast = 60000000, signed cast = 80000000
Angle = -180.00000: xs_RoundToInt = 80000000, unsigned cast = 80000000, signed cast = 80000000
Angle = -135.00000: xs_RoundToInt = a0000000, unsigned cast = a0000000, signed cast = a0000000
Angle = -90.00000: xs_RoundToInt = c0000000, unsigned cast = c0000000, signed cast = c0000000
Angle = -45.00000: xs_RoundToInt = e0000000, unsigned cast = e0000000, signed cast = e0000000
Angle = 0.00000: xs_RoundToInt = 00000000, unsigned cast = 00000000, signed cast = 00000000
Angle = 45.00000: xs_RoundToInt = 20000000, unsigned cast = 20000000, signed cast = 20000000
Angle = 90.00000: xs_RoundToInt = 40000000, unsigned cast = 40000000, signed cast = 40000000
Angle = 135.00000: xs_RoundToInt = 60000000, unsigned cast = 60000000, signed cast = 60000000
Angle = 180.00000: xs_RoundToInt = 80000000, unsigned cast = 80000000, signed cast = 80000000
Angle = 225.00000: xs_RoundToInt = a0000000, unsigned cast = a0000000, signed cast = 80000000
Angle = 270.00000: xs_RoundToInt = c0000000, unsigned cast = c0000000, signed cast = 80000000
Angle = 315.00000: xs_RoundToInt = e0000000, unsigned cast = e0000000, signed cast = 80000000
Angle = 360.00000: xs_RoundToInt = 00000000, unsigned cast = 00000000, signed cast = 80000000
Angle = 405.00000: xs_RoundToInt = 20000000, unsigned cast = 20000000, signed cast = 80000000
Angle = 450.00000: xs_RoundToInt = 40000000, unsigned cast = 40000000, signed cast = 80000000
Angle = 495.00000: xs_RoundToInt = 60000000, unsigned cast = 60000000, signed cast = 80000000
Angle = 540.00000: xs_RoundToInt = 80000000, unsigned cast = 80000000, signed cast = 80000000
Angle = 585.00000: xs_RoundToInt = a0000000, unsigned cast = a0000000, signed cast = 80000000
Angle = 630.00000: xs_RoundToInt = c0000000, unsigned cast = c0000000, signed cast = 80000000
Angle = 675.00000: xs_RoundToInt = e0000000, unsigned cast = e0000000, signed cast = 80000000
Angle = 720.00000: xs_RoundToInt = 00000000, unsigned cast = 00000000, signed cast = 80000000
Angle = 765.00000: xs_RoundToInt = 20000000, unsigned cast = 20000000, signed cast = 80000000
Angle = 810.00000: xs_RoundToInt = 40000000, unsigned cast = 40000000, signed cast = 80000000
Angle = 855.00000: xs_RoundToInt = 60000000, unsigned cast = 60000000, signed cast = 80000000
So, xs_CRoundToInt works correctly, so does the unsigned cast, but the signed int cast doesn't. Why?
Actually quite simple:
The unsigned cast uses an inlined intrinsic that performs x87 math specially tailored to that case.
The signed cast calls a function 'ftol_sse'. Which performs
Code: Select all
0159EC82 fstp qword ptr [esp]
0159EC85 cvttsd2si eax,mmword ptr [esp]
So, to summarize:
- we cannot use unsigned casts, because ARM does not like them.
- we cannot use signed casts because x86-SSE and x64 do not like them.
- xs_CRoundToInt completely circumvents the floating point math units and only depends on the bit ordering in a double. And that's identical everywhere.
I'm going to test this on ARM, too, but I have used xs_Float on iOS and Android before and it worked fine there, so this seems indeed to be the solution.
I think ZDBSP needs to have this addressed as well...
-
- Posts: 1774
- Joined: Sat Oct 17, 2009 9:40 am
Re: [script]Pitch broken for REAL now.
Just for information, I get the same output in linux x64 compiled with debug and gcc:
[edit]and if I change the 'int' cast in 'ang3' assignment to 'int64_t', it works fine:
Code: Select all
Testing degrees to angle conversion:
Angle = -900.00000: xs_RoundToInt = 80000000, unsigned cast = 80000000, signed cast = 80000000
Angle = -855.00000: xs_RoundToInt = a0000000, unsigned cast = a0000000, signed cast = 80000000
Angle = -810.00000: xs_RoundToInt = c0000000, unsigned cast = c0000000, signed cast = 80000000
Angle = -765.00000: xs_RoundToInt = e0000000, unsigned cast = e0000000, signed cast = 80000000
Angle = -720.00000: xs_RoundToInt = 00000000, unsigned cast = 00000000, signed cast = 80000000
Angle = -675.00000: xs_RoundToInt = 20000000, unsigned cast = 20000000, signed cast = 80000000
Angle = -630.00000: xs_RoundToInt = 40000000, unsigned cast = 40000000, signed cast = 80000000
Angle = -585.00000: xs_RoundToInt = 60000000, unsigned cast = 60000000, signed cast = 80000000
Angle = -540.00000: xs_RoundToInt = 80000000, unsigned cast = 80000000, signed cast = 80000000
Angle = -495.00000: xs_RoundToInt = a0000000, unsigned cast = a0000000, signed cast = 80000000
Angle = -450.00000: xs_RoundToInt = c0000000, unsigned cast = c0000000, signed cast = 80000000
Angle = -405.00000: xs_RoundToInt = e0000000, unsigned cast = e0000000, signed cast = 80000000
Angle = -360.00000: xs_RoundToInt = 00000000, unsigned cast = 00000000, signed cast = 80000000
Angle = -315.00000: xs_RoundToInt = 20000000, unsigned cast = 20000000, signed cast = 80000000
Angle = -270.00000: xs_RoundToInt = 40000000, unsigned cast = 40000000, signed cast = 80000000
Angle = -225.00000: xs_RoundToInt = 60000000, unsigned cast = 60000000, signed cast = 80000000
Angle = -180.00000: xs_RoundToInt = 80000000, unsigned cast = 80000000, signed cast = 80000000
Angle = -135.00000: xs_RoundToInt = a0000000, unsigned cast = a0000000, signed cast = a0000000
Angle = -90.00000: xs_RoundToInt = c0000000, unsigned cast = c0000000, signed cast = c0000000
Angle = -45.00000: xs_RoundToInt = e0000000, unsigned cast = e0000000, signed cast = e0000000
Angle = 0.00000: xs_RoundToInt = 00000000, unsigned cast = 00000000, signed cast = 00000000
Angle = 45.00000: xs_RoundToInt = 20000000, unsigned cast = 20000000, signed cast = 20000000
Angle = 90.00000: xs_RoundToInt = 40000000, unsigned cast = 40000000, signed cast = 40000000
Angle = 135.00000: xs_RoundToInt = 60000000, unsigned cast = 60000000, signed cast = 60000000
Angle = 180.00000: xs_RoundToInt = 80000000, unsigned cast = 80000000, signed cast = 80000000
Angle = 225.00000: xs_RoundToInt = a0000000, unsigned cast = a0000000, signed cast = 80000000
Angle = 270.00000: xs_RoundToInt = c0000000, unsigned cast = c0000000, signed cast = 80000000
Angle = 315.00000: xs_RoundToInt = e0000000, unsigned cast = e0000000, signed cast = 80000000
Angle = 360.00000: xs_RoundToInt = 00000000, unsigned cast = 00000000, signed cast = 80000000
Angle = 405.00000: xs_RoundToInt = 20000000, unsigned cast = 20000000, signed cast = 80000000
Angle = 450.00000: xs_RoundToInt = 40000000, unsigned cast = 40000000, signed cast = 80000000
Angle = 495.00000: xs_RoundToInt = 60000000, unsigned cast = 60000000, signed cast = 80000000
Angle = 540.00000: xs_RoundToInt = 80000000, unsigned cast = 80000000, signed cast = 80000000
Angle = 585.00000: xs_RoundToInt = a0000000, unsigned cast = a0000000, signed cast = 80000000
Angle = 630.00000: xs_RoundToInt = c0000000, unsigned cast = c0000000, signed cast = 80000000
Angle = 675.00000: xs_RoundToInt = e0000000, unsigned cast = e0000000, signed cast = 80000000
Angle = 720.00000: xs_RoundToInt = 00000000, unsigned cast = 00000000, signed cast = 80000000
Angle = 765.00000: xs_RoundToInt = 20000000, unsigned cast = 20000000, signed cast = 80000000
Angle = 810.00000: xs_RoundToInt = 40000000, unsigned cast = 40000000, signed cast = 80000000
Angle = 855.00000: xs_RoundToInt = 60000000, unsigned cast = 60000000, signed cast = 80000000
Code: Select all
Testing degrees to angle conversion:
Angle = -900.00000: xs_RoundToInt = 80000000, unsigned cast = 80000000, signed cast = 80000000
Angle = -855.00000: xs_RoundToInt = a0000000, unsigned cast = a0000000, signed cast = a0000000
Angle = -810.00000: xs_RoundToInt = c0000000, unsigned cast = c0000000, signed cast = c0000000
Angle = -765.00000: xs_RoundToInt = e0000000, unsigned cast = e0000000, signed cast = e0000000
Angle = -720.00000: xs_RoundToInt = 00000000, unsigned cast = 00000000, signed cast = 00000000
Angle = -675.00000: xs_RoundToInt = 20000000, unsigned cast = 20000000, signed cast = 20000000
Angle = -630.00000: xs_RoundToInt = 40000000, unsigned cast = 40000000, signed cast = 40000000
Angle = -585.00000: xs_RoundToInt = 60000000, unsigned cast = 60000000, signed cast = 60000000
Angle = -540.00000: xs_RoundToInt = 80000000, unsigned cast = 80000000, signed cast = 80000000
Angle = -495.00000: xs_RoundToInt = a0000000, unsigned cast = a0000000, signed cast = a0000000
Angle = -450.00000: xs_RoundToInt = c0000000, unsigned cast = c0000000, signed cast = c0000000
Angle = -405.00000: xs_RoundToInt = e0000000, unsigned cast = e0000000, signed cast = e0000000
Angle = -360.00000: xs_RoundToInt = 00000000, unsigned cast = 00000000, signed cast = 00000000
Angle = -315.00000: xs_RoundToInt = 20000000, unsigned cast = 20000000, signed cast = 20000000
Angle = -270.00000: xs_RoundToInt = 40000000, unsigned cast = 40000000, signed cast = 40000000
Angle = -225.00000: xs_RoundToInt = 60000000, unsigned cast = 60000000, signed cast = 60000000
Angle = -180.00000: xs_RoundToInt = 80000000, unsigned cast = 80000000, signed cast = 80000000
Angle = -135.00000: xs_RoundToInt = a0000000, unsigned cast = a0000000, signed cast = a0000000
Angle = -90.00000: xs_RoundToInt = c0000000, unsigned cast = c0000000, signed cast = c0000000
Angle = -45.00000: xs_RoundToInt = e0000000, unsigned cast = e0000000, signed cast = e0000000
Angle = 0.00000: xs_RoundToInt = 00000000, unsigned cast = 00000000, signed cast = 00000000
Angle = 45.00000: xs_RoundToInt = 20000000, unsigned cast = 20000000, signed cast = 20000000
Angle = 90.00000: xs_RoundToInt = 40000000, unsigned cast = 40000000, signed cast = 40000000
Angle = 135.00000: xs_RoundToInt = 60000000, unsigned cast = 60000000, signed cast = 60000000
Angle = 180.00000: xs_RoundToInt = 80000000, unsigned cast = 80000000, signed cast = 80000000
Angle = 225.00000: xs_RoundToInt = a0000000, unsigned cast = a0000000, signed cast = a0000000
Angle = 270.00000: xs_RoundToInt = c0000000, unsigned cast = c0000000, signed cast = c0000000
Angle = 315.00000: xs_RoundToInt = e0000000, unsigned cast = e0000000, signed cast = e0000000
Angle = 360.00000: xs_RoundToInt = 00000000, unsigned cast = 00000000, signed cast = 00000000
Angle = 405.00000: xs_RoundToInt = 20000000, unsigned cast = 20000000, signed cast = 20000000
Angle = 450.00000: xs_RoundToInt = 40000000, unsigned cast = 40000000, signed cast = 40000000
Angle = 495.00000: xs_RoundToInt = 60000000, unsigned cast = 60000000, signed cast = 60000000
Angle = 540.00000: xs_RoundToInt = 80000000, unsigned cast = 80000000, signed cast = 80000000
Angle = 585.00000: xs_RoundToInt = a0000000, unsigned cast = a0000000, signed cast = a0000000
Angle = 630.00000: xs_RoundToInt = c0000000, unsigned cast = c0000000, signed cast = c0000000
Angle = 675.00000: xs_RoundToInt = e0000000, unsigned cast = e0000000, signed cast = e0000000
Angle = 720.00000: xs_RoundToInt = 00000000, unsigned cast = 00000000, signed cast = 00000000
Angle = 765.00000: xs_RoundToInt = 20000000, unsigned cast = 20000000, signed cast = 20000000
Angle = 810.00000: xs_RoundToInt = 40000000, unsigned cast = 40000000, signed cast = 40000000
Angle = 855.00000: xs_RoundToInt = 60000000, unsigned cast = 60000000, signed cast = 60000000
- Graf Zahl
- Lead GZDoom+Raze Developer
- Posts: 49073
- Joined: Sat Jul 19, 2003 10:19 am
- Location: Germany
Re: [script]Pitch broken for REAL now.
Yeah, not that this surprises me the least.
But since the xs_RoundToInt method is the only one that's hardware independent I think it's the safest bet here.
All it does is multiply the float with a magic constant, stores the value in a union that maps to two ints and then reads out the lower part. There's no dependency on some weird conversion instruction that gets implemented differently by different hardware.
It only depends on how doubles are stored in memory, and all common architectures are still using the same IEEE format. (And to calm the paranoid ones - it can be switched off and revert to the compiler's implementation, should something not work on some odd system.
But since the xs_RoundToInt method is the only one that's hardware independent I think it's the safest bet here.
All it does is multiply the float with a magic constant, stores the value in a union that maps to two ints and then reads out the lower part. There's no dependency on some weird conversion instruction that gets implemented differently by different hardware.
It only depends on how doubles are stored in memory, and all common architectures are still using the same IEEE format. (And to calm the paranoid ones - it can be switched off and revert to the compiler's implementation, should something not work on some odd system.
- Graf Zahl
- Lead GZDoom+Raze Developer
- Posts: 49073
- Joined: Sat Jul 19, 2003 10:19 am
- Location: Germany
Re: [script]Pitch broken for REAL now.
I hope, that this is plugged for good now.
I had a look over the entire code and replaced all angle conversions I found with the abovelisted macros.
But seeing that nearly every place did it differently, there may still be hiding some deep in the bowels of the code. The ones in vmexec.h I only found by random chance because they didn't match any pattern I was looking for.
And it should be clear for the future: Use those macros! If they create a problem on some odd system, it's only one place that needs to be addressed, not fifty different variations on the same theme.
I had a look over the entire code and replaced all angle conversions I found with the abovelisted macros.
But seeing that nearly every place did it differently, there may still be hiding some deep in the bowels of the code. The ones in vmexec.h I only found by random chance because they didn't match any pattern I was looking for.
And it should be clear for the future: Use those macros! If they create a problem on some odd system, it's only one place that needs to be addressed, not fifty different variations on the same theme.
-
- Posts: 1774
- Joined: Sat Oct 17, 2009 9:40 am
Re: [script]Pitch broken for REAL now.
Do we need the angleconvtest CCMD at all?
Re: [script]Pitch broken for REAL now.
Wouldn't hurt to keep it around for platform testing.