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

Server cvar message constantly showing up in chat #1632

Open
mexikoedi opened this issue Sep 25, 2024 · 10 comments
Open

Server cvar message constantly showing up in chat #1632

mexikoedi opened this issue Sep 25, 2024 · 10 comments
Assignees
Milestone

Comments

@mexikoedi
Copy link
Contributor

Your version of TTT2 (mandatory)

  • TTT2 v0.14.0b Workshop version from Steam

Describe the bug (mandatory)

The following message shows up in the chat each map on the first round: "Server cvar 'ttt2_enable_map_prefix_gm' changed to 0".

To reproduce

Steps to reproduce the behaviour:

  1. Go into a round
  2. Start the round (maybe change the map a couple of times)
  3. See described issue

Expected behaviour

This message shouldn't appear each time in the chat.

Context (please provide as much as you can)

@TimGoll TimGoll added the type/bug Something isn't working label Sep 25, 2024
@TimGoll TimGoll added this to the v0.14.1b milestone Sep 25, 2024
@TimGoll TimGoll self-assigned this Sep 25, 2024
@Histalek
Copy link
Member

I assume you have set ttt2_enable_map_prefix_gm to 0 manually once and this issue has occurred since then?

@TimGoll
Copy link
Member

TimGoll commented Sep 25, 2024

I know why this happens. I dynamically create the convar AFTER the file loaded, maybe a tenth of a second later or so. It then detects this as a SetConVar instead of a CreateConVar

I used this because I thought that it is a nice simplification here. But I have overseen the chat massage. Checking if the convar already exists will resolve this issue

@Histalek
Copy link
Member

I know why this happens. I dynamically create the convar AFTER the file loaded, maybe a tenth of a second later or so. It then detects this as a SetConVar instead of a CreateConVar

I used this because I thought that it is a nice simplification here. But I have overseen the chat massage. Checking if the convar already exists will resolve this issue

Are you sure this isn't happening because of the cvars.AddChangeCallback call here ?

Besides that i think we should not create the _gm convar dynamically anyway as two gm maps are included with the gmod server installation and we should expect them to be there, right?

@TimGoll
Copy link
Member

TimGoll commented Sep 25, 2024

Are you sure this isn't happening because of the cvars.AddChangeCallback call here ?

Setting a global bool doesn't trigger the convar change message

Besides that i think we should not create the _gm convar dynamically anyway as two gm maps are included with the gmod server installation and we should expect them to be there, right?

I create this convar dynamically for every prefix, gm, css, ttt, de, etc. By default all of them are enabled and you can disable the maps with that prefix in the UI. If I'd remove that convar, to which group the gm_ maps should be moved? And why should the gm maps be handled differently than the ttt, css etc maps?

I'm asking because I'm not sure if you misunderstood the reason of the convar or if we're talking about something completely different here.

@Histalek
Copy link
Member

Are you sure this isn't happening because of the cvars.AddChangeCallback call here ?

Setting a global bool doesn't trigger the convar change message

Hmm but what is triggering this then? CreateConVar should never change an already existing convar and only return it, right?

Besides that i think we should not create the _gm convar dynamically anyway as two gm maps are included with the gmod server installation and we should expect them to be there, right?

I create this convar dynamically for every prefix, gm, css, ttt, de, etc. By default all of them are enabled and you can disable the maps with that prefix in the UI. If I'd remove that convar, to which group the gm_ maps should be moved? And why should the gm maps be handled differently than the ttt, css etc maps?

My bad for the wording. We do create the _de _cs and _test convars on file load and disable them. Under the assumption that this only happens for convars that are created dynamically only, we could fix the issue at least for the _gm convar by also creating it on file load.
Given that this is the most likely case someone will run into this issue, i feel like this would be a reasonable approach

@TimGoll
Copy link
Member

TimGoll commented Sep 25, 2024

Hmm but what is triggering this then? CreateConVar should never change an already existing convar and only return it, right?

I have to test this, but my guess is that using CreateConVar at runtime instead of at file load causes this to happen.

My bad for the wording. We do create the _de _cs and _test convars on file load and disable them. Under the assumption that this only happens for convars that are created dynamically only, we could fix the issue at least for the _gm convar by also creating it on file load.
Given that this is the most likely case someone will run into this issue, i feel like this would be a reasonable approach

Ah, I get it now. I disable them by default because I assumed no TTT player actually wants to play a GM map. Changing the default might not fix it, if the player then manually sets it to disabled. I have to test this

@mexikoedi
Copy link
Contributor Author

I assume you have set ttt2_enable_map_prefix_gm to 0 manually once and this issue has occurred since then?

I think I have never set this manually.

@TimGoll
Copy link
Member

TimGoll commented Sep 25, 2024

I assume you have set ttt2_enable_map_prefix_gm to 0 manually once and this issue has occurred since then?

I think I have never set this manually.

Yes, my bad, Gmod is set to false by default as Histalek noted

@TimGoll
Copy link
Member

TimGoll commented Sep 28, 2024

okay, aparantly that isn't such an easy fix. Because I do not set this convar when the files are loaded, it is announced to chat. There is no way around it as far as I can see.

But this poses the general question: Should convar changes be announced in chat at all?

@mexikoedi
Copy link
Contributor Author

okay, aparantly that isn't such an easy fix. Because I do not set this convar when the files are loaded, it is announced to chat. There is no way around it as far as I can see.

But this poses the general question: Should convar changes be announced in chat at all?

I think not. Feels weird if everyone sees it in the chat.

@Histalek Histalek removed the type/bug Something isn't working label Oct 2, 2024
Histalek added a commit that referenced this issue Nov 15, 2024
* 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.
Even if either or both of them are not currently installed on the server
creating these unconditionally would allow an admin to configure their
behaviour in that case.

Creating convars dynamically if we find more map prefixes is still
done.

* 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 added a commit that referenced this issue Nov 15, 2024
* 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.
Even if either or both of them are not currently installed on the server
creating these unconditionally would allow an admin to configure their
behaviour in that case.

Creating convars dynamically if we find more map prefixes is still
done.

* 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 added a commit that referenced this issue Nov 15, 2024
* 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 added a commit that referenced this issue Nov 15, 2024
#1669)

* 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
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

No branches or pull requests

3 participants