negative uints - Githubissues

Shpoike commented 8 months ago

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 commented 8 months ago

Do you have a patch?

CC: @andrei-drexler, @ericwa

ericwa commented 8 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

sezero commented 3 months ago

.. 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 commented 3 months ago

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 commented 3 months ago

... 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 commented 3 months ago

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

sezero commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

Diordany commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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.

sezero commented 3 months ago

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

Thanks.

sezero / quakespasm

negative uints #93