negative uints #93

Shpoike · 2024-01-16T09:31:35Z

https://github.com/sezero/quakespasm/blob/master/Quake/r_world.c#L324
there are bad bsps out there with only 1 or 2 edges per face (ericw-tools seems to generate them).
these degenerate faces can thus return -3 here...
R_BatchSurface then ends up overflowing the vbo_indices array with the next surface, or telling the glDrawElements to draw rather more primitives than was really intended. both situations are rather likely to result in crashes...

sezero · 2024-01-16T09:45:23Z

Do you have a patch?

CC: @andrei-drexler, @ericwa

ericwa · 2024-01-16T16:56:37Z

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?

sezero · 2024-06-06T15:17:01Z

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?

ericwa · 2024-06-06T17:04:11Z

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.

sezero · 2024-06-06T17:09:08Z

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

sezero · 2024-06-06T19:44:19Z

.. 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)

sezero · 2024-06-07T20:02:26Z

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?

sezero · 2024-06-08T09:35:01Z

... well, of course I couldn't hit the issue because I mistakenly
compared with < instead of <= : Fixed the patch above and can
easily hit the warning..

@ericwa -- what do you think?

ericwa · 2024-06-09T03:44:51Z

IMO the right thing is do a developer warning on first encountering this, but silence after that, and just ignore the faces.

sezero · 2024-06-09T10:20:51Z

IMO the right thing is do a developer warning on first encountering this, but silence after that, and just ignore the faces.

Isn't the patch above doing that already (inlined below again with warning in R_BatchSurface changed into a Con_DWarning), or am I missing something? If you want to Con_DWarning in R_BatchSurface only once, I really don't know how it's doable.

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;
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 @@ void R_DrawTextureChains_Glow (qmodel_t *model, entity_t *ent, texchain_t chain)
 //
 //==============================================================================
 
-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_DWarning("bad numedges for surface\n");
+		return;
+	}
 
 	if (num_vbo_indices + num_surf_indices > MAX_BATCH_SIZE)
 		R_FlushBatch();

Diordany · 2024-06-09T10:31:01Z

Isn't the patch above doing that already (inlined below again with warning in R_BatchSurface changed into a Con_DWarning), or am I missing something? If you want to Con_DWarning in R_BatchSurface only once, I really don't know how it's doable.

I think @ericwa probably means the thing that you present in this patch (the previous two combined), minus the Con_DWarning("bad numedges for surface\n"); line. From a map developer's perspective, I think that would make it clear which surfaces are to blame, while the engine silently ignores them in the engine loop.

sezero · 2024-06-09T10:41:28Z

I.e.: drop the warning at render time altogether?

Diordany · 2024-06-09T11:08:03Z

I.e.: drop the warning at render time altogether?

That's what I think it meant yes, but @ericwa would have to confirm that.

sezero · 2024-06-09T16:03:28Z

I.e.: drop the warning at render time altogether?

That's what I think it meant yes, but @ericwa would have to confirm that.

OK, here is what I have hopefully finalized

diff --git a/Quake/gl_model.c b/Quake/gl_model.c
index 6e5cfd7..cc12d27 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;
diff --git a/Quake/r_world.c b/Quake/r_world.c
index 5cc9667..22dd574 100644
--- a/Quake/r_world.c
+++ b/Quake/r_world.c
@@ -319,7 +319,7 @@ void R_DrawTextureChains_Glow (qmodel_t *model, entity_t *ent, texchain_t chain)
 //
 //==============================================================================
 
-static unsigned int R_NumTriangleIndicesForSurf (msurface_t *s)
+static int R_NumTriangleIndicesForSurf (msurface_t *s)
 {
 	return 3 * (s->numedges - 2);
 }
@@ -384,9 +384,13 @@ 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_DWarning ("bad numedges for surface\n");
+		return;
+	}
 
 	if (num_vbo_indices + num_surf_indices > MAX_BATCH_SIZE)
 		R_FlushBatch();

Diordany · 2024-06-10T00:51:48Z

Just an additional comment on the render time warning:

I tried your patch again (the one where the render time warning isn't commented out) and realized that I missed the significance of the change from Con_Warning to Con_DWarning. I had a similar thing in mind before, but didn't realize there was already a dedicated function for this. Either way, I'm not sure if you would want to use that in a tight loop.

sezero · 2024-06-10T01:18:32Z

Either way, I'm not sure if you would want to use that in a tight loop.

It is commented out, so no worries in there, no? (I had just wanted to test whether any badness was associated with the fix, now no need I guess.)

Diordany · 2024-06-10T01:43:03Z

It is commented out, so no worries in there, no? (I had just wanted to test whether any badness was associated with the fix, now no need I guess.)

Sure, no worries there. I just thought I'd mention that in case there was more to the render time warning.

ericwa · 2024-06-10T01:53:48Z

Just tested the last patch on rotj_entsoy.bsp.

I might suggest changing the Mod_LoadFaces warning to a Con_DWarning - thinking about it more, this was really just a bug specific to QS's batching code (which is fixed by adding the if (num_surf_indices <= 0) check). I doubt other engines will have an issue with these 0/1/2 sided faces, although I haven't checked.

Either Con_DWarning or Con_Warning is fine with me though, thanks for fixing the issue.

e.g.: mj4m?, alk_dancing, ej3_bizz, rotj_entsoy... See: #93 issue was reported by Spike.

sezero · 2024-06-10T02:22:47Z

Patch pushed as commit 7029526, kept the warning in Mod_LoadFaces as Con_Warning.

Thanks.

e.g.: mj4m?, alk_dancing, ej3_bizz, rotj_entsoy... See: sezero#93 issue was reported by Spike.

e.g.: mj4m?, alk_dancing, ej3_bizz, rotj_entsoy... See: sezero/quakespasm#93 issue was reported by Spike. (cherry picked from commit 7029526)

See sezero#93

sezero#93

sezero added a commit that referenced this issue Jun 10, 2024

Warn about bad bsps with only 1 or 2 edges per face

7029526

e.g.: mj4m?, alk_dancing, ej3_bizz, rotj_entsoy... See: #93 issue was reported by Spike.

sezero closed this as completed Jun 10, 2024

Diordany pushed a commit to Diordany/quakespasm that referenced this issue Jun 12, 2024

Warn about bad bsps with only 1 or 2 edges per face

c7d560d

e.g.: mj4m?, alk_dancing, ej3_bizz, rotj_entsoy... See: sezero#93 issue was reported by Spike.

andrei-drexler added a commit to andrei-drexler/ironwail that referenced this issue Jul 30, 2024

Avoid unsigned underflow for surfaces with < 2 edges

31efc23

See sezero#93

andrei-drexler added a commit to andrei-drexler/ironwail that referenced this issue Aug 3, 2024

Avoid issues due to bad numedges in GL_BuildBModelMarkBuffers

61789b1

sezero#93

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

negative uints #93

negative uints #93

Shpoike commented Jan 16, 2024

sezero commented Jan 16, 2024

ericwa commented Jan 16, 2024

sezero commented Jun 6, 2024 •

edited

Loading

ericwa commented Jun 6, 2024

sezero commented Jun 6, 2024

sezero commented Jun 6, 2024

sezero commented Jun 7, 2024 •

edited

Loading

sezero commented Jun 8, 2024

ericwa commented Jun 9, 2024

sezero commented Jun 9, 2024

Diordany commented Jun 9, 2024

sezero commented Jun 9, 2024

Diordany commented Jun 9, 2024

sezero commented Jun 9, 2024

Diordany commented Jun 10, 2024

sezero commented Jun 10, 2024

Diordany commented Jun 10, 2024

ericwa commented Jun 10, 2024

sezero commented Jun 10, 2024

negative uints #93

negative uints #93

Comments

Shpoike commented Jan 16, 2024

sezero commented Jan 16, 2024

ericwa commented Jan 16, 2024

sezero commented Jun 6, 2024 • edited Loading

ericwa commented Jun 6, 2024

sezero commented Jun 6, 2024

sezero commented Jun 6, 2024

sezero commented Jun 7, 2024 • edited Loading

sezero commented Jun 8, 2024

ericwa commented Jun 9, 2024

sezero commented Jun 9, 2024

Diordany commented Jun 9, 2024

sezero commented Jun 9, 2024

Diordany commented Jun 9, 2024

sezero commented Jun 9, 2024

Diordany commented Jun 10, 2024

sezero commented Jun 10, 2024

Diordany commented Jun 10, 2024

ericwa commented Jun 10, 2024

sezero commented Jun 10, 2024

sezero commented Jun 6, 2024 •

edited

Loading

sezero commented Jun 7, 2024 •

edited

Loading