sezero / quakespasm

QuakeSpasm -- A modern, cross-platform Quake game engine based on FitzQuake.
https://sourceforge.net/projects/quakespasm/
GNU General Public License v2.0
227 stars 96 forks source link

Increase default gl_farclip #62

Closed Flecked42 closed 1 year ago

Flecked42 commented 1 year ago

The default gl_farclip of 16384 needs to be increased for bigger maps. spasm0000

Map in image is ej3_aesop from https://www.slipseer.com/index.php?resources/explore-jam-3.228/

Increasing it to 65536 should probably be good enough.

sezero commented 1 year ago

Ironwail and vkQuake have gl_farclip as 16384. Does the issue show itself in those engines?

CC: @temx, @andrei-drexler

Flecked42 commented 1 year ago

Yes it happens in those engines aswell.

IW IW

VK VK

QS QS

IW and VK don't have the yellow stuff, but its the same issue.

sezero commented 1 year ago

Joequake recently bumped it to 65536: https://github.com/j0zzz/JoeQuake/commit/7ad2e8da49ac516c7019831bef369e009cd0ad73

@ericwa: Any objections? (CC: @andrei-drexler)

sezero commented 1 year ago

Default value of gl_farclip bumped to 65536 in git: Closing as fixed.

andrey-budko commented 1 year ago

Can we add glEnable(GL_DEPTH_CLAMP_NV); to avoid this?

master ![image](https://github.com/sezero/quakespasm/assets/6490190/86529941-ad64-4733-989b-7d855f94d6b9)
patched ![image](https://github.com/sezero/quakespasm/assets/6490190/add67bfe-983c-4c82-9d95-2695f749f55c)
diff --git a/Quake/gl_vidsdl.c b/Quake/gl_vidsdl.c
index cedc791..67a08f9 100644
--- a/Quake/gl_vidsdl.c
+++ b/Quake/gl_vidsdl.c
@@ -1351,6 +1351,7 @@ static void GL_SetupState (void)
    glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_REPEAT);
    glDepthRange (0, 1); //johnfitz -- moved here becuase gl_ztrick is gone.
    glDepthFunc (GL_LEQUAL); //johnfitz -- moved here becuase gl_ztrick is gone.
+   glEnable(GL_DEPTH_CLAMP_NV);
 }

 /*
sezero commented 1 year ago

Can we add glEnable(GL_DEPTH_CLAMP_NV); to avoid this?

Done: https://github.com/sezero/quakespasm/commit/74c47869742cce169b8569a33cd292d36d43917a , thanks.

andrey-budko commented 1 year ago

Probably we need to check GL_ARB_depth_clamp instead of GL_NV_depth_clamp, or probably both

https://registry.khronos.org/OpenGL/extensions/ARB/ARB_depth_clamp.txt

GL_EXTENSIONS on my Intel HD Graphics 4600 does not contain GL_NV_depth_clamp, but GL_ARB_depth_clamp is there

I used GL_DEPTH_CLAMP_NV just because it is already defined in SDL_opengl.h

sezero commented 1 year ago

Probably we need to check GL_ARB_depth_clamp instead of GL_NV_depth_clamp, or probably both

Done: Checking both now in git HEAD

sputnikutah commented 12 months ago

MH added this function to dynamically get the extent of the map at load time

static float R_GetFarClip (void) //MH
{
    // don't go below the standard Quake farclip
    float farclip = 4096.0f;
    int i;

    // this provides the maximum far clip per view position and worldmodel bounds
    for (i = 0; i < 8; i++)
    {
        float dist;
        vec3_t corner;

        // get this corner point
        if (i & 1) corner[0] = cl.worldmodel->mins[0]; else corner[0] = cl.worldmodel->maxs[0];
        if (i & 2) corner[1] = cl.worldmodel->mins[1]; else corner[1] = cl.worldmodel->maxs[1];
        if (i & 4) corner[2] = cl.worldmodel->mins[2]; else corner[2] = cl.worldmodel->maxs[2];

        if ((dist = VectorDistance(r_refdef.vieworg, corner)) > farclip)
            farclip = dist;
    }

    return farclip;
}

called in r_SetupGL

    farclip = R_GetFarClip();
    Cvar_SetValue("r_farclip",farclip);
sezero commented 12 months ago

MH added this function to dynamically get the extent of the map at load time

Doesn't seem to work as expecked with ej3_aesop (CC: @mhQuake )

mhQuake commented 12 months ago

This shouldn't be used at load time, as it depends on the current player position. It's appropriate to call it each frame at runtime and use it instead of the r_farclip cvar. It only takes player position into account, not orientation nor fov, so there is scope for optimization. It can also be micro-optimized by not updating it if the player's position hasn't changed since last time it was called.

It is of course dependent on the worldmodel bounds being correctly set. If they're set wrong, then it will likewise give the wrong result. If in doubt, recalc the worldmodel bounds correctly at load time.

sezero commented 12 months ago

This shouldn't be used at load time, as it depends on the current player position. It's appropriate to call it each frame at runtime and use it instead of the r_farclip cvar.

That's what I did: put it in R_SetupGL, not in R_NewMap

It only takes player position into account, not orientation nor fov, so there is scope for optimization. It can also be micro-optimized by not updating it if the player's position hasn't changed since last time it was called.

It is of course dependent on the worldmodel bounds being correctly set. If they're set wrong, then it will likewise give the wrong result. If in doubt, recalc the worldmodel bounds correctly at load time.

Seems too much hassle for corner cases. Player can always set gl_farclip by hand himself.

mhQuake commented 12 months ago

That's what I did: put it in R_SetupGL, not in R_NewMap

So other than that it's just getting distances from the player position to the 8 corners of the worldmodel bounding box. So either mathematics is broken, or something else is wrong. Is the player position valid at the time you call it? Have you verified that the worldmodel bounding box is correct?

Seems too much hassle for corner cases. Player can always set gl_farclip by hand himself.

I do understand the rationale behind this, I just don't agree with it. If a solution can be found to an issue that doesn't require player input, then it should at least be considered. Requiring player input is a barrier and can lead to false bug reports. A quick once-per-frame calculation of the distance from the player position to the farthest corner of a bounding box hardly seems "hassle". It depends on who should have the "hassle" inflicted on them: the player or the engine?

sezero commented 12 months ago

That's what I did: put it in R_SetupGL, not in R_NewMap

So other than that it's just getting distances from the player position to the 8 corners of the worldmodel bounding box. So either mathematics is broken, or something else is wrong. Is the player position valid at the time you call it? Have you verified that the worldmodel bounding box is correct?

Haven't put serious effort in it. The call chain is the standard SCR_UpdateScreen -> V_RenderView -> R_RenderView -> R_RenderScene -> R_SetupScene -> R_SetupGL