Quick question on 10567: Did you test that the "-1" part of the following construction really works? GAMEMODE_e newmode = static_cast<GAMEMODE_e>( GetValueGAMEMODE_e( name )); // AK No need to change the game mode if it's invalid or if we're already playing it. if (( newmode == -1 ) || ( newmode == oldmode )) return 0; While GetValueGAMEMODE_e can return an int with value -1, -1 does not seem to be a valid value for the type GAMEMODE_e. So I'm not sure what static_cast<GAMEMODE_e> will do with such a -1. To be on the save side, you could check if GetValueGAMEMODE_e returned -1. If that's the case, return otherwise cast the returned value to GAMEMODE_e.
akmdm Reply To torr_samaho
Quick question on 10567: Did you test that the "-1" part of the following construction really works? GAMEMODE_e newmode = static_cast<GAMEMODE_e>( GetValueGAMEMODE_e( name )); // AK No need to change the game mode if it's invalid or if we're already playing it. if (( newmode == -1 ) || ( newmode == oldmode )) return 0; While GetValueGAMEMODE_e can return an int with value -1, -1 does not seem to be a valid value for the type GAMEMODE_e. So I'm not sure what static_cast<GAMEMODE_e> will do with such a -1. To be on the save side, you could check if GetValueGAMEMODE_e returned -1. If that's the case, return otherwise cast the returned value to GAMEMODE_e.
I did test if it would work (tried passing fake or invalid game mode names into SetCurrentGamemode) and the return value would always be '0'. If I passed a valid game mode name into the function which wasn't already the current game mode, then it would return '1'. In any case, I re-uploaded 10567 which guarantees that the logic above always works without creating undefined behaviour.
akmdm I made some improvements to the "GameSettings" and "LockedGameSettings" features in GAMEMODE (10569-10572). Most notably, I added "DefaultGameSettings" and "DefaultLockedGameSettings" blocks which enables/disables flags across all game modes. This is useful for mods that are played on more than one game mode and require the same flags to be on or off universally.
Thanks for the new patches and the updates to them! They should be all in our main repo now.
akmdm I added a bunch of new patches (inside "Patches - 2021 10 03.zip"), based on some of the testing and feedback we got last week. Most notably, I fixed serverinfo CVars that were entered on the command line not being applied properly, and fixed the scoreboard not displaying all the columns properly.
Thanks for the new patches! I added all except for 10578 and 10580, as we just discussed on IRC.
Thanks for the updated patches! I added both of them.
akmdm I uploaded a bunch of new patches from this week, addressing some of the issues encountered during testing. I will have more patches to upload later today.
EDIT: 10588 doesn't respect lms_spectatorchat though it's acceptable anyways. The private message was intended for the server host, not specifically for the RCON client. The RCON client only receives these messages so they can communicate with players who privately message the server more effectively, and the players usually don't know who has RCON access.
EDIT 2: There's been some reports about people getting low FPS in 3.1. I think I recall that on my old PC, I got lower FPS in some cases than I normally would in 3.0. I'm a little suspicious that this commit: http://hg.osdn.net/view/zandronum/zandronum-stable/rev/ea2f3cf2ebc6 might be the cause, but would like to check it out more just to be sure.
akmdm)
akmdm From the discussion we had during our last meeting, I touched up on the private chat mode so that spectators cannot send private messages to the server while the chat is restricted. It doesn't seem important to restrict the private message when the server is sending to a spectator, though.
Thanks! I added the new patches.
akmdm Due to not many people understanding how the private chat mode works, I added a message that tells the user how to change the player they want to send a private message to, if they're in the middle of typing one. This only appears if there's at least one other valid player to send the message to (i.e. no bots or the server).
Thanks for the new patches! I checked and added all four of them.
akmdm I just got a bug report from someone saying that they had issues logging into RCON remotely in 211024-2022:
https://media.discordapp.net/attachments/879280566879535104/902508233107841094/unknown.png
I did a bisect and https://osdn.net/projects/zandronum/scm/hg/zandronum-stable/commits/11401a91efa1429d815b0d3bc0fdc93c9dae1bb4 is the culprit. Apparently, adding CLC_WEAPONSELECTBACKUP (52) is problematic because CLRC_BEGINCONNECTION is also equal to 52, and now NUM_CLIENT_COMMANDS is equal to 53. This creates a conflict in SERVER_DetermineConnectionType, on line 1696 in sv_main.cpp: if (( lCommand >= CLC_USERINFO ) && ( lCommand < NUM_CLIENT_COMMANDS )) return;
As a very temporary solution, I tried reducing the number of client commands by merging the client commands CLC_VOTEYES and CLC_VOTENO into a single command. This means the client now has to send an extra byte telling the server whether they voted yes or no on the current vote, but these commands are sent rarely enough that it's almost negligible. At least, it drops NUM_CLIENT_COMMANDS back to 52 and fixes the issue for now. However, it's not a permanent solution in the long run.
Changing CLRC_BEGINCONNECTION to a higher number isn't feasible at this time, and we'd also risk breaking compatibility with lots of RCON utilities like Doomseeker's or Doom Explorer's unless they somehow updated theirs immediately.
EDIT: I uploaded 10630.patch which combines the two vote commands together. If it gets pushed, I suggest that we release another beta right away just because I think this a major issue that needs to be addressed. I can add an update to the announcement I posted on the forums to indicate this.
akmdm)
10630 is a good solution for this problem. I agree that the increased net traffic for voting is negligible, pushed the patch to our repo and uploaded new win32 binaries to the mediafire folder. Feel free to release a new beta with this. Thanks for fixing this so quickly!
akmdm Reply To torr_samaho
10630 is a good solution for this problem. I agree that the increased net traffic for voting is negligible, pushed the patch to our repo and uploaded new win32 binaries to the mediafire folder. Feel free to release a new beta with this. Thanks for fixing this so quickly!
Thanks for sending the new builds! I'll upload them when I come home tonight.
akmdm akmdm I added a bunch of new patches (inside "Patches - 2021 10 31.zip") from this week.
akmdm akmdm akmdm Thanks for the new stack of patches! I reviewed and added all of them.
akmdm I thought about 10647 again, and I think it's still reasonable to allow modders to revive dead spectators if the game is waiting for players or in the countdown sequence should any exist in these states (e.g. setting a player to dead spectators in a 1v1 deathmatch game, then reviving them shortly after). Dead spectators aren't meant to exist in these states, so reviving them before the game starts still follows the original approach. This function still doesn't work during the results sequence.
I updated the patch as 10647_v2 and uploaded a new patch: 10648 which fixes a "connection interrupted" issue that occurred when newly connected players didn't start as spectators.
akmdm)
akmdm In addition to 10647_v2.patch and 10648.patch, I got a bunch of new patches ready. For the private chat mode, I was concerned about players on opposing teams being able to send private messages to each other and potentially cheating, so I replaced sv_noprivatechat with a new CVar sv_allowprivatechat.
If the latter is set to 2, then in-game players are only allowed to send private messages to their teammates. They will not be able to send private messages to the server either, again, to avoid potential cheating. However, true spectators are allowed to send private messages to the server or other true spectators.
akmdm On second thought, still letting true spectators be able to chat with the server while sv_allowprivatechat is set to 2 is just as bad as letting the in-game players do the same. I updated 10657 so now true spectators can only private chat with other true spectators in this case, meaning nobody can send private messages to the server.
akmdm)
Thanks for the new patches! I'm going through them and will post comments here as I go.
10657_v2: I'd say the function name CHAT_CanSendPrivateChatToTeammate is misleading, since this function is also used when ulSender and ulReceiver are no team mates, but can still return true in that case.
akmdm Thanks for reviewing and pushing many of these patches already.
Reply To torr_samaho
10657_v2: I'd say the function name CHAT_CanSendPrivateChatToTeammate is misleading, since this function is also used when ulSender and ulReceiver are no team mates, but can still return true in that case.
I uploaded 10657_v3, renaming CHAT_CanSendPrivateChatToTeammate to CHAT_CanSendPrivateMessageTo, which should more self-explanatory. Is there anything that I should know about regarding 10654 (appending limit strings on the scoreboard)?
Thanks for the updated patch! I added it.
Regarding 10654, I started to look at this, but scoreboard_AppendLimit is somewhat confusing. Its comment says // AK Checks if we can append this limit string to the previous one., but this doesn't seem to be what the function is actually doing. It only checks whether "lines" is not empty and then removes the last line from list of lines and prepends that to the FString limit.
akmdm Reply To torr_samaho
Thanks for the updated patch! I added it. Regarding 10654, I started to look at this, but scoreboard_AppendLimit is somewhat confusing. Its comment says // AK Checks if we can append this limit string to the previous one., but this doesn't seem to be what the function is actually doing. It only checks whether "lines" is not empty and then removes the last line from list of lines and prepends that to the FString limit.
Thanks for the comment. I updated 10654 to better reflect what the code actually does, "prepend" seems like a better word to choose instead of "append" in this case. I also uploaded a new patch: 10661.patch.
akmdm I uploaded a new round of patches, including a new executable icon to celebrate 3.1's eventual release.
akmdm I uploaded the updated patches, as well as a couple of extra patches. I fixed the server sending SERVERINFO or SENSITIVESERVERSETTING CVars to clients who don't have RCON access and implemented sector reconciliation with the backtrace.
For implementing the sector reconciliation, because the unlagged only stores up to 35 tics of position data in the past, I decided that a backtrace should also not be performed if the player was extrapolated 35 tics ago or more, so this way it won't get messed up. Furthermore, reconciling the sectors means that I don't have to save the player's floorsector or floor height (i.e. floorHeight = player->mo->z - pFloorSector->floorplane.ZatPoint( player->mo->x, player->mo->y) anymore, so that simplifies the code a bit. :)
I tried finding Medicris, but unfortunately it doesn't look like he's active anywhere (the last message he posted on the Zandronum forums was over three years ago), so I made sure to give him proper credit in zandronum-history.txt and the commit log message. It looks like his icon (or something very similar) is already used as an emoji on the official Zandronum Discord server, so maybe we're okay to use it here too?
If all six patches get pushed into the main repo, then I'd say we're ready to release the next beta. Thanks again, and have a great week!
EDIT: I added another patch, 10687, which allows SetPlayerClass to assign random classes to players.
akmdm)
Thanks for the new patches!
Quick question on 10681: Could the problem of AdminClientIterator be that ClientIterator::isCurrentValid is not virtual?
akmdm Reply To torr_samaho
Thanks for the new patches! Quick question on 10681: Could the problem of AdminClientIterator be that ClientIterator::isCurrentValid is not virtual?
I didn't check to see if making it virtual will fix the problem. Unfortunately I'm not home right now, so I can't test it out.
No problem. It's not urgent.
Question on 10684: server_PerformBacktrace seems to change pClient->OldData (since it uses ++pClient->OldData->ulSavedGametic). Wouldn't it be safer to replace unlaggedIndex = ++pClient->OldData->ulSavedGametic % UNLAGGEDTICS; with ++unlaggedIndex;? I think this would also be easier to read.
akmdm Reply To torr_samaho
No problem. It's not urgent. Question on 10684: server_PerformBacktrace seems to change pClient->OldData (since it uses ++pClient->OldData->ulSavedGametic). Wouldn't it be safer to replace unlaggedIndex = ++pClient->OldData->ulSavedGametic % UNLAGGEDTICS; with ++unlaggedIndex;? I think this would also be easier to read.
I think so too. The modulus operation should still be kept so that the index can't go out of bounds.
Yeah, right. The modulus needs to be kept, so I guess you wouldn't use ++, but unlaggedIndex = (unlaggedIndex +1) % UNLAGGEDTICS;.
akmdm If it's okay with you, would you be able to amend that change into 10684?
Sorry, while it would be no problem to amend the patch, I won't have time for any testing and don't want to push completely untested code to our repo.
akmdm That's perfectly fine. I'll update the patches whenever I can and re-upload them when ready.
akmdm Turning ClientIterator::isCurrentValid into a virtual function didn't solve the problem. Could it have something to do with the constructors of both classes, despite ClientIterator's constructor consisting of optional parameters?
akmdm)
akmdm Right, so when a AdminClientIterator object gets created, ClientIterator::ClientIterator executes incremntCurrentTillValid which then executes ClientIterator::isCurrentValid, not AdminClientIterator::isCurrentValid, because the derived object hasn't been created yet at this point. I don't I can fix this problem without compromising ClientIterator, creating a default constructor for the derived class won't work because the base class's members are private. But is it really important? All AdminClientIterator does extra is check if SERVER_GetClient( **this )->bRCONAccess is true and is only used in SERVERCOMMANDS_SyncCVarToAdmins.
akmdm I uploaded the updated patches (except 10681 and 10682). I had to make another small change with 10684 so that a player's z-position is properly corrected after a backtrace is performed. I also decided to reuse 10679 so that the backtrace still fails if a player activated any specials during extrapolation, but removed the "tricky" part out. I won't be able to update these patches again today, unfortunately.
I don't I can fix this problem without compromising ClientIterator, creating a default constructor for the derived class won't work because the base class's members are private. But is it really important?
No, the important thing was to understand why this doesn't work. So the actual increment works when the virtual is added, but the initial element is not valid because of the problem you found. One could make ClientIterator::incremntCurrentTillValid protected and also call it in the derived class constructor, but we can just use your existing patch as is. I'll push that.
The patches look all good. I added all of them to our repo. Feel free to make a new beta release if everything is in now that you'd like to have.
akmdm I uploaded another round of patches. I got some feedback from some competitive players who expressed their dislike for the mouse movement in 3.1 even after changing the value of smooth_mouse. Therefore, I added a new CVar cl_useskulltagmouse which restores the mouse movement as it felt in 3.0 and earlier. After testing a build with this option enabled (both offline and online), they were satisfied with the change. I reckon there are more people out there who might prefer the old mouse, so it would be safe to have the option around. The rest of the patches are general bug fixes and other minor additions.
Thanks for the new patches! Quick comment on 10710: The two added !cl_useskulltagmouse in d_main.cpp are in ZDoom code, aren't they? If so, they are still missing a comment with your initials.
akmdm Right, I should've added comments in those places too. I uploaded 10710_v2.patch to correct this.
akmdm I'm going to close this ticket because it's getting too long and there's too many attachments to keep track of. Any new patches will be uploaded on a new ticket.
Fork
As we're set to release another official beta of 3.1, this is where any fixes for regression bugs or other issues will go if they're found.