akmdm akmdm akmdm akmdm akmdm akmdm akmdm akmdm akmdm akmdm I just added patches 10496-10499 which further refine the skip correction, particularly with the backtrace subroutine.
akmdm akmdm akmdm akmdm akmdm akmdm I added some patches that fix more issues that were found during testing.
Thanks! I added 10500-10503. Can you describe the idea of 10504?
akmdm akmdm akmdm The new patches look fine! I added 10508-10510.
akmdm akmdm akmdm akmdm akmdm akmdm akmdm akmdm akmdm akmdm akmdm akmdm akmdm akmdm akmdm akmdm akmdm akmdm akmdm akmdm akmdm akmdm akmdm akmdm akmdm akmdm I added some new patches which fix or improvement the performance of the skip correction. I also deleted all older attachments which have now long been pushed into the repository.
Thanks for the new patches! I'm currently looking through them and I have a question on 10525: SERVERCOMMANDS_SetLocalPlayerJumpTics is called at two places in the code, but you only block it from being called while backtracking in one of these places. Is this intentional? If not, I would prevent it from being called like you did for SERVERCOMMANDS_SoundActor in 10524.
akmdm)
10526: It's not obvious whether lLastBacktraceTic is always containing a reasonable value, since it's not explicitly initialized unconditionally. It's only set when a backtrack is done. Before that it contains a random value, but is potentially accessed in an if condition.
akmdm)
akmdm Reply To torr_samaho
Thanks for the new patches! I'm currently looking through them and I have a question on 10525: SERVERCOMMANDS_SetLocalPlayerJumpTics is called at two places in the code, but you only block it from being called while backtracking in one of these places. Is this intentional? If not, I would prevent it from being called like you did for SERVERCOMMANDS_SoundActor in 10524.
No, I must've overlooked the second instance (inside P_ZMovement) of SERVERCOMMANDS_SetLocalPlayerJumpTics being called while a player is being extrapolated. I agree that the extrapolation check should be made inside of the command function itself just to be on the safe side.
P.S. I thought I was adding a reply, but I was actually editing your comments earlier. My bad.
akmdm)
akmdm Reply To torr_samaho
10526: It's not obvious whether lLastBacktraceTic is always containing a reasonable value, since it's not explicitly initialized unconditionally. It's only set when a backtrack is done. Before that it contains a random value, but is potentially accessed in an if condition.
Yikes, I forgot to initialize lLastBacktraceTic to 0 inside of SERVER_ResetClientTicBuffer. I'll fix that quickly.
akmdm akmdm Thanks for the updated patches! I now added everything except for 10529: Here, the new/delete handling looks very error prone. If the constructor of a class/struct (here CLIENT_PLAYER_DATA_s) calls new, this class should also delete. BTW: Why are storing PositionData as pointer in the first place? You wouldn't need to worry about new/delete if your member PositionData is an instance of MoveThingData instead of a pointer.
akmdm Reply To torr_samaho
BTW: Why are storing PositionData as pointer in the first place? You wouldn't need to worry about new/delete if your member PositionData is an instance of MoveThingData instead of a pointer.
Before, I was having a lot trouble defining PositionData as an object (including sv_commands.h into sv_main.h wouldn't work either), so I kept it as a pointer to avoid those problems, as long as the memory heap was deleted when it needed to be deleted. Now, it seems I have to move the declaration of the MoveThingData struct int sv_main.h and add a default constructor to ensure the PositionData member can be defined as an object without throwing compiler errors anywhere. I want to double check that the skip correction still works correctly before I re-upload the patch, but I don't think there should be any issues with it anyways. I'm going to upload a separate patch for moving the struct between header files.
akmdm akmdm akmdm I fixed up 10529.patch in two new patches: 10529_v2a.patch and 10529_v2b.patch. One patch moves MoveThingData into sv_main.h and renames it to MOVE_THING_DATA_s (to match the naming convention of the structs defined in that header file), the other patch applies the original 10529.patch without the extra dynamic memory allocation based, so it should be more stable now.
Thanks, the updates patches look good. I just added both of them.
akmdm akmdm akmdm akmdm akmdm akmdm akmdm akmdm akmdm akmdm akmdm akmdm akmdm akmdm akmdm I uploaded new patches that further refine the skip correction, especially the back trace subroutine, based on my own testing and feedback I got from other people. I also took the time to cleanup and reorganize the code to improve readability and make it easier to follow.
Thanks for the patches!
Quick question: 10603 seems to remove the pActor->player->mo == pActor check. Is that intentional?
akmdm Reply To torr_samaho
Thanks for the patches! Quick question: 10603 seems to remove the pActor->player->mo == pActor check. Is that intentional?
Yes, I removed this check because I originally used it to prevent the server crashing when a client was connecting, meaning that the game simulation was running and AActor::Tick was running. Replacing the check with SERVER_IsValidClient doesn't appear to be causing crashes either.
EDIT: Actually, let me double-check that it still won't crash if SERVERCOMMANDS_SoundActor is called either.
akmdm)
The check pActor->player->mo == pActor ensures that pActor is not a voodoo doll. That's why I was wondering why you don't need it anymore.
akmdm Thinking about it more, 10603 shouldn't reintroduce the crashes I previously fixed with the pActor->player->mo check, because SERVER_IsValidClient still returns false if ulClient >= MAXPLAYERS. The only reason for the pActor->player->mo check was in case pActor->player - players was equal to MAXPLAYERS, which was only the case when a client was connecting or authenticating a level. SERVER_GetClient( MAXPLAYERS )->bIsBactracing is what caused the crash, and 10603 should still prevent this from happening, so it's safe to push.
akmdm)
Thanks for clarifying! If this was just about crashes and not voodoo dolls, it should be fine to remove it.
akmdm To be honest, I didn't think about voodoo dolls when I added that check. The crash could happen whether voodoo dolls existed on the level or not. Maybe it's something worth noting.
Thanks! I added the two new patches.
akmdm akmdm I added some new patches to improve the skip correction.
akmdm Thanks for the new patches! I reviewed and added all of them.
akmdm akmdm akmdm I uploaded at least a dozen new patches to try and improve the skip correction, based on a list problems that I could think of.
- I decided to limit the number of tics the server can extrapolate a player's movement to only 7 tics (by default, it will extrapolate for 3 tics). It's best if the skip correction focuses only on eliminating the smaller but more periodic stutters due to a player's bad connection, rather than doing too much to eliminate all kinds of stuttering. The less the server needs to predict, the better.
- I removed ServerCommands::UpdateLocalPlayerGametics and modified ServerCommands::MoveLocalPlayer such that the lagging player's movement is still smooth on their own ends.
- Tightened restrictions with the backtrace: it now always fails if the player teleported, took environmental damage, or activated any specials during extrapolation.
- Any specials the player activates during a backtrace will now only execute if the backtrace succeeds.
Unfortunately, I haven't had a chance to test out these changes thoroughly yet, other than checking for stability.
Thanks for the new patches! Some quick comments:
- 10668: If I understood the patch correctly, the client is only informed about the state of the settings when the client connects. I don't think it makes sense to send these values only when the client connects. Either the client is supposed to know those values, then they should be also updated on the client when the server changes them while the client is already connected, or the client is not informed about those values at all.
- 10669: This seems to change more than the commit message says: server_PerformBacktrace does not seem to update pClient->ulClientGameTic anymore in any case.
akmdm Reply To torr_samaho
Thanks for the new patches! Some quick comments: - 10668: If I understood the patch correctly, the client is only informed about the state of the settings when the client connects. I don't think it makes sense to send these values only when the client connects. Either the client is supposed to know those values, then they should be also updated on the client when the server changes them while the client is already connected, or the client is not informed about those values at all. - 10669: This seems to change more than the commit message says: server_PerformBacktrace does not seem to update pClient->ulClientGameTic anymore in any case.
10668: Because sv_smoothplayers, sv_extrapolatetics, and sv_backtracethreshold all have the CVAR_SERVERINFO flag, when they're changed on the server's end, the server already updates them automatically on the client's end via SVC2_SETCVAR. Therefore, it was only necessary for me to inform the clients about the skip correction's settings upon connecting, since the server doesn't do this automatically.
10669: Because the backtrace can now only be performed when there's at least one move command in the client's tic buffer, I don't have to update pClient->ulClientGameTic anymore, since the "on time" move command will update ulClientGameTic to what it's supposed to be right after. Updating that member in server_PerformBacktrace was only necessary in case the player needed to be extrapolated again, right after a backtrace was performed on the same tic.
akmdm)
Thanks for the swift answer! 10669 sounds good, but 10668 sounds like there is a general bug. I thought only cvars with CVAR_MOD and CVAR_SERVERINFO are synced automatically to the clients. Perhaps something went wrong when I ported mod cvars. If all cvars with CVAR_SERVERINFO are synced automatically, we shouldn't need SERVERCOMMANDS_SetGameModeLimits and all those cvars should be send to the client upon joining. Do you know where this sync is done? All places I found so far check CVAR_MOD.
akmdm Reply To torr_samaho
Thanks for the swift answer! 10669 sounds good, but 10668 sounds like there is a general bug. I thought only cvars with CVAR_MOD and CVAR_SERVERINFO are synced automatically to the clients. Perhaps something went wrong when I ported mod cvars. If all cvars with CVAR_SERVERINFO are synced automatically, we shouldn't need SERVERCOMMANDS_SetGameModeLimits and all those cvars should be send to the client upon joining. Do you know where this sync is done? All places I found so far check CVAR_MOD.
After doing some debugging, it appears that SERVERCOMMANDS_SyncCVarToAdmins isn't working properly. Although it suggests that it only synchronizes server settings with RCON clients, it looks like it's still sending updates to clients without RCON access too. Right now, I don't know why this is happening.
akmdm akmdm Sorry for the incredibly long comment. I uploaded a bunch of new patches for to the skip correction and backup commands, based on the recent feedback I got. Testing of the latest beta build (211116-0614) showed that the previous issue that was affecting the skip correction is seemingly gone now, but people with spotty connections were generally unhappy with the skip correction causing stuttering on their ends and the fact that they had to enable cl_backupcommands to eliminate it, which also delayed their inputs by 29-58 ms. I didn't think the stuttering would be such a huge issue for so many people, but after receiving so much negative feedback on it, I decided to strip a lot of the skip correction's code, removed some of the restrictions, and reworked the backup commands feature. Here's a summary of the changes:
- I removed sv_extrapolatetics and sv_backtracethreshold and transformed sv_smoothplayers into an int CVar that can range between 0-3. This means the server can extrapolate a player's movement for up to 3 tics. It's best to keep this limit as low as possible to avoid bigger prediction, and after doing some testing with people it seems 3 tics is a good cutoff to cover most cases of player lag.
- I decided that performing a distance or sight check at the end of a backtrace really isn't too important, especially if the server doesn't extrapolate the player for so long now, so I got rid of them. Now, the position check is performed BEFORE a backtrace starts, meaning that a backtrace will always succeed once it's begun. This also means that I don't have to mess with the player's actor flags (e.g. MF_PICKUP or MF2_THRUACTORS).
- Backed out a bunch of commits or removed code wherever. Particularly removed checks that prevented a backtrace from happening if a player activated specials or took environmental damage during extrapolation, as these were too strict and causing more harm than good. It still keeps track of whatever specials a player executes during extrapolation so that during a backtrace, the player doesn't accidentally execute the same specials a second time.
- cl_backupcommands is now used exclusively for the skip correction for the purpose of making a backtrace possible even in cases where the player suffers from packet loss. This means that backup commands no longer delay a player's input or increase their ping, but this still doesn't eliminate the extra outbound net traffic required to send them. However, I suspect people were more concerned about the delays than the extra bandwidth consumption, and I don't think there's any other reliable option for mitigating packet loss. Sending backup commands is still better than nothing, and people will need to be aware of this CVar's existence.
- The server now cycles through a player's weapon sprites during a backtrace, but not during extrapolation anymore. For all intents and purposes, extrapolating weapon sprites isn't the goal of the skip correction more than it is to smooth the player's movement. This change improves the firing and weapon switching for lagging players, especially when using cl_backupcommands.
Overall, the skip correction has become a lot more simple than it was before. I don't think I can change anything else with the skip correction than where it is now, but at least it works while still being lenient towards lagging players, whereas before it was a lot more punishing. I already tested this with more than a dozen other people on my server for a few hours and they confirmed that the skip correction felt better for them now. I didn't have people complaining about stuttering anymore once they enabled cl_backupcommands. At this point, and because it's also an opt-in feature, I'd leave the skip correction where it is and move on.
Thanks for the detailed explanation and the new patches!
A quick comment on 10697: A the start of SERVER_HandleSkipCorrection there is // AK Only run the skip correction on players that are alive. if (( playersulClient.mo == NULL ) || ( pClient->lLastMoveTickProcess == gametic )) return; It looks like the comment is a copy and paste leftover from the old version, since the if block seems to be checking something completely different from what the comment suggests.
akmdm Thanks for reviewing the patches so early, and thanks for catching that misleading comment in 10697. I corrected this and uploaded a new patch, 10697_v2.patch.
Thanks! I now added all of the patches except for 10708. This one has an outdated comment too: // AK Only do this when we're not backtracing this player's movement. if (( pCmd->ucmd.buttons & BT_ATTACK ) && ( SERVER_IsExtrapolatingPlayer( ulClient ) == false )) in ClientMoveCommand::process. Also the commit message doesn't seem to cover the backtraceRestoreD related part of this patch. Other than that, this patch looks fine.
akmdm Thanks for reviewing and adding all the patches! I quickly touched up 10708 and re-uploaded it as 10708_v2.patch.
akmdm akmdm I have a couple more refinement patches for the skip correction. Last week's testing showed that sv_smoothplayers and cl_backupcommands have improved a bit, but there's still a couple of issues that had to be fixed. For lagging players, weapon desyncs were apparent, weapon shots felt delayed or wouldn't register at all, and movement still felt jittery for them.
These patches should fix (and at least mitigate the weapon shot delays) the issues above (I'm crossing my fingers that these are the last few issues I need to address with the skip correction), and I also decided to scrap a few other things that I think aren't so important to keep. I was only able to test these changes by myself with a simulated bad connection and hadn't gotten a chance to test them in an actual multiplayer game, so I'd suggest releasing another beta once these are in.
P.S. After getting feedback from some people on this, and also asking them about what they thought about the net traffic increase due to the backup commands, I'm thinking about changing the default value of cl_backupcommands to 1, on the notion that some people believed that Zandronum was broken because they stuttered while the skip correction was on. At the very least, I would also add some safe guard to ensure that players who aren't suffering from ping spikes or packet loss don't send backup commands and waste their own bandwidth unnecessarily.
Thanks for the further tweaking the skip correction! Quick question on 10725: Did you accidentally remove the ( NETWORK_GetState( ) == NETSTATE_SERVER )check? I don't see any reason why anyone but the server should enter that block. I guess nothing bad will happen with what is currently inside this block, but I think the server check would make the code more readable and less error prone.
akmdm Reply To torr_samaho
Thanks for the further tweaking the skip correction! Quick question on 10725: Did you accidentally remove the ( NETWORK_GetState( ) == NETSTATE_SERVER )check? I don't see any reason why anyone but the server should enter that block. I guess nothing bad will happen with what is currently inside this block, but I think the server check would make the code more readable and less error prone.
I removed the ( NETWORK_GetState( ) == NETSTATE_SERVER ) check since SERVER_IsBacktracingPlayer and SERVER_IsExtrapolatingPlayer already return false if they're not executed by the server, so although anyone can enter that block, nothing actually happens unless it's the server who enters it.
EDIT: Readability does sound like an issue, so I restored the check.
akmdm)
Thanks for the updates patch! I added this and all the remaining patches from this batch.
akmdm Update: despite adding all those tweaks to the skip correction which seemed to improve it further, I'm still getting complaints about more problems with it. Most people agree that sv_smoothplayers being disabled is overall better.
EDIT: The whole mechanism of the feature could be overhauled, but right now it doesn't seem worth doing and will only delay 3.1 even further. It's a question of whether this feature should be forcibly disabled in the release build or backed out entirely until it can be addressed again post-3.1.
EDIT: I'll upload the only fix I have for the skip correction, but I think that's about all I can do.
EDIT 2: I thought about it more, and I might have a theory on where the issues with the skip correction might still be occurring. I think at best I can mitigate the issues, but not fix them completely. In any case, since the changes are all serversided and minor, I can just keep building new betas for server hosts to try it.
akmdm)
akmdm akmdm akmdm akmdm akmdm akmdm Update: I decided to change the methodology of the skip correction and simplify it even further. Surprisingly, the changes work just the same, if not better than before! I've been sending server builds containing these fixes to a few hosts of MM8BDM and so far I haven't been getting reports of people complaining about stuttering, dropped shots, delays, etc. There are still a couple of issues regarding "wall jumping and short jumping" not being 100% responsive when the skip correction. However, considering that MM8BDM heavily utilizes ACS and/or DECORATE for many of its mechanics, it seems obvious why this might still be an issue. I'm still working on a solution to those issues, but at least I have a few patches ready for review.
akmdm akmdm akmdm akmdm I removed the .zip for now, since I want to make more improvements. I will re-submit the patches when I think they've been tested thoroughly and confident that they work without issues.
akmdm akmdm I'm going to upload the last few patches I have for improving the skip correction (2021 12 04). Half of them are simple tweaks, but 10734.patch are 10735.patch are the big ones.
- For 10734, I simplified the skip correction so that in case the server extrapolated the player's movement for "n" number of tics, instead of determining which commands are "late" or "on time" based on their associated client gametics, the server will accept whichever "n" number of move commands arrive first as the "late" commands, regardless of whether they're actually late or not. This also reduces the occurrences of dropped commands, such as if an "on time" move command arrives first before the "late" move commands do.
- For 10735, I refined parseBufferedCommand so that it won't reject commands that arrive too late, but only rejects commands that the server already received from the client. Each client has two ring buffers containing the last 15 gametics of move and weapon select commands which the server uses to determine if the newly parsed command is actually a duplicate. Particularly useful if the client is sending backup commands, but also reduces the occurrences of dropped commands.
While it might seem counter-intuitive to remove the checks that drop late commands, I realize that in 3.0, commands arriving too late and being processed in the wrong order wasn't really an issue for lagging players anyways. All things considered, it's better to process the commands in the wrong order than to outright drop them and screw with the client.
EDIT: I've been sending out modified builds of 3.1-alpha-r211128-1928 containing these changes to server hosts throughout the week in order to get enough feedback so that I'm confident that the changes actually improve the skip correction even further before submitting them. For servers that used these modified builds, I didn't really hear any issues about people complaining about stutters. There was the case in MM8BDM that players couldn't "wall jump" or "short jump" in some cases while the skip correction was on. However, that mod does a lot of complicated things with the movement that right now, I can't cover with the skip correction. Considering that this is an opt-in feature, hosts don't have to use it if they don't want to.
akmdm)
Thanks for the new patches! A quick comment on 10735: in server_ParseBufferedCommand you add the loop for ( unsigned int i = 0; i < 15; i++ ). Looks like you accidentally hard coded 15 there instead of using MAX_RECENT_COMMANDS.
akmdm Reply To torr_samaho
Thanks for the new patches! A quick comment on 10735: in server_ParseBufferedCommand you add the loop for ( unsigned int i = 0; i < 15; i++ ). Looks like you accidentally hard coded 15 there instead of using MAX_RECENT_COMMANDS.
Oh shoot, I forgot to update that line when I added the MAX_RECENT_COMMANDS constant to replace the magic numbers. I can update this patch quickly.
Thanks! I added 10735_v2 and the remaining new patches.
akmdm Awesome, thanks a lot!
Fork
These patches add the skip correction feature (i.e. "sv_smoothplayers") which servers can enable to smooth the movement of players who are lagging, either due to packet loss or ping spikes, without the addition of any artificial delays. For now, I have this CVar set to false by default so server hosts will need to enable it themselves. When this feature is off, the game should behave like it does in 3.0, or 3.1 without any movement regulation.
A little bit of closed testing seems to show some improvements and a step in the right direction, according to some people. It would still be great to test this feature out with a greater number of players in order to gain more constructive feedback and see if further improvements need to be made.