gl_overbright cvar not working

[Flecked42]

This issue happens with the current git build. It does not happen with 95.1 release.

Also the issue is not present with -noglsl

Maybe it was caused from the "blending lights" fix?

[sezero]

Maybe it was caused from the "blending lights" fix?

Possible. @andrey-budko ?

[andrey-budko]

I knew about gl_overbright 0 issue, that is why I wrote "for testing purposes only", sorry for not saying it explicitly. But which render is reference for "no overbright" quakespasm? The previous quakespasm release? glquake renders those test maps differently.

[sezero]

I knew about gl_overbright 0 issue, that is why I wrote "for testing purposes only", sorry for not saying it explicitly.

And obviously I did not pay enough attention, so nothing to be sorry about :)

How big is the problem really? The original lighting issue looked somehow a minor and I can still revert the patch.

[sezero]

Also: @Flecked42: In order to make sure the 10bit color lightmap patch is the culprit, run with -nopackedpixels and see that the desired behavior is back.

[Flecked42]

The issue is not present with -nopackedpixels

[andrey-budko]

Is enabling the new behavior only for "overbright on" a good idea?

[sezero]

Is enabling the new behavior only for "overbright on" a good idea?

Ugh.. You mean change things like if (gl_packed_pixels) to if (gl_packed_pixels && gl_overbright.value) ??

Sounds like adding extra complication possibly not actually worth?? Is https://github.com/sezero/quakespasm/issues/47 really such a common issue worth this much trouble??

@andyp123, @ericwa (and @andrei-drexler): I am tending towards reverting the 10 bit color depth lightmaps patch. Objections?? (Or solutions???)

[Flecked42]

I'd take the 10 bit color depth lightmaps over not being able to disable overbright any day, but that's just my opinion.

I doubt many people disable overbright anyway, but i could be wrong.

[sezero]

I'd take the 10 bit color depth lightmaps over not being able to disable overbright any day, but that's just my opinion.

I doubt many people disable overbright anyway, but i could be wrong.

Having regressions in a new release (getting close to one) is not a good thing.

Will be waiting for others' comments a bit.

[andrey-budko]

Ugh.. You mean change things like if (gl_packed_pixels) to if (gl_packed_pixels && gl_overbright.value) ??

I suggest something like this: https://github.com/andrey-budko/ironwail/commit/beac3ff082eebedd4aea3109a64d4918218c78a8

[sezero]

Ugh.. You mean change things like if (gl_packed_pixels) to if (gl_packed_pixels && gl_overbright.value) ??

I suggest something like this: andrey-budko@beac3ff

Yeah, practically the same thing (with nicer uniform name, I admit)

[sezero]

BTW, joequake adopted the 10 bit lightmaps patch and has gl_overbright. Does joequake have the same issue?

(CC'ing @j0zzz too to hear if he has anything to say, maybe even a solution.)

[Flecked42]

Yes it does. I let Joe know about it just before I posted the issue here.

Ironwail just removed the gl_overbright cvar, which is probably the way to go imo. vkQuake removed it also.

[j0zzz]

I only experienced the problem but haven't spent any efforts to find a solution. From joequake's point of view fixing it isn't necessary imo, since it was using a brighter palette for bmodel and world textures already for a long time now, without any option to switch it off. And nobody requested any CR to darken it back.

[sezero]

Ugh.. You mean change things like if (gl_packed_pixels) to if (gl_packed_pixels && gl_overbright.value) ??

I suggest something like this: andrey-budko@beac3ff

If I am to do this it will be as simple as the following two-liner (for current git HEAD which had some clean-ups leading to it.) And if I am to disable the feature, it will be as simple as commenting out the Cvar_SetROM call in there, instead.

Will wait a bit more before a final decision.

diff --git a/Quake/gl_rmisc.c b/Quake/gl_rmisc.c
index 93791f6..d7d5a2b 100644
--- a/Quake/gl_rmisc.c
+++ b/Quake/gl_rmisc.c
@@ -60,6 +60,7 @@ GL_Overbright_f -- johnfitz
 */
 static void GL_Overbright_f (cvar_t *var)
 {
+   Cvar_SetROM ("r_lightmapwide",(gl_packed_pixels && var->value) ? "1" : "0");
    R_RebuildAllLightmaps ();
 }

@@ -220,7 +221,7 @@ void R_Init (void)
    Cvar_SetCallback (&r_noshadow_list, R_Model_ExtraFlags_List_f);
    //johnfitz
    Cvar_RegisterVariable (&r_lightmapwide);
-   Cvar_SetROM ("r_lightmapwide", gl_packed_pixels ? "1" : "0");
+   Cvar_SetROM ("r_lightmapwide",(gl_packed_pixels && gl_overbright.value) ? "1" : "0");

    Cvar_RegisterVariable (&gl_zfix); // QuakeSpasm z-fighting fix
    Cvar_RegisterVariable (&r_lavaalpha);

[ericwa]

Side note: testing on c8cee0f8bc0b2049253de34ea3276fc4513f1e35 ..

r_lightmap 1 is also broken (too dark) with the new 10-bit lightmaps.

We use fixed-function GL for r_lightmap 1. This bit of R_DrawTextureChains will need updating:

    if (r_lightmap_cheatsafe)
    {
        if (!gl_overbright.value)
        {
            glTexEnvf(GL_TEXTURE_ENV, GL_TEXTURE_ENV_MODE, GL_MODULATE);
            glColor3f(0.5, 0.5, 0.5);
        }

[sezero]

Side note: testing on c8cee0f ..

r_lightmap 1 is also broken (too dark) with the new 10-bit lightmaps.

Do you have a patch for the issues?

[ericwa]

Do you have a patch for the issues?

Not yet, I think the r_lightmap implementation needs to be moved to the glsl shader in that case. Seems like multiplying by 4 or 8 with fixed-function is nontrivial..

Regarding the "r_overbright 0" + 10 bit lightmaps combination:

• "r_overbright 0" basically means the input lightmap values are clamped at 127, which becomes 1.0 the the shader. (so they're multiplied by 2 before uploading to an 8-bit texture, which gives a 0..1 range in the shader).
• "r_overbright 1" means the input lightmap values 0..255 are uploaded as-is, then mapped to 0..2 in the shader.

So I think we can emulate the "r_overbright 0" look in the 10-bit codepath - we just need to do the clamping properly before uploading the texture.

I'll see if I can come up with a patch this weekend that fixes these issues while hopefully keeping the code complexity reasonable, ebcause it's easy to get mixed up about why there's a give * 2 or * 4 factor.

[sezero]

Thanks.

P.S.: There is another issue with the patch that it is broken for non-GLSL mode, i.e. broken with -noglslalias : I can still live with that (with teeth grinding) but only if the regressions are fixed properly. Otherwise the patch should go away..

[ericwa]

P.S.: There is another issue with the patch that it is broken for non-GLSL mode, i.e. broken with -noglslalias : I can still live with that (with teeth grinding) but only if the regressions are fixed properly. Otherwise the patch should go away..

I think this part is expected/OK to me, since rendering the 10-bit lightmaps requries brightness compensation in the shaders (4), and apparently it's not possible to just do a `4` with fixed function GL.

Here's my patch:

fixes to 10-bit lightmap support
- Fix r_lightmap 1 (was rendering too dark)
- Fix gl_overbright 0 (was having no effect)

The gl_overbright 0 fix was short (R_BuildLightMap) and fixing r_lightmap was more involved.

diff --git a/Quake/gl_rmisc.c b/Quake/gl_rmisc.c
index 93791f65..04ca6c29 100644
--- a/Quake/gl_rmisc.c
+++ b/Quake/gl_rmisc.c
@@ -60,7 +60,6 @@ GL_Overbright_f -- johnfitz
 */
 static void GL_Overbright_f (cvar_t *var)
 {
-   Cvar_SetROM ("r_lightmapwide", gl_packed_pixels ? "1" : "0");
    R_RebuildAllLightmaps ();
 }

diff --git a/Quake/r_brush.c b/Quake/r_brush.c
index 4495b0bf..3af68e3e 100644
--- a/Quake/r_brush.c
+++ b/Quake/r_brush.c
@@ -1239,6 +1239,11 @@ void R_BuildLightMap (msurface_t *surf, byte *dest, int stride)
                    r = *bl++ >> 7;
                    g = *bl++ >> 7;
                    b = *bl++ >> 7;
+
+                   // artifically clamp to 255 so gl_overbright 0 renders as expected in the wide10bits case
+                   r = (r > 255) ? 255 : r;
+                   g = (g > 255) ? 255 : g;
+                   b = (b > 255) ? 255 : b;
                }
                if (wide10bits)
                {
@@ -1276,6 +1281,11 @@ void R_BuildLightMap (msurface_t *surf, byte *dest, int stride)
                    r = *bl++ >> 7;
                    g = *bl++ >> 7;
                    b = *bl++ >> 7;
+
+                   // artifically clamp to 255 so gl_overbright 0 renders as expected in the wide10bits case
+                   r = (r > 255) ? 255 : r;
+                   g = (g > 255) ? 255 : g;
+                   b = (b > 255) ? 255 : b;
                }
                if (wide10bits)
                {
diff --git a/Quake/r_world.c b/Quake/r_world.c
index d117c2c5..1f327cb6 100644
--- a/Quake/r_world.c
+++ b/Quake/r_world.c
@@ -554,6 +554,7 @@ static GLint  useFullbrightTexLoc;
 static GLint  useOverbrightLoc;
 static GLint  useAlphaTestLoc;
 static GLint  useLightmapWideLoc;
+static GLint  useLightmapOnlyLoc;
 static GLint  alphaLoc;

 #define vertAttrIndex 0
@@ -639,6 +640,7 @@ void R_DrawTextureChains_Water (qmodel_t *model, entity_t *ent, texchain_t chain
        GL_Uniform1iFunc (useOverbrightLoc, overbright);
        GL_Uniform1iFunc (useAlphaTestLoc, 0);
        GL_Uniform1iFunc (useLightmapWideLoc, wide10bits);
+       GL_Uniform1iFunc (useLightmapOnlyLoc, 0);

        for (i=0 ; i<model->numtextures ; i++)
        {
@@ -849,6 +851,7 @@ void GLWorld_CreateShaders (void)
        "uniform bool UseOverbright;\n"
        "uniform bool UseAlphaTest;\n"
        "uniform bool UseLightmapWide;\n"
+       "uniform bool UseLightmapOnly;\n"
        "uniform float Alpha;\n"
        "\n"
        "varying float FogFragCoord;\n"
@@ -856,6 +859,8 @@ void GLWorld_CreateShaders (void)
        "void main()\n"
        "{\n"
        "   vec4 result = texture2D(Tex, gl_TexCoord[0].xy);\n"
+       "   if (UseLightmapOnly)\n"
+       "       result = vec4(0.5, 0.5, 0.5, 1.0);\n"
        "   if (UseAlphaTest && (result.a < 0.666))\n"
        "       discard;\n"
        "   result *= texture2D(LMTex, gl_TexCoord[1].xy);\n"
@@ -888,6 +893,7 @@ void GLWorld_CreateShaders (void)
        useOverbrightLoc = GL_GetUniformLocation (&r_world_program, "UseOverbright");
        useAlphaTestLoc = GL_GetUniformLocation (&r_world_program, "UseAlphaTest");
        useLightmapWideLoc = GL_GetUniformLocation (&r_world_program, "UseLightmapWide");
+       useLightmapOnlyLoc = GL_GetUniformLocation (&r_world_program, "UseLightmapOnly");
        alphaLoc = GL_GetUniformLocation (&r_world_program, "Alpha");
    }
 }
@@ -943,6 +949,7 @@ void R_DrawTextureChains_GLSL (qmodel_t *model, entity_t *ent, texchain_t chain)
    GL_Uniform1iFunc (useOverbrightLoc, overbright);
    GL_Uniform1iFunc (useAlphaTestLoc, 0);
    GL_Uniform1iFunc (useLightmapWideLoc, wide10bits);
+   GL_Uniform1iFunc (useLightmapOnlyLoc, 0);
    GL_Uniform1fFunc (alphaLoc, entalpha);

    for (i=0 ; i<model->numtextures ; i++)
@@ -1012,6 +1019,90 @@ void R_DrawTextureChains_GLSL (qmodel_t *model, entity_t *ent, texchain_t chain)
    }
 }

+/*
+================
+R_DrawLightmapChains_GLSL -- ericw
+================
+*/
+void R_DrawLightmapChains_GLSL(qmodel_t* model, entity_t* ent, texchain_t chain)
+{
+   const int   overbright = !!gl_overbright.value;
+   const int   wide10bits = !!r_lightmapwide.value;
+
+   int         i;
+   msurface_t* s;
+   texture_t* t;
+   int     lastlightmap;
+
+   GL_UseProgramFunc(r_world_program);
+
+   // Bind the buffers
+   GL_BindBuffer(GL_ARRAY_BUFFER, gl_bmodel_vbo);
+   GL_BindBuffer(GL_ELEMENT_ARRAY_BUFFER, 0); // indices come from client memory!
+
+   GL_EnableVertexAttribArrayFunc(vertAttrIndex);
+   GL_EnableVertexAttribArrayFunc(texCoordsAttrIndex);
+   GL_EnableVertexAttribArrayFunc(LMCoordsAttrIndex);
+
+   GL_VertexAttribPointerFunc(vertAttrIndex, 3, GL_FLOAT, GL_FALSE, VERTEXSIZE * sizeof(float), ((float*)0));
+   GL_VertexAttribPointerFunc(texCoordsAttrIndex, 2, GL_FLOAT, GL_FALSE, VERTEXSIZE * sizeof(float), ((float*)0) + 3);
+   GL_VertexAttribPointerFunc(LMCoordsAttrIndex, 2, GL_FLOAT, GL_FALSE, VERTEXSIZE * sizeof(float), ((float*)0) + 5);
+
+   // set uniforms
+   GL_Uniform1iFunc(texLoc, 0);
+   GL_Uniform1iFunc(LMTexLoc, 1);
+   GL_Uniform1iFunc(fullbrightTexLoc, 2);
+   GL_Uniform1iFunc(useFullbrightTexLoc, 0);
+   GL_Uniform1iFunc(useOverbrightLoc, overbright);
+   GL_Uniform1iFunc(useAlphaTestLoc, 0);
+   GL_Uniform1iFunc(useLightmapWideLoc, wide10bits);
+   GL_Uniform1fFunc(alphaLoc, 1.0f);
+   GL_Uniform1iFunc(useFullbrightTexLoc, 0);
+   GL_Uniform1iFunc(useLightmapOnlyLoc, 1);
+
+   R_ClearBatch();
+   lastlightmap = -1;
+
+   for (i = 0; i < model->numtextures; i++)
+   {
+       t = model->textures[i];
+
+       if (!t || !t->texturechains[chain] || t->texturechains[chain]->flags & (SURF_DRAWTILED | SURF_NOTEXTURE))
+           continue;
+
+       if (t->texturechains[chain]->texinfo->flags & TEX_SPECIAL)
+           continue; // unlit water
+
+       for (s = t->texturechains[chain]; s; s = s->texturechain)
+       {
+           if (s->lightmaptexturenum < 0)
+               continue;
+
+           if (s->lightmaptexturenum != lastlightmap)
+           {
+               R_FlushBatch();
+
+               GL_SelectTexture(GL_TEXTURE1);
+               GL_Bind(lightmaps[s->lightmaptexturenum].texture);
+               lastlightmap = s->lightmaptexturenum;
+           }
+           R_BatchSurface(s);
+
+           rs_brushpasses++;
+       }
+   }
+
+   R_FlushBatch();
+
+   // clean up
+   GL_DisableVertexAttribArrayFunc(vertAttrIndex);
+   GL_DisableVertexAttribArrayFunc(texCoordsAttrIndex);
+   GL_DisableVertexAttribArrayFunc(LMCoordsAttrIndex);
+
+   GL_UseProgramFunc(0);
+   GL_SelectTexture(GL_TEXTURE0);
+}
+
 /*
 =============
 R_DrawWorld -- johnfitz -- rewritten
@@ -1046,6 +1137,12 @@ void R_DrawTextureChains (qmodel_t *model, entity_t *ent, texchain_t chain)

    if (r_lightmap_cheatsafe)
    {
+       if (r_world_program != 0)
+       {
+           R_DrawLightmapChains_GLSL(model, ent, chain);
+           return;
+       }
+
        if (!gl_overbright.value)
        {
            glTexEnvf(GL_TEXTURE_ENV, GL_TEXTURE_ENV_MODE, GL_MODULATE);

[sezero]

@ericwa: Thanks, will apply shortly.

Would it be worth conditionalizing the artifically clamp to if (wide10bits) ?

[ericwa]

Would it be worth conditionalizing the artifically clamp to if (wide10bits) ?

Yes, that might make it clearer.

[sezero]

OK thanks.

PS: #78 is another issue for which I'll revert the relevant change.

[sezero]

Patch applied as https://github.com/sezero/quakespasm/commit/a2677856c010fc4a50ee57fd19c693980a9ce850

Would it be worth conditionalizing the artifically clamp to if (wide10bits) ?

Yes, that might make it clearer.

That follow-up applied as https://github.com/sezero/quakespasm/commit/0f66fcc51e9d5cd38a0dc67c0be5b85e9a50e112

Closing as fixed. @Flecked42: Please test latest git to confirm.

[Flecked42]

Yes fixed on my end