search

mvdevs / jk2mv

JK2MV - improved, modernized JK2 client and server
https://jk2mv.org
GNU General Public License v2.0
105 stars 33 forks source link

Option to not drop connection when cl.snap.serverTime < cl.oldFrameServerTime? #146

Closed TomArrow closed 2 years ago

TomArrow commented 2 years ago

With bad connection, sometimes cl.snap.serverTime < cl.oldFrameServerTime happens. As per now, this results in an error that forces a disconnect from the server, which is kinda frustrating.

I think it's here: https://github.com/mvdevs/jk2mv/blob/1077d69d22376dd90f15920a97834637dd9c74bf/src/client/cl_cgame.cpp#L1571

Would be nice to add a settable option that allows to downgrade this error from an ERR_DROP to a PRINT_WARNING or something like that. Maybe it could even be auto-on? Is there a good reason to force a disconnect when this happens? In the case of a bad internet connection it doesn't seem like the desired behavior and since it's a clientside behavior, it seems it wouldn't interfere with the server either.

Just a thought.

TomArrow commented 2 years ago

It looks like this problem might only exist on a fork. I thought I remembered it in jk2mv too but maybe I was wrong. I will try to reproduce a bit more, otherwise just consider this nil.

TomArrow commented 2 years ago

Okay I was finally able to reproduce it. I had no server with an active game to test on earlier, maybe that's why I failed to reproduce. Proof:

2021-10-29 22_44_27-JK2MV

aufau commented 2 years ago

Fixed in 2eae94300395072598eb88c6f5b97ab2218beb4c Sorry I forgot to add you as reported-by and tested-by in commit message and don't want to ammend now that I've pushed to the master.

TomArrow commented 2 years ago

No problem! Thanks for the fix.

entdark commented 2 years ago

Should that be changed in all q3-based engines?

P.S.: you can leave the reporter and the tester in the comment section under the commit.

TomArrow commented 2 years ago

Maybe. The base Q3A repo by id software has a similar fix https://github.com/id-Software/Quake-III-Arena/blob/dbe4ddb10315479fc00086f08e25d968b4b43c49/code/qcommon/net_chan.c#L448

entdark commented 2 years ago

Yeah but their "fix" was part of the original source code released in 2005 of version 1.32b. I guess they worked close to Raven Software to share some fixes. But Raven Software were wuss to apply the "fix" according to the comments. The very first commit of Jedi Outcast doesn't contain the "fix". It was added in the second one: https://github.com/jedis/jedioutcast/commit/a8c260f9e47dbf490efa75eb2047fdfe6b7138f4#diff-8f9663e86e075916aed4ca92fe1824910e99c4595f24b5cca73895030567d300R451 So according to this the "fix" wasn't in Q3 source code before 2002-2003 (since Raven Software licensed it and used for themself and the very first commit doesn't have the "fix") and was applied in some earlier versions of Q3 before 1.32b. But we will never know which.

TomArrow commented 2 years ago

Makes sense. Either way just looking at the code logic, even if it wasn't the cause of this bug, it does seem like a clear logical flaw to not update the incoming sequence once the fragmented message is assembled.

aufau commented 2 years ago

As far as I understand this is how it should have always been. The way I found it is by looking where that function returns qtrue and checking if there is chan->incomingSequence = sequence; line there - debugged it and found the error/solution independently. Commented line there only confirmed my diagnosis.

If that code branch was reached, cg.snap was updated with new snapshot but incomingSequence wasn't. This made it possible to receive an earlier snapshot as next snapshot (due to packet reordering), parse it as new snapshot (because incomingSequence was not updated) and "downgrade" cg.snap to older snapshot which caused reported error.

aufau commented 2 years ago

I'm gonna write down how it happened in case someone asks me about it N years later.

To reproduce: