sourcemod icon indicating copy to clipboard operation
sourcemod copied to clipboard

Clear sm_nextmap so we don't get stuck in a loop

Open asherkin opened this issue 4 years ago • 8 comments

This seems to work well, but it requires a change to the nextmap plugin as well as the implementation in core - which will be a pain for users as they almost never update the base plugins on upgrade. If the old plugin is used against a newer core version the sm_nextmap cvar will never get re-populated after map change, so SM will stop controlling the map cycle.

Fixes #1429

asherkin avatar Jul 18 '21 23:07 asherkin

Got two potential solutions for the bug here (in each commit), one on the C++ side that is more generic but a slightly hairy implementation (I think only TF2 can take this path though, other games don't recover from bad map changes), or one purely in the nextmap plugin that listens to the event added for TF2 specifically. The plugin code has a nice advantage of preserving the SM-enforced map cycle, whereas the C++ impl just gives up for a map. I'm really not sure which I prefer.

asherkin avatar Jul 19 '21 21:07 asherkin

I think having a hybrid of both would be best — the changes on the C++ side are necessary since core manages sm_nextmap and there's no guarantee that the plugin will be up to date, let alone actually be installed.

nosoop avatar Jul 20 '21 03:07 nosoop

Ah, I hadn't spotted that any other plugins were using SetNextMap - yeah, probably best to take both of these then (they do work well together). The core changes will ensure nothing gets stuck, and the plugin changes will keep it on the right mapcycle if it would have gotten stuck.

asherkin avatar Jul 20 '21 20:07 asherkin

If the old plugin is used against a newer core version the sm_nextmap cvar will never get re-populated after map change, so SM will stop controlling the map cycle.

This sounds rather undesirable - no?

KyleSanderson avatar Aug 01 '21 19:08 KyleSanderson

If the old plugin is used against a newer core version the sm_nextmap cvar will never get re-populated after map change, so SM will stop controlling the map cycle.

This sounds rather undesirable - no?

I believe that comment was for the original version of the PR, which (if I remember correctly) blanked out nextmap at the start of every map change. The latest revision for core should be fine with an older version of the plugin; the effect of a failed map change will be one off-cycle map before getting back on track (with an updated plugin, a failed map change will advance to the next available map in the cycle).

nosoop avatar Aug 03 '21 03:08 nosoop

I've tested the latest commit against TF2 - including running a copy of nextmap.smx from before this change - and it should be fine, aside from the current behavior the plugin where a failed changelevel mid-game unconditionally modifies sm_nextmap (which wouldn't cause a softlock as the game only continues on the current map).

nosoop avatar Sep 03 '21 16:09 nosoop

I don't like this - I think there's a better approach.

This has messed up my bench for ages (readas: 15 years) - the reason why is we don't do any validation of the level before setting the convar. Things are OK as the engine takes over and ignores case sensitivity which is excellent as levels are delivered using netchannel at a speedy rate speed (usually 30 to 100KB/s). However, the requests that are serviced against sv_downloadurl are still in the wrong case which frequently results in a 404 situation as the requested file does not actually exist on the webserver.

What I think we should do: I think we should resolve the relative path, then verify the file exists, and then set the convar. It's going to result in a "no workshop2" support situation, but we can add support for that situation when it happens (like when workshop shipped previously).

KyleSanderson avatar Nov 02 '21 00:11 KyleSanderson

Could you clarify? There is no change in that the engine still handles checking the validity of the map and preparing resources here; I don't quite understand where sv_downloadurl ties into this.

newmap will be whatever was stored in sm_nextmap; there's no change in that behavior either, other than it being blanked out between pre-ChangeLevel and the next LevelInit.

nosoop avatar Jul 23 '22 18:07 nosoop

Yeah, boiling the ocean lead to nothing happening... this is still an improvement, feel free to merge.

KyleSanderson avatar Mar 30 '23 04:03 KyleSanderson

Sounds like this should be merged a while ago and nobody pressed da button. Handling failed mid-round mapchanges slightly wrong is better than deadlocking in a map loop and can be addressed later.

peace-maker avatar Jul 03 '23 18:07 peace-maker