skullernet / q2pro

Enhanced Quake 2 client and server
GNU General Public License v2.0
244 stars 82 forks source link

Concerning cl_maxfps, rounding, q2jump #350

Closed kke closed 3 months ago

kke commented 3 months ago

I've been playing q2jump a lot lately and there's a lot of cl_maxfps voodoo involved.

If I set cl_maxfps to 120, I get the warning about inexactness and stating "using 125 instead", which gets me kicked out of the server. I'm using 111 which seems to work for everything, but of course when failing to do some jumps I'm blaming the equipment and the client instead of my skills, so I went digging in the source a bit.

I'm not any kind of C/C++ expert so I may be a bit lost. Also this is a bit vague and rambley.

My first observation is this: https://github.com/skullernet/q2pro/blob/42e77caf98a98aa66fc5c3a2fb05ec793eede792/src/client/main.c#L2606-L2629

It uses a simplified way to calculate the real_maxfps compared to what the CL_UpdateFrameTimes function does, so I'm not sure if this message is even always correct?

I went to see how the jumpmod determines if the player should be kicked and looks like it just uses the value of cl_maxfps, which looks a bit insecure as the client could be reporting anything for that:

    // fps
    s = Info_ValueForKey (userinfo, "cl_maxfps");
    if (!s) { //needs stuffing
        ent->client->pers.stuffed = false;
    }

    // check for the string, fpskick mset
    if (strlen(s) && gset_vars->fpskick == 1) {
        ent->client->pers.fps = atoi(s);
        if (ent->client->pers.fps<20) { // kick for lower than 20
            gi.cprintf (ent,PRINT_HIGH, "[JumpMod]   You have been kicked for lowering CL_MAXFPS below 20\n");
            sprintf(temps,"kick %d\n",ent-g_edicts-1);
            gi.AddCommandString(temps);
        }
        if (ent->client->pers.fps>120) { // kick for higher than 120
            gi.cprintf (ent,PRINT_HIGH, "[JumpMod]   You have been kicked for raising CL_MAXFPS above 120\n");
            sprintf(temps,"kick %d\n",ent-g_edicts-1);
            gi.AddCommandString(temps);
        }
    }

There is a !fps command you can use in the chat and it calculates the actual FPS of everyone online by looking at the history of msecs and averaging over last five of them. For me it shows 111.1 when using cl_maxfps 111 and cl_async 0. To me this indicates that my physics FPS is actually truly 111.

I'm not sure if something should/could be changed or not. I feel like I can't make some of the jumps with the same cl_maxfps that people in the replays do but I'm just not quite as good as the masters.

The warn_on_fps_rounding is not where the true value of cl_maxfps cvar is set, so maybe it's not important or relevant. Maybe how the value is set or reported to the server should be investigated.

Next I'm going to stalk when there are some people online that use q2pro and see what !fps says for them.

skullernet commented 3 months ago

cl_warn_on_fps_rounding is purely informative and does not affect how cl_maxfps is set and reported to server. It exists simply to tell the user that attempting to set values like 120 for cl_maxfps does not work as user expects due to millisecond rounding (effective framerate will be 125). You can disable this warning if it bothers you.

In this regard, Q2PRO behavior should not be different from other clients that use milliseconds internally to measure time. This rounding issue can be alleviated by switching to microsecond precision for timekeeping, but this is a potentially breaking change with lots of subtle consequences I'd rather not attempt at this point.

If I set cl_maxfps to 120, I get the warning about inexactness and stating "using 125 instead", which gets me kicked out of the server.

Not sure why that would occur for you if according to the snippet you posted JumpMod only checks for cl_maxfps value from userinfo, and not actual framerate.

kke commented 3 months ago

Yeah the warning is kind of irrelevant in this puzzle, but I believe what it says can be different from what is actually set(?).

Not sure why that would occur for you if according to the snippet you posted JumpMod only checks for cl_maxfps value from userinfo, and not actual framerate.

Hmm, it actually looks like it doesn't. Maybe I remember wrong, perhaps I've tried to use 125 because that's the next one after 111 that doesn't warn, but indeed setting to 120 allows me to play.

When I set it to 120, !fps says 125 (which makes sense).

!fps
            cl_maxfps actual.
xxx                 66   61.0
yyy                 66   66.7
zzz                120  125.0
xyz                 66   47.2

Perhaps there isn't a problem, except possibly the warning that could in some conditions show a different value than what is used.

skullernet commented 3 months ago

I believe what it says can be different from what is actually set(?).

If you mean the wording (it reads like it sets cl_maxfps to some "corrected" value but in reality it doesn't set anything), then yes, wording can be improved. But I don't see a problem with calculation itself.

If you use cl_maxfps 66 you should get ~66.6 fps. If server estimates something different, and client window has keyboard focus, it's probably a network problem.

kke commented 3 months ago

warn_on_fps_rounding uses fps_to_msec but CL_UpdateFrameTimes uses fps_to_clamped_msec. I believe this causes the actual value that is used to be clamped between MIN_PHYS_HZ and MAX_PHYS_HZ. If you set cl_maxfps to 312, the warning says using 333 instead, but I think it's actually using 125, not 333.

But it should not be any kind of problem for anyone.

skullernet commented 3 months ago

Ah, I see why this can be a problem. I'll change it so that warning if printed only after clamping the cvar to allowed range.