FP precision crushing the player on buttons

[ericw]

We had a report that, on 64-bit builds of Quakespasm, players were getting crushed when pressing angled buttons in some custom maps/mods: *
- Travail's qte1m2.bsp: noclip to 2788 732 -584, and press the button. The player will get crushed and killed on affected engines.
- sm179_otp.bsp: pressing the left button causes the player to get stuck.

The above happens on 64-bit QS builds (Windows x64, macOS builds) but also Fitzquake 0.85 compiled with VS2017. It'll probably happen on anything but gcc/msvc building for x86 without SSE.

I've posted about 80-bit x87 vs C standard compliant FP precision behaviour differences before (e.g. lightmaps:
viewtopic.php?f=12&t=5516&p=53977&hilit=dot+product#p53977
) so I knew what to expect, it was going to be some FP sensitive code (probably a dot product) that worked fine with 80-bit temporaries but "broke" with 32-bit rounded temporaries.

The culprit - whether the button worked normally or crushed the player - turned out to be this bit of code in SUB_CalcMove (subs.qc):

Code: Select all

// calculate length of vector
        len = vlen (vdestdelta);

// divide by speed to get time to reach dest
        traveltime = len / tspeed;

        if (traveltime < 0.1)
        {


traveltime would be very near 0.1, and the "if" branch would either be taken or not causing the crushing or not.

The source of the FP precision difference based on the compiler settings was PF_vlen:

Code: Select all

void PF_vlen (void)
{
   float   *value1;
   float   new;
   
   value1 = G_VECTOR(OFS_PARM0);

   new = value1[0] * value1[0] + value1[1] * value1[1] + value1[2]*value1[2];
   new = sqrt(new);
   
   G_FLOAT(OFS_RETURN) = new;
}


The fix was:

Code: Select all

-   float   new_temp;
+   double   new_temp;
 
    value1 = G_VECTOR(OFS_PARM0);
 
-   new_temp = value1[0] * value1[0] + value1[1] * value1[1] + value1[2]*value1[2];
+   new_temp = (double)value1[0] * value1[0] + (double)value1[1] * value1[1] + (double)value1[2]*value1[2];


After tracking down and "fixing" enough instances of the same bug in response to reports from users, I'm thinking all math temporaries where the precision could matter should just have casts added preemptively.

[*] QS issue / commit links:
https://sourceforge.net/p/quakespasm/code/1554/
https://sourceforge.net/p/quakespasm/bugs/26/

[Baker]

That's a great bug fix.