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.

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.

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.

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?

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).

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).

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).

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.

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

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.

I actually liked that forced change map behaviour...

Anyway, this change affected DOD:S game, because it have a bug inside game.
https://forums.alliedmods.net/showpost.php?p=2831497&postcount=4

*edit
Update, made plugin
[DOD:S] Fix ChangeLevel() Spam