Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Nov 15, 2024

Conversation

Histalek
Copy link
Member

@Histalek Histalek commented Oct 20, 2024

Updated changes after a few iterations are now the following:

  • Unconditionally create gm, ttt and ttt2 prefixes

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.

  • Disable all map prefixes but ttt and ttt2 by default

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.

  • Drop FCVAR_NOTIFY for all map_prefix_enabled convars

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 #1632

@Histalek Histalek requested a review from TimGoll October 20, 2024 10:31
@Histalek Histalek self-assigned this Oct 20, 2024
@TimGoll
Copy link
Member

TimGoll commented Oct 20, 2024

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

@Histalek
Copy link
Member Author

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
Copy link
Member

TimGoll commented Oct 20, 2024

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
Copy link
Member Author

Histalek commented Nov 6, 2024

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

Copy link
Member

@TimGoll TimGoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like these changes, thank you. In my opinion this can be merged

@Histalek Histalek changed the title fix(map): Unconditionally create gm, ttt and ttt2 prefixes fix(map): Streamline behaviour for the ttt2_enable_map_prefix_ convars Nov 15, 2024
@Histalek Histalek marked this pull request as ready for review November 15, 2024 17:04
@Histalek
Copy link
Member Author

cleaned up the commit and wrote changelog entries

* Unconditionally create gm, ttt and ttt2 prefixes

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. Only prefixes for installed maps will be shown anyway.

* Disable all map prefixes but `ttt` and `ttt2` by default

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.

* Drop FCVAR_NOTIFY for all `map_prefix_enabled` convars

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 #1632
@Histalek
Copy link
Member Author

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)

@Histalek Histalek merged commit ea7eb5a into master Nov 15, 2024
4 checks passed
@Histalek Histalek deleted the fix-map-prefixes branch November 15, 2024 17:18
@Histalek Histalek added this to the v0.14.1b milestone Nov 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants