2 Obscure WinQuake fixes

[Baker]

2 fixes for WinQuake. I left the start demos running and they looped and looped and looped.

And after a while, access violations would happen. Often took 15 minutes. I'm sure they eventually happen during normal use and WinQuake just crashes.

#1 is C code only
#2 can happen in both C code and asm.

#1 This is for the C code (if you are using asm, this doesn't apply):

Code: Select all

/*
================
D_PolysetDrawFinalVerts
================
*/
void D_PolysetDrawFinalVerts (finalvert_t *fv, int numverts)
{
   int      i, z;
   short   *zbuf;

   for (i=0 ; i<numverts ; i++, fv++)
   {
   // valid triangle coordinates for filling can include the bottom and
   // right clip edges, due to the fill rule; these shouldn't be drawn
      if ((fv->v[0] < r_refdef.vrectright) &&
         (fv->v[1] < r_refdef.vrectbottom) )
      {
#if 1 // Baker - in very rare occassions, these may have negative values and can result in a memory access violation
         if (fv->v[0] >=0 && fv->v[1] >=0)
         {
#endif
            z = fv->v[5]>>16;
            zbuf = zspantable[fv->v[1]] + fv->v[0];
            if (z >= *zbuf)
            {
               int      pix;
               
               *zbuf = z;
               pix = skintable[fv->v[3]>>16][fv->v[2]>>16];
               pix = ((byte *)acolormap)[pix + (fv->v[4] & 0xFF00) ];
               d_viewbuffer[d_scantable[fv->v[1]] + fv->v[0]] = pix;
            }
#if 1 // Baker
         }
#endif
      }
   }
}

#2  This one can happen using asm or C

[code]/*
================
D_RasterizeAliasPolySmooth
================
*/
void D_RasterizeAliasPolySmooth (void)
{
   int            initialleftheight, initialrightheight;
   int            *plefttop, *prighttop, *pleftbottom, *prightbottom;
   int            working_lstepx, originalcount;

   plefttop = pedgetable->pleftedgevert0;
   prighttop = pedgetable->prightedgevert0;

   pleftbottom = pedgetable->pleftedgevert1;
   prightbottom = pedgetable->prightedgevert1;

   initialleftheight = pleftbottom[1] - plefttop[1];
   initialrightheight = prightbottom[1] - prighttop[1];

//
// set the s, t, and light gradients, which are consistent across the triangle
// because being a triangle, things are affine
//
   D_PolysetCalcGradients (r_affinetridesc.skinwidth);

//
// rasterize the polygon
//

//
// scan out the top (and possibly only) part of the left edge
//
   d_pedgespanpackage = a_spans;

   ystart = plefttop[1];
   d_aspancount = plefttop[0] - prighttop[0];

   d_ptex = (byte *)r_affinetridesc.pskin + (plefttop[2] >> 16) +
         (plefttop[3] >> 16) * r_affinetridesc.skinwidth;
#if   id386
   d_sfrac = (plefttop[2] & 0xFFFF) << 16;
   d_tfrac = (plefttop[3] & 0xFFFF) << 16;
#else
   d_sfrac = plefttop[2] & 0xFFFF;
   d_tfrac = plefttop[3] & 0xFFFF;
#endif
   d_light = plefttop[4];
   d_zi = plefttop[5];

   d_pdest = (byte *)d_viewbuffer +
         ystart * screenwidth + plefttop[0];
   d_pz = d_pzbuffer + ystart * d_zwidth + plefttop[0];

   if (initialleftheight == 1)
   {
      d_pedgespanpackage->pdest = d_pdest;
      d_pedgespanpackage->pz = d_pz;
      d_pedgespanpackage->count = d_aspancount;
      d_pedgespanpackage->ptex = d_ptex;

      d_pedgespanpackage->sfrac = d_sfrac;
      d_pedgespanpackage->tfrac = d_tfrac;

   // FIXME: need to clamp l, s, t, at both ends?
      d_pedgespanpackage->light = d_light;
      d_pedgespanpackage->zi = d_zi;

      d_pedgespanpackage++;
   }
   else
   {
      D_PolysetSetUpForLineScan(plefttop[0], plefttop[1],
                       pleftbottom[0], pleftbottom[1]);

   #if   id386
      d_pzbasestep = (d_zwidth + ubasestep) << 1;
      d_pzextrastep = d_pzbasestep + 2;
   #else
      d_pzbasestep = d_zwidth + ubasestep;
      d_pzextrastep = d_pzbasestep + 1;
   #endif

      d_pdestbasestep = screenwidth + ubasestep;
      d_pdestextrastep = d_pdestbasestep + 1;

   // TODO: can reuse partial expressions here

   // for negative steps in x along left edge, bias toward overflow rather than
   // underflow (sort of turning the floor () we did in the gradient calcs into
   // ceil (), but plus a little bit)
      if (ubasestep < 0)
         working_lstepx = r_lstepx - 1;
      else
         working_lstepx = r_lstepx;

      d_countextrastep = ubasestep + 1;
      d_ptexbasestep = ((r_sstepy + r_sstepx * ubasestep) >> 16) +
            ((r_tstepy + r_tstepx * ubasestep) >> 16) *
            r_affinetridesc.skinwidth;
   #if   id386
      d_sfracbasestep = (r_sstepy + r_sstepx * ubasestep) << 16;
      d_tfracbasestep = (r_tstepy + r_tstepx * ubasestep) << 16;
   #else
      d_sfracbasestep = (r_sstepy + r_sstepx * ubasestep) & 0xFFFF;
      d_tfracbasestep = (r_tstepy + r_tstepx * ubasestep) & 0xFFFF;
   #endif
      d_lightbasestep = r_lstepy + working_lstepx * ubasestep;
      d_zibasestep = r_zistepy + r_zistepx * ubasestep;

      d_ptexextrastep = ((r_sstepy + r_sstepx * d_countextrastep) >> 16) +
            ((r_tstepy + r_tstepx * d_countextrastep) >> 16) *
            r_affinetridesc.skinwidth;
   #if   id386
      d_sfracextrastep = (r_sstepy + r_sstepx*d_countextrastep) << 16;
      d_tfracextrastep = (r_tstepy + r_tstepx*d_countextrastep) << 16;
   #else
      d_sfracextrastep = (r_sstepy + r_sstepx*d_countextrastep) & 0xFFFF;
      d_tfracextrastep = (r_tstepy + r_tstepx*d_countextrastep) & 0xFFFF;
   #endif
      d_lightextrastep = d_lightbasestep + working_lstepx;
      d_ziextrastep = d_zibasestep + r_zistepx;

      D_PolysetScanLeftEdge (initialleftheight);
   }

//
// scan out the bottom part of the left edge, if it exists
//
   if (pedgetable->numleftedges == 2)
   {
      int      height;

      plefttop = pleftbottom;
      pleftbottom = pedgetable->pleftedgevert2;

      height = pleftbottom[1] - plefttop[1];

// TODO: make this a function; modularize this function in general

      ystart = plefttop[1];
      d_aspancount = plefttop[0] - prighttop[0];
      d_ptex = (byte *)r_affinetridesc.pskin + (plefttop[2] >> 16) +
            (plefttop[3] >> 16) * r_affinetridesc.skinwidth;
      d_sfrac = 0;
      d_tfrac = 0;
      d_light = plefttop[4];
      d_zi = plefttop[5];

      d_pdest = (byte *)d_viewbuffer + ystart * screenwidth + plefttop[0];
      d_pz = d_pzbuffer + ystart * d_zwidth + plefttop[0];

      if (height == 1)
      {
         d_pedgespanpackage->pdest = d_pdest;
         d_pedgespanpackage->pz = d_pz;
         d_pedgespanpackage->count = d_aspancount;
         d_pedgespanpackage->ptex = d_ptex;

         d_pedgespanpackage->sfrac = d_sfrac;
         d_pedgespanpackage->tfrac = d_tfrac;

      // FIXME: need to clamp l, s, t, at both ends?
         d_pedgespanpackage->light = d_light;
         d_pedgespanpackage->zi = d_zi;

         d_pedgespanpackage++;
      }
      else
      {
         D_PolysetSetUpForLineScan(plefttop[0], plefttop[1],
                          pleftbottom[0], pleftbottom[1]);

         d_pdestbasestep = screenwidth + ubasestep;
         d_pdestextrastep = d_pdestbasestep + 1;

   #if   id386
         d_pzbasestep = (d_zwidth + ubasestep) << 1;
         d_pzextrastep = d_pzbasestep + 2;
   #else
         d_pzbasestep = d_zwidth + ubasestep;
         d_pzextrastep = d_pzbasestep + 1;
   #endif

         if (ubasestep < 0)
            working_lstepx = r_lstepx - 1;
         else
            working_lstepx = r_lstepx;

         d_countextrastep = ubasestep + 1;
         d_ptexbasestep = ((r_sstepy + r_sstepx * ubasestep) >> 16) +
               ((r_tstepy + r_tstepx * ubasestep) >> 16) *
               r_affinetridesc.skinwidth;
   #if   id386
         d_sfracbasestep = (r_sstepy + r_sstepx * ubasestep) << 16;
         d_tfracbasestep = (r_tstepy + r_tstepx * ubasestep) << 16;
   #else
         d_sfracbasestep = (r_sstepy + r_sstepx * ubasestep) & 0xFFFF;
         d_tfracbasestep = (r_tstepy + r_tstepx * ubasestep) & 0xFFFF;
   #endif
         d_lightbasestep = r_lstepy + working_lstepx * ubasestep;
         d_zibasestep = r_zistepy + r_zistepx * ubasestep;

         d_ptexextrastep = ((r_sstepy + r_sstepx * d_countextrastep) >> 16) +
               ((r_tstepy + r_tstepx * d_countextrastep) >> 16) *
               r_affinetridesc.skinwidth;
   #if   id386
         d_sfracextrastep = ((r_sstepy+r_sstepx*d_countextrastep) & 0xFFFF)<<16;
         d_tfracextrastep = ((r_tstepy+r_tstepx*d_countextrastep) & 0xFFFF)<<16;
   #else
         d_sfracextrastep = (r_sstepy+r_sstepx*d_countextrastep) & 0xFFFF;
         d_tfracextrastep = (r_tstepy+r_tstepx*d_countextrastep) & 0xFFFF;
   #endif
         d_lightextrastep = d_lightbasestep + working_lstepx;
         d_ziextrastep = d_zibasestep + r_zistepx;

         D_PolysetScanLeftEdge (height);
      }
   }

// scan out the top (and possibly only) part of the right edge, updating the
// count field
   d_pedgespanpackage = a_spans;

   D_PolysetSetUpForLineScan(prighttop[0], prighttop[1],
                    prightbottom[0], prightbottom[1]);
   d_aspancount = 0;
   d_countextrastep = ubasestep + 1;
   originalcount = a_spans[initialrightheight].count;
   a_spans[initialrightheight].count = -999999; // mark end of the spanpackages
   D_PolysetDrawSpans8 (a_spans);

// scan out the bottom part of the right edge, if it exists
   if (pedgetable->numrightedges == 2)
   {
      int            height;
      spanpackage_t   *pstart;

      pstart = a_spans + initialrightheight;
      pstart->count = originalcount;

#if 1 // Baker: In very rare situations, the count can be zero which will result in a memory access violation
      if (pstart->count) // Baker
      {
#endif
         d_aspancount = prightbottom[0] - prighttop[0];

         prighttop = prightbottom;
         prightbottom = pedgetable->prightedgevert2;

         height = prightbottom[1] - prighttop[1];

         D_PolysetSetUpForLineScan(prighttop[0], prighttop[1],
                          prightbottom[0], prightbottom[1]);

         d_countextrastep = ubasestep + 1;
         a_spans[initialrightheight + height].count = -999999;
                                    // mark end of the spanpackages
         D_PolysetDrawSpans8 (pstart);
#if 1 // Baker
      }
#endif
   }
}

[mankrip]

Holy f* damn. Thanks.

I'll try to get my old laptop working again to test this. This sounds exactly like the Bug I Could Never Solve™ in Makaqu.

[Baker]

Holy f* damn. Thanks.

Fix #2 needs slightly more tuning, it will avoid the crash but it is a tad overkill. I need to tune #2 it to only avoid the memory violation.

[mankrip]

After looking in my code, I remember the bug #1. It only happens when frame interpolation is implemented, so that's why it doesn't crash vanilla WinQuake.
I've worked around it by removing the code for non-clipped triangles.

#2 is more like a tradeoff. The engine stops crashing, at the cost of numerous triangles going missing.
I've looked into it for a while and couldn't figure out what may be the exact reason for the access violations yet, but given that it's near the end of the drawing, it would most likely go away if I rewrite the MDL triangle drawing code to implement perspective correction.

I'll do more tests to see if there's anything else crashing my current code in comparison to Makaqu 1.3.1.

[Baker]

I'm going to do some more work catching situation #2 again and digging around to see what the errand situation is.

I'm rather confident that I can improve (or possibly perfect) fix #2. I'll see what I can come up with.

[szo]

Where does the crash happen, what backtrace does gdb give?

Is the only way to reproduce the crash is letting the demo loop to run a long long time?

(I'dl like to know whether hexen2 has the issue, and if it does I'd like to fix it there.)

[Baker]

I've found maybe 4 different crash points One for sprites (standard sprite draw, tries to write at row -1 on rare occasions) Possibly 2 that relate to MH's interpolation animation (write at row -1, haven't figured out root issue with the second one but I can stop it). One other one that I can't recall.

One effective method to neutralize these (whether or not fixed, but by doing so the ability to easily catch them) is marking the start and the end of the surface cache and check writes against that range. I'm not getting slow-downs by doing this, probably because the C codes optimizes well.

What I have done to trap these guys is run demos in higher resolutions using the C code and give it low cpu priority (results in larger host_frametime amounts). Some of them rear up quickly (Quake demo2.dem can get issues real quick), but some of them will not necessarily cause crashes immediately despite trying to write out of range.

I'll document it better the next time I have the opportunity. Hitting the sprite one made very irritated after I had managed to fix/evade 3 circumstances, another one showed up

[mh]

If it's related to the frame interpolation code, then it's probably because I never fixed up the condition for doing a trivial_accept. The quick and dirty solution is to just never do a trivial_accept.

[Baker]

If it's related to the frame interpolation code, then it's probably because I never fixed up the condition for doing a trivial_accept. The quick and dirty solution is to just never do a trivial_accept.

I'll have to look into that.

I took your interpolation code and made it essentially identical to what I use with the GL rendererer (which happens to be able to interpolate in reverse).

Code: Select all

/*
=================
R_SetupAliasFrame
=================
*/
void R_SetupAliasFrame (aliashdr_t *paliashdr, int frame, lerpdata_t *lerpdata)
{
   entity_t      *e = currententity;
   int            posenum, numposes;

   if ((frame >= paliashdr->numframes) || (frame < 0))
   {
      Con_DPrintf ("R_AliasSetupFrame: no such frame %d\n", frame);
      frame = 0;
   }

   posenum = paliashdr->frames[frame].firstpose;
   numposes = paliashdr->frames[frame].numposes;

   if (numposes > 1)
   {
      e->lerptime = paliashdr->frames[frame].interval;
      posenum += (int)(cl.ctime / e->lerptime) % numposes;
   }
   else
      e->lerptime = 0.1;

   if (e->lerpflags & LERP_RESETANIM) //kill any lerp in progress
   {
      e->lerpstart = 0;
      e->previouspose = posenum;
      e->currentpose = posenum;
      e->lerpflags -= LERP_RESETANIM;
   }
   else if (e->currentpose != posenum) // pose changed, start new lerp
   {
      if (e->lerpflags & LERP_RESETANIM2) //defer lerping one more time
      {
         e->lerpstart = 0;
         e->previouspose = posenum;
         e->currentpose = posenum;
         e->lerpflags -= LERP_RESETANIM2;
#if 1 // Baker: Special exception :(
         if (cls.demoplayback && cls.demorewind)
            e->lerpflags -= LERP_RESETANIM;
#endif
      }
      else
      {
      e->lerpstart = cl.ctime;
         e->previouspose = e->currentpose;
         e->currentpose = posenum;
      }
   }

   //set up values
   if (r_lerpmodels.value && !((e->model->flags & MOD_NOLERP) && r_lerpmodels.value != 2))
   {
      if (e->lerpflags & LERP_FINISH && numposes == 1)
         lerpdata->blend = CLAMP (0, (cl.ctime - e->lerpstart) / (e->lerpfinish - e->lerpstart), 1);
      else
         lerpdata->blend = CLAMP (0, (cl.ctime - e->lerpstart) / e->lerptime, 1);
      lerpdata->pose1 = e->previouspose;
      lerpdata->pose2 = e->currentpose;
   }
   else //don't lerp
   {
      lerpdata->blend = 1;
      lerpdata->pose1 = posenum;
      lerpdata->pose2 = posenum;
   }
}

/*
=================
R_SetupEntityTransform
=================
*/
void R_SetupEntityTransform (entity_t *e)
{
   lerpdata_t _lerpdata;
   lerpdata_t *lerpdata = &_lerpdata;
   float blend;
   vec3_t d;
   int i;

   // if LERP_RESETMOVE, kill any lerps in progress
   if (e->lerpflags & LERP_RESETMOVE)
   {
      e->movelerpstart = 0;
      VectorCopy (e->origin, e->previousorigin);
      VectorCopy (e->origin, e->currentorigin);
      VectorCopy (e->angles, e->previousangles);
      VectorCopy (e->angles, e->currentangles);
      e->lerpflags -= LERP_RESETMOVE;
   }
   else if (!VectorCompare (e->origin, e->currentorigin) || !VectorCompare (e->angles, e->currentangles)) // origin/angles changed, start new lerp
   {
      e->movelerpstart = cl.ctime;
      VectorCopy (e->currentorigin, e->previousorigin);
      VectorCopy (e->origin,  e->currentorigin);
      VectorCopy (e->currentangles, e->previousangles);
      VectorCopy (e->angles,  e->currentangles);
   }

   //set up values
   if (r_lerpmove.value && e != &cl.viewent && e->lerpflags & LERP_MOVESTEP)
   {
      if (e->lerpflags & LERP_FINISH)
         blend = CLAMP (0, (cl.ctime - e->movelerpstart) / (e->lerpfinish - e->movelerpstart), 1);
      else
         blend = CLAMP (0, (cl.ctime - e->movelerpstart) / 0.1, 1);

      //translation
      VectorSubtract (e->currentorigin, e->previousorigin, d);
      lerpdata->origin[0] = e->previousorigin[0] + d[0] * blend;
      lerpdata->origin[1] = e->previousorigin[1] + d[1] * blend;
      lerpdata->origin[2] = e->previousorigin[2] + d[2] * blend;

      //rotation
      VectorSubtract (e->currentangles, e->previousangles, d);
      for (i = 0; i < 3; i++)
      {
         if (d[i] > 180)  d[i] -= 360;
         if (d[i] < -180) d[i] += 360;
      }
      lerpdata->angles[0] = e->previousangles[0] + d[0] * blend;
      lerpdata->angles[1] = e->previousangles[1] + d[1] * blend;
      lerpdata->angles[2] = e->previousangles[2] + d[2] * blend;
   }
   else //don't lerp
   {
      VectorCopy (e->origin, lerpdata->origin);
      VectorCopy (e->angles, lerpdata->angles);
   }
#if 1 // Baker: Software uses these fields in too many places
   VectorCopy (lerpdata->origin, e->origin);
   VectorCopy (lerpdata->angles, e->angles);
#endif
}



/*
=================
R_AliasSetupFrameMH

set currverts and lastverts and blend between the two frames
=================
*/
void R_AliasSetupFrameMH (entity_t *ent)
{
   maliasgroup_t   *paliasgroup;
   int            frame;
   lerpdata_t      lerpdata;
   float         blend;
   
   frame = ent->frame;

   if ((frame >= pmdl->numframes) || (frame < 0))
   {
      Con_DPrintf ("R_AliasSetupFrameMH: no such frame %d\n", frame);
      frame = 0;
   }
   
   paliasgroup = (maliasgroup_t *) ((byte *) paliashdr + paliashdr->frames[frame].frame);

   R_SetupAliasFrame (paliashdr, frame, &lerpdata);

   if (lerpdata.pose1 != lerpdata.pose2)
      blend = lerpdata.blend;
   else blend = 0;

   {
      trivertx_t      *currverts;
      trivertx_t      *lastverts;
      int            i;


      if (paliashdr->frames[frame].type == ALIAS_SINGLE)
      {
         R_BoundPoseSingle (ent, pmdl, &lerpdata);

         currverts = (trivertx_t *) ((byte *) paliashdr + paliashdr->frames[lerpdata.pose2].frame);
         lastverts = (trivertx_t *) ((byte *) paliashdr + paliashdr->frames[lerpdata.pose1].frame);
      }
      else
      {
         R_BoundPoseGroup (ent, paliasgroup, &lerpdata);

         currverts = (trivertx_t *) ((byte *) paliashdr + paliasgroup->frames[lerpdata.pose2].frame);
         lastverts = (trivertx_t *) ((byte *) paliashdr + paliasgroup->frames[lerpdata.pose1].frame);
      }

      lerpverts = r_aliaslerpverts;

      // perform the lerp
      for (i = 0; i < pmdl->numverts; i++, currverts++, lastverts++, lerpverts++)
      {
         lerpverts->v[0] = currverts->v[0] * blend + lastverts->v[0] * (1.0f - blend);
         lerpverts->v[1] = currverts->v[1] * blend + lastverts->v[1] * (1.0f - blend);
         lerpverts->v[2] = currverts->v[2] * blend + lastverts->v[2] * (1.0f - blend);

         lerpverts->currlightnormal = currverts->lightnormalindex;
         lerpverts->lastlightnormal = lastverts->lightnormalindex;
         lerpverts->blend = blend;
      }
   }
}

[mankrip]

I've found maybe 4 different crash points One for sprites (standard sprite draw, tries to write at row -1 on rare occasions) [...]

Hitting the sprite one made very irritated after I had managed to fix/evade 3 circumstances, another one showed up

IIRC there's a bug in the sprite loading code. A bad data offset somewhere in it. I remember fixing it when implementing the padding for the ordered dithering filter on sprites, although it didn't seem to cause any problem.

Leileilol also had pointed out that sprites may crash the engine when rendered too close to the camera.

[szo]

IIRC there's a bug in the sprite loading code. A bad data offset somewhere in it. I remember fixing it when implementing the padding for the ordered dithering filter on sprites, although it didn't seem to cause any problem.

Leileilol also had pointed out that sprites may crash the engine when rendered too close to the camera.

I did have the 'sprites rendered too close to the camera causing a crash'
in hexen2, and fixed it like this back in 2012:
http://sourceforge.net/p/uhexen2/code/4956/
http://sourceforge.net/tracker/?func=de ... _id=124987

[Baker]

That looks like the one. And it looks identical to what I did (i.e. if (ymin < 0 ) {Con_Print ("Some warning"); return };

http://sourceforge.net/p/uhexen2/code/4956/

But I had such a hard time getting to happen again, in fact I couldn't get it to happen again to examine it in more detail.

IIRC there's a bug in the sprite loading code.

That's interesting, I will have to check it out.

[leileilol]

I discovered the crash happening in Quake 1.01 (yes this crash goes way back, errormessage spewing something about surfacecache) after trying a mod that spammed bubbles exactly at the view, where swimming backwards would trigger a crash. I kept reproducing this crash, kept debugging and managed to fix it around 2012 as well (without any knowledge of szo's fix).

Also, much like Hexen II, the mod was also programmed by Mike Gummelt. XD so props to him for coding stuff that triggers obscure engine crashes easily in '96!

[Baker]

Yeah, I remember you pointing out this sprite bug a few times.

[mankrip]

I'll try to get my old laptop working again to test this. This sounds exactly like the Bug I Could Never Solve™ in Makaqu.

After several tests, it turns out that it wasn't. I'm still getting the same crash, both with and without the fix #2.