TTT-2 / TTT2

Trouble in Terrorist Town 2 for Garry's Mod (gmod)
https://steamcommunity.com/sharedfiles/filedetails/?id=1357204556
179 stars 77 forks source link

fix(map): Streamline behaviour for the ttt2_enable_map_prefix_ convars #1669

Closed Histalek closed 1 week ago

Histalek commented 1 month ago

Updated changes after a few iterations are now the following:

We can reasonably assume gm maps to be present on the server, the same goes for ttt and ttt2 maps.

Creating convars dynamically if we find more map prefixes is still done. And only prefixes for installed maps will be shown anyway.

We will assume people only want to run maps intended for ttt(2).

Given that the prefix selection can be modified on the same admin settings page that is (currently) the only one using these convars this seems fine.

Given that these are purely used in an admin context there is no need to announce changes to all users on the server.

The last changes especially fixes the original Issue https://github.com/TTT-2/TTT2/issues/1632

TimGoll commented 1 month ago

This only resolves the issue for the prefixes you added. If one adds a map with another prefix, the issue still persists

Histalek commented 1 month ago

This only resolves the issue for the prefixes you added. If one adds a map with another prefix, the issue still persists

i'm aware of that, if we can find a fix for all prefixes that would be nice! This would only fix the common occurrences of the issue

if we would flip all other prefixes to off by default this issue would only occur for servers that enable any other prefixes and i would guess that not many servers would want to play on non-ttt prefixed maps

TimGoll commented 1 month ago

This only resolves the issue for the prefixes you added. If one adds a map with another prefix, the issue still persists

i'm aware of that, if we can find a fix for all prefixes that would be nice! This would only fix the common occurrences of the issue

if we would flip all other prefixes to off by default this issue would only occur for servers that enable any other prefixes and i would guess that not many servers would want to play on non-ttt prefixed maps

If you are aware, then it is fine with me. I already tried for an hour or so, sadly without luck so far.

But mexikoedi and I discussed if we should maybe remove the convar change announcement to chat for all convars. That would resolve this issue as well. And announcing convar changes to chat doesn't see polished to me. We could probably make sure that it is still logged to the console or so.

Histalek commented 2 weeks ago

Reading over this again i'm somewhat inclined to only default ttt and ttt2 maps to enabled and disable the rest

Reason being that this would be the most likely setup people are running, correct?

But mexikoedi and I discussed if we should maybe remove the convar change announcement to chat for all convars. That would resolve this issue as well. And announcing convar changes to chat doesn't see polished to me. We could probably make sure that it is still logged to the console or so.

I don't think removing this for all convars would be correct, but we could just drop FCVAR_NOTIFY for the dynamically created convars as a workaround

EDIT: i pushed these changes to the branch

Histalek commented 1 week ago

cleaned up the commit and wrote changelog entries

Histalek commented 1 week ago

the last force-push clarified a misconception of mine in the commit message.

that being: only prefixes for installed maps will be shown in the maps settings page. E.g. the option to disable/enable ttt_ maps will only be shown if ttt maps are installed (the convar data is kept regardless)