R_DrawSprite: Enhancement to stop Things from disappearing due to "anchor" sector not being in line of sight

[SirBofu]

There is an issue across source ports where sprites will abruptly disappear if the "center" point of a Thing is located on a sector that isn't currently visible to the player, even if the vast majority of the Thing would be visible to the player. This behavior is not unique to Woof, but improving the function here will likely enable it to be improved across all related sourceports.

I have attached a demonstration WAD here. There are three mancubi facing away from the player, each with a small column between them and the player. From left to right, I will call these Mancubus 1, Mancubus 2, and Mancubus 3. Mancubus 1 is in the same sector as the player; Mancubus 2 is in a small child sector; and Mancubus 3 is in an even smaller child sector. By strafing left and right and approaching the columns, we can observe different behaviors:

1. Mancubus 1 will always be shown without issue, regardless of the player's position behind the pillars.
2. Mancubus 2 will generally always be visible when the player views them from a distance, but if the player moves close enough to the pillar behind it, the Mancubus will abruptly disappear.
3. Mancubus 3 will very easily pop in and pop out as the player strafes left and right, even when the player is not close to the pillar behind it.

While I'm not sure what form an improvement would take from a technical standpoint, in general, it seems like we could try to add some additional checks before determining that a sprite doesn't need to be drawn: in addition to checking the center position of the Thing, perhaps we should also check for visibility based on one or more of the Thing's four corners if the center is not visible. While this would not catch all cases, it would certainly result in much less pop-in and pop-out, and thus be a net improvement.

DrawTest.zip

[fabiangreffrath]

This might not be as easy as it seems. Actually, during a BSP traversal, all visible subsectors are processed [1], and in each subsector the sprites in the corresponding sector are rendered [2]. So, the renderer doesn't even care which sprites should be visible due to their sheer size. It checks which sectors are (partly) visible and renders the sprites of the map things only in these sectors.

[1] https://github.com/fabiangreffrath/woof/blob/029045e03472684f28287b2193bfae0aa36ae21d/src/r_bsp.c#L686 [2] https://github.com/fabiangreffrath/woof/blob/029045e03472684f28287b2193bfae0aa36ae21d/src/r_bsp.c#L649

[SirBofu]

That makes sense. I wonder if it would be feasible to also check if the player has visibility of a sector that neighbors the sector the Thing is in as a second "check"? It wouldn't catch every case, but it would certainly make for a good start. It would probably be experimenal at first, though.

[SirBofu]

I was able to piece something together that I think could work for this, but the code could probably use some cleaning. As it turns out, you can accomplish this all without even touching the renderer.

in DSDA, I got it working by changing R_AddSprites in r_things.c to this:

//
// R_AddSprites
// During BSP traversal, this adds sprites by sector.
//
// killough 9/18/98: add lightlevel as parameter, fixing underwater lighting
void R_AddSprites(subsector_t* subsec, int lightlevel)
{
  sector_t* sec=subsec->sector;
  mobj_t *thing;
  mobj_t *neighborthing;

  if (compatibility_level <= boom_202_compatibility)
    lightlevel = sec->lightlevel;

  // Handle all things in sector.

  if (dsda_ShowAliveMonsters())
  {
    if (dsda_ShowAliveMonsters() == 1)
    {
      for (thing = sec->thinglist; thing; thing = thing->snext)
      {
        if (!ALIVE(thing))
          R_ProjectSprite(thing, lightlevel);
      }
      if (sec->touching_thinglist)
      {
        for (neighborthing = sec->touching_thinglist->m_thing; neighborthing; neighborthing = neighborthing->snext)
        {
          if (!ALIVE(neighborthing))
          {
            if (!(neighborthing->touching_sectorlist->visited) && neighborthing->subsector->sector != sec)
              R_ProjectSprite(neighborthing, neighborthing->subsector->sector->lightlevel);
          }
        }
      }
    }
  }
  else
  {
    for (thing = sec->thinglist; thing; thing = thing->snext)
    {
      R_ProjectSprite(thing, lightlevel);
    }
    if (sec->touching_thinglist)
    {
      for (neighborthing = sec->touching_thinglist->m_thing; neighborthing; neighborthing = neighborthing->snext)
      {
        if (!(neighborthing->touching_sectorlist->visited) && neighborthing->subsector->sector != sec)
          R_ProjectSprite(neighborthing, neighborthing->subsector->sector->lightlevel);
      }
    }
  }
}

The challenge here is that Woof doesn't have as clean a way of forcing a different light level for a thing (because, using this method, a thing is getting drawn outside of the sector it actually belongs to). This code in my local branch of DSDA accounts for that by grabbing the light level of the sector the neighborthing is actually in.

I've only done some basic testing with this so far, but when played with the test map I have attached here, I'm able to consistently see all three Mancubi no matter my position, and they're able to move freely from sector to sector without issue. But there may need to be additional safeguards added to prevent a sprite from getting drawn multiple times or something.

[fabiangreffrath]

Wow, I wasn't even aware of the meaning of sec->touching_thinglist until now! Thanks for the concept/patch! Would you like to file a PR (draft, probably), so we can discuss this further with some actual code at our hands?

[fabiangreffrath]

Never mind, got a working patch:

diff --git a/src/r_things.c b/src/r_things.c
index bc363392..b8a53989 100644
--- a/src/r_things.c
+++ b/src/r_things.c
@@ -737,6 +737,19 @@ void R_AddSprites(sector_t* sec, int lightlevel)
 
   for (thing = sec->thinglist; thing; thing = thing->snext)
     R_ProjectSprite(thing);
+
+  if (sec->touching_thinglist)
+  {
+    msecnode_t *node;
+
+    for (node = sec->touching_thinglist; node; node = node->m_snext)
+    {
+      thing = node->m_thing;
+
+      if (!thing->touching_sectorlist->visited && thing->subsector->sector != sec)
+        R_ProjectSprite(thing);
+    }
+  }
 }
 
 //

We could move the spritelights calculation from R_AddSprites() to R_ProjectSprite() and that'd be it.

[fabiangreffrath]

I have to admit that I'm still a bit unsure about the thing->touching_sectorlist->visited part, though. Could you elaborate a bit @SirBofu ?

[SirBofu]

That visited part isn't necessary (and in fact doesn't do anything). I'm still ironing out things on my end, but right now, it looks like things can get rendered multiple times, or at least to trigger multiple times (no visual issue as far as I can tell, but I bet it's be a performance hit if you have lots of sprites displayed and they overlap multiple visible sectors). I was going to try adding a Boolean, but the issue is that this is a call that gets made multiple times, so I was having issues getting the Boolean to unset itself once the next frame has been written.

I think we might need some sort of global hashmap of things that have gotten rendered and check for a lack of membership in that group before R_ProjectSprites is called, then flush the group of all things at the start of each frame.

[fabiangreffrath]

Naive approach that tracks projected mobj sprites:

diff --git a/src/r_things.c b/src/r_things.c
index bc363392..17d50b33 100644
--- a/src/r_things.c
+++ b/src/r_things.c
@@ -331,10 +331,16 @@ void R_InitSprites(char **namelist)
 // Called at frame start.
 //
 
+#define M_ARRAY_INIT_CAPACITY 128
+#include "m_array.h"
+
+static mobj_t **projected = NULL;
+
 void R_ClearSprites (void)
 {
   rendered_vissprites = num_vissprite;
   num_vissprite = 0;            // killough
+  array_clear(projected);
 }
 
 //
@@ -485,7 +491,7 @@ void R_DrawVisSprite(vissprite_t *vis, int x1, int x2)
 
 boolean flipcorpses = false;
 
-void R_ProjectSprite (mobj_t* thing)
+static void R_ProjectSprite (mobj_t* thing)
 {
   fixed_t   gzt;               // killough 3/27/98
   fixed_t   tx, txc;
@@ -504,6 +510,16 @@ void R_ProjectSprite (mobj_t* thing)
   fixed_t tr_x, tr_y, gxt, gyt, tz;
   fixed_t interpx, interpy, interpz, interpangle;
 
+  for (int i = 0; i < array_size(projected); i++)
+  {
+    if (projected[i] == thing)
+    {
+      return;
+    }
+  }
+
+  array_push(projected, thing);
+
   // andrewj: voxel support
   if (VX_ProjectVoxel (thing))
       return;
@@ -737,6 +753,9 @@ void R_AddSprites(sector_t* sec, int lightlevel)
 
   for (thing = sec->thinglist; thing; thing = thing->snext)
     R_ProjectSprite(thing);
+
+  for (msecnode_t *n = sec->touching_thinglist; n; n = n->m_snext)
+    R_ProjectSprite(n->m_thing);
 }
 
 //

[SirBofu]

I'll do some testing with this implementation over my vacation the next several days. It definitely has promise.

[fabiangreffrath]

It's a performance disaster, that's for sure. 😉

[SirBofu]

It's also very possible that the performance drain of storing each thing (or even a reference to each thing) far outweighs the performance hit from possibly projecting the same sprite multiple times. I'll need to find some sufficiently complicated wads with high sprite counts to test with.

[fabiangreffrath]

Indeed, I guess maintaining the list of projected mobj sprites consumed the most performance. If you need a benchmark, try the city view at the start of the first map of comatose.wad. With this patch it drops from ~25 fps down to 2 fps on my system.

[rfomin]

Indeed, I guess maintaining the list of projected mobj sprites consumed the most performance.

I think it's a linear search in a projected array. We can come up with a better data structure, but we already have BSP. There has to be a better way.

[fabiangreffrath]

Hm, maybe like this:

diff --git a/src/p_mobj.h b/src/p_mobj.h
index 3538c71..1348eee 100644
--- a/src/p_mobj.h
+++ b/src/p_mobj.h
@@ -385,6 +385,8 @@ typedef struct mobj_s

     // [FG] height of the sprite in pixels
     int actualheight;
+
+    unsigned int render_iter;
 } mobj_t;

 // External declarations (fomerly in p_local.h) -- killough 5/2/98
diff --git a/src/r_things.c b/src/r_things.c
index bc36339..0a7141d 100644
--- a/src/r_things.c
+++ b/src/r_things.c
@@ -331,10 +331,13 @@ void R_InitSprites(char **namelist)
 // Called at frame start.
 //

+static unsigned int render_iter;
+
 void R_ClearSprites (void)
 {
   rendered_vissprites = num_vissprite;
   num_vissprite = 0;            // killough
+  render_iter++;
 }

 //
@@ -485,7 +488,7 @@ void R_DrawVisSprite(vissprite_t *vis, int x1, int x2)

 boolean flipcorpses = false;

-void R_ProjectSprite (mobj_t* thing)
+static void R_ProjectSprite (mobj_t* thing)
 {
   fixed_t   gzt;               // killough 3/27/98
   fixed_t   tx, txc;
@@ -504,6 +507,13 @@ void R_ProjectSprite (mobj_t* thing)
   fixed_t tr_x, tr_y, gxt, gyt, tz;
   fixed_t interpx, interpy, interpz, interpangle;

+  if (thing->render_iter == render_iter)
+  {
+    return;
+  }
+
+  thing->render_iter = render_iter;
+
   // andrewj: voxel support
   if (VX_ProjectVoxel (thing))
       return;
@@ -737,6 +747,9 @@ void R_AddSprites(sector_t* sec, int lightlevel)

   for (thing = sec->thinglist; thing; thing = thing->snext)
     R_ProjectSprite(thing);
+
+  for (msecnode_t *n = sec->touching_thinglist; n; n = n->m_snext)
+    R_ProjectSprite(n->m_thing);
 }

 //

[fabiangreffrath]

Seems to work fine, and no performance penalty on comatose.wad MAP01.

[fabiangreffrath]

Lighting is definitely still an issue. I'd like to have this in a separate loop, so that all "regular" vissprites are processed first, and then all additional sprites if they haven't been rendered yet.