negative uints about quakespasm HOT 19 OPEN

Do you have a patch?

CC: @andrei-drexler, @ericwa

from quakespasm.

I'll add a guard against outputting those <3 vert faces in ericw-tools in the future..

Not sure where the best place to patch it in QS is though. BuildSurfaceDisplayList maybe, and set the msurface_t::polys member to null for these degenerate faces, plus check the codebase that all msurface_t::polys accesses can handle null?

from quakespasm.

Would it not be OK to simply Sys_Error with any such bad maps in Mod_LoadEdges ??

[bad patch deleted]

Also: Can you give links to such bad maps please?

from quakespasm.

It looks like that patch is checking the wrong thing - that's checking total edge count in the .bsp being < 3. This issue is about otherwise well-formed maps that have one degenerate face (0, 1, or 2 edges just for a specific bad face.)

I've fixed this in ericw-tools a while ago so hopefully it won't be a problem on many .bsp's.

from quakespasm.

OK, should I close this then? Or, do you have a better patch?

from quakespasm.

.. and the correct check should be something like this I guess ?

diff --git a/Quake/gl_model.c b/Quake/gl_model.c
index 6e5cfd7..52bad53 100644
--- a/Quake/gl_model.c
+++ b/Quake/gl_model.c
@@ -1281,6 +1281,9 @@ static void Mod_LoadFaces (lump_t *l, qboolean bsp2)
 			ins++;
 		}
 
+		if (out->numedges < 3)
+			Sys_Error ("surfnum %d: bad numedges %d", surfnum, out->numedges);
+
 		out->flags = 0;
 
 		if (side)

from quakespasm.

OK, I just tried several (surely not all) mods with the following:

diff --git a/Quake/gl_model.c b/Quake/gl_model.c
index 6e5cfd7..70b1689 100644
--- a/Quake/gl_model.c
+++ b/Quake/gl_model.c
@@ -1282,6 +1282,8 @@ static void Mod_LoadFaces (lump_t *l, qboolean bsp2)
 		}
 
 		out->flags = 0;
+		if (out->numedges < 3)
+			Con_Warning("surfnum %d: bad numedges %d\n", surfnum, out->numedges);
 
 		if (side)
 			out->flags |= SURF_PLANEBACK;

It resulted in the following:

• Alkaline 1.2: Only one map:
  alk_dancing:
  surfnum 54053: bad numedges 2

• Explore jam3: Only one map:
  ej3_bizz:
  surfnum 5688: bad numedges 2
  surfnum 10190: bad numedges 2
  surfnum 10191: bad numedges 2
  surfnum 10192: bad numedges 2

• Rotting jam: Only one map:
  rotj_entsoy: surfnum 20737: bad numedges 2

• 'mjolnir' mod: Multiple maps:
  start2: surfnum 70379: bad numedges 2
  mj4m1: surfnum 179825: bad numedges 2
  mj4m2: surfnum 82355: bad numedges 2
  mj4m3: surfnum 69430: bad numedges 2
  mj4m4: surfnum 156826: bad numedges 2
  mj4m6: surfnum 14105: bad numedges 2

Then applied the following to r_world.c:

diff --git a/Quake/r_world.c b/Quake/r_world.c
index 5cc9667..1cab21b 100644
--- a/Quake/r_world.c
+++ b/Quake/r_world.c
@@ -319,7 +319,7 @@
 //
 //==============================================================================
 
-static unsigned int R_NumTriangleIndicesForSurf (msurface_t *s)
+static int R_NumTriangleIndicesForSurf (msurface_t *s)
 {
 	return 3 * (s->numedges - 2);
 }
@@ -384,9 +384,12 @@ using VBOs.
 */
 static void R_BatchSurface (msurface_t *s)
 {
-	int num_surf_indices;
+	int num_surf_indices = R_NumTriangleIndicesForSurf (s);
 
-	num_surf_indices = R_NumTriangleIndicesForSurf (s);
+	if (num_surf_indices < 0) {
+		Con_Warning("bad numedges for surface\n");
+		return;
+	}
 
 	if (num_vbo_indices + num_surf_indices > MAX_BATCH_SIZE)
 		R_FlushBatch();

... and noclip'ed around in alk_dancing and ej3_bizz, hoping to see what
would happen and whether I hit that warning message: nothing did..

I want to apply the first warning patch to Mod_LoadFaces so the issue
won't go unnoticed.

However, I don't know whether on not the change to r_world.c is correct
(minus the warning). @ericwa: What do you think?

from quakespasm.