skullernet / q2pro

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

Certain movement commands can get lost between movement packets #328

Open Paril opened 1 year ago

Paril commented 1 year ago

In certain circumstances, a press, depress followed by a press can lead to a complete loss of the depress state, causing the server to never even know the key was unpressed, causing "missed" double-jumps for instance.

What lead me down this road is I was noticing that, despite having identical packet rates, Quake II Re-release is much easier to consistently do jumps with, and I couldn't remember exactly what it was that I had done to the movement code in Q2RR to make them work so consistently. After a bit of experimentation (please note that this is the re-release game branch, which uses a button state instead of upmove and has 40hz server tick rate, but the movement code was mostly unchanged - I compared how upmove is handled in vanilla and the issue would still be present there), I think I narrowed down the problem: Q2PRO doesn't consider upmove to be "important" when it changes (ie not an instant packet), meaning the server may completely miss a depress like it can do if cl_instantpacket is 0 with +attack/+use.

I wrote a quick UI tool for debugging the problem. I picked a low timescale so I could watch the feed a bit easier, I slowed down my presses to compensate: https://streamable.com/spw90g

Top-left is real time input status; center-left is the last client-side cmd, bottom-left is the last server-side move cmd buttons aggregated together (since you can technically get more than one in a row; note that this display wouldn't be accurate if the movement cmd has two movements, one with down and one with up, but I don't think it's entirely necessary for this display). Here's a more realistic example, trying to hit the DJ up to the mega: https://streamable.com/4fuwyf

Notice how the server never takes me off of jump, even though the client predicted that it should have, multiple times, and causes prediction misses in double jumping. I think this is the issue that q2rr ends up solving, because jump/crouch buttons are always instant, so the server should always receive the "depress" command even if a press immediately follows it.

The only worry I have in changing this is that the movement crowd tends to see any change to usercmd/movement to be a negative, even though I think this is a legitimate defect, heh.

On my branch, I have added the cl.sendPacketNow. logic to In_Down* which seems to solve the issue and allows jumps to be more consistent (https://streamable.com/qsbhp8) as well as making crouch work more immediately in preventing prediction errors, but I don't know if it's the best solution.

skullernet commented 1 year ago

Can this inconsistency be reproduced with upstream Q2PRO source code only, i.e. without BUTTON_JUMP changes? For me double jumping works quite consistently if using cl_maxfps ≥ 60.

But of course, press-depress-press sequences can be missed if keys are pressed/released fast enough, because there is no event queue, only final key state that's sampled at cl_maxfps rate. But this is how it always worked in Q2, you have to time your jumps for double jump to work consistently. Also I'm not sure how converting upmove to BUTTON_JUMP bit is going to help with this.

Are you getting actual prediction misses between client/server (that show up with cl_showmiss)? Because otherwise I don't think there's an issue.

It should be noted that running pending client cmd can produce different results (and possible intermittent visual glitches), but this is just cosmetic issue, because pending cmd is never sent to server. This is due to nature of how cl_async implementation works in Q2PRO.

cl_instantpacket is to allow +attack or +use button presses to be sent to server immediately ignoring cl_maxpackets limit. I don't really think it should be abused to make double jumping "easier".

Paril commented 1 year ago

The upmove/BUTTON_JUMP thing is just an implementation detail that Q2RR does, it's not important to this issue - just wanted to note it.

Yeah, it is possible to reproduce this on the upstream version - I have an earlier build from around 2020 (source build) and it does happen there too. There are cases where a double-jump should have registered, but the server didn't receive me depressing the jump button at all causing PMF_JUMP_HELD to stick between button presses, which is why I consider this a defect - this does not occur for attacking or +use because of the instant packet 'hack', but this same hack doesn't apply to jumping which is similar in design (just attached to a different field rather than buttons). In theory you can actually work around this limitation by using an alias that does both +moveup and +use (or +attack) at the same time, which should basically accomplish the same goal and makes jumping less prone to sticking, but I can't confirm atm if that actually helps or not since I don't have this same debug display on this old build.

The only thing that it really changes, making this packet instant like q2rr does, is to make jumps feel more reliable and give the server a more accurate representation of your button states (or, well, upmove state in the case of vanilla). I guess it boils down to the question of why (other than historical accuracy) is +moveup treated differently to +attack and +use when they're both things that should be, in theory, instant actions; we made them instant in Q2RR but I know it's a bit of a breaking change for Q2PRO, so I dunno. It's always frustrating to miss a jump, even at 62 maxfps, that you know for sure you should have landed but it didn't because you happened to press the button directly after a packet was sent from firing or whatever and the -moveup got lost - you have no control over that timing and it doesn't really feel fair, imo, and your only recourse to have it feel as consistent as Q2RR is to either pump up maxfps (which has its own set of problems in vanilla because of the attraction towards origin) or try to find a workaround like forcing a +attack/+use to instantly send that packet out.

EDIT: also I believe you're right regarding prediction misses; I think it was just the pending cmd visual artifacting, which looked to me like a miss but it doesn't register on cl_showmiss. That being said, making jump instant does eliminate the artifacting from jumping from what I can tell.