ec- / Quake3e

Improved Quake III Arena engine
GNU General Public License v2.0
1.21k stars 154 forks source link

Don't change serverid during map restart #283

Closed Chomenor closed 5 months ago

Chomenor commented 5 months ago

Since the SV_ClientEnterWorld call for loading clients was removed in ed4af16763684a9b492c4c6737833531b43963e2, clients will hang if a map restart occurs while they are loading the map.

This is because a new serverid is generated during the map restart, but the new serverid is not sent to the client because it is part of the systeminfo configstring, and configstring updates are deferred for loading (CS_PRIMED) clients until ClientEnterWorld is called. However ClientEnterWorld is never called through the normal route in SV_ExecuteClientMessage because it has checks that abort any actions for clients with an outdated serverid. As a result the client is stuck, not set to active, not receiving valid snapshots, and not receiving the updated serverid.

This change fixes this problem by removing the serverid modification during map restart. It also fixes a case where a map change AND map restart occur while a client is loading the previous map. Some extra checks are added to skip movement commands prior to the map restart, to emulate original behavior for mod compatibility purposes.

One compatibility concern with this fix is if a mod were to explicitly check the sv_serverid cvar and expect it to change after a map restart. However I think this is very unlikely.

Chomenor commented 5 months ago

I noticed that q3e doesn't set the sv_serverid cvar on the client so a mod cgame couldn't depend on that anyway, not that it was ever realistically likely.

I'm happy with how this works from the testing I've done so far. It handles map changes and map restarts in close proximity cleanly even for actively loading clients. I think this should be good to merge.

Chomenor commented 5 months ago

This should be fixed now.

Chomenor commented 5 months ago

I cleaned it up a bit. I think it should still match the original behavior.

Chomenor commented 5 months ago

I changed it to use lastUsercmd.serverTime as requested. It should function basically the same as before, just a slightly different implementation.