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

Enable a button to allow players to change teams #872

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

Zanieon
Copy link
Contributor

@Zanieon Zanieon commented Sep 7, 2024

This a feature not related to vanilla behavior but rather QoL in regards to the very nature of the Northstar servers, as the only way of changing teams is using mods that gives these functionalities to the server, when it actually should be bundled in Northstar itself already, imo.

image

@Zanieon Zanieon added the needs testing Changes from the PR still need to be tested label Sep 7, 2024
@github-actions github-actions bot added the needs code review Changes from PR still need to be reviewed in code label Sep 7, 2024
@Zanieon
Copy link
Contributor Author

Zanieon commented Sep 7, 2024

@F1F7Y This one is specially for you due to your Server Utilities :D

Copy link
Contributor

@ASpoonPlaysGames ASpoonPlaysGames left a comment

Choose a reason for hiding this comment

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

If you made the convar replicated, you could enable and disable the UI button based on if the server has the convar enabled. I think we should try to avoid using SendHudMessage for most things if possible

@Zanieon
Copy link
Contributor Author

Zanieon commented Sep 7, 2024

If you made the convar replicated, you could enable and disable the UI button based on if the server has the convar enabled. I think we should try to avoid using SendHudMessage for most things if possible

Alright, convar replicated and button visibility bound to it, now in regards to SendHudMessage, those i'll have to probably localize them eventually because would be quite frustrating to players trying to click the button and no info will ever pop in for those two specific cases they could appear.

Copy link
Member

@F1F7Y F1F7Y left a comment

Choose a reason for hiding this comment

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

Train WiFi review

Northstar.Client/mod/scripts/vscripts/ui/menu_ingame.nut Outdated Show resolved Hide resolved
Northstar.Client/mod/scripts/vscripts/ui/menu_ingame.nut Outdated Show resolved Hide resolved
{
"Name": "ns_allow_team_change",
"DefaultValue": "1",
"Flags": "REPLICATED"
Copy link
Member

Choose a reason for hiding this comment

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

I know spoon wanted this ( and I agree with it ) but would be great to know version compat of this change before release

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be up to @GeckoEidechse when he does a release with this change right?

Copy link
Member

Choose a reason for hiding this comment

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

I don't have the capacity to test that atm. If someone could take care of that, that would great.
It can be tested without making a release.

Basically testing should cover

  • New server <-> Old Client
  • Old server <-> New Client

And on that note

  • New server with old client not working is a non-issue as we can just version gate
  • Old server with new client would suck as servers don't update that fast. We could still do it though, just need to ping server hosts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New Clients to Old Servers won't change anything, the server will recieve the command from the button but ignore it.

New Servers to Old Clients will also not change anything substantial besides old clients having to type "changeteam" in console in order to swap.

Copy link
Contributor

@ASpoonPlaysGames ASpoonPlaysGames left a comment

Choose a reason for hiding this comment

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

Code looks good as far as I'm concerned

@Alystrasz
Copy link
Contributor

In regards to SendHudMessage, those i'll have to probably localize them eventually because would be quite frustrating to players trying to click the button and no info will ever pop in for those two specific cases they could appear.

I agree information should be transmitted to player somehow.
A less invasive way could be to print warning message in chat (using another colour than white).

Copy link
Member

@F1F7Y F1F7Y left a comment

Choose a reason for hiding this comment

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

I hate the thread thing but if it works it works

@Zanieon
Copy link
Contributor Author

Zanieon commented Sep 13, 2024

I hate the thread thing but if it works it works

It's actually not really necessary, but talking with @ASpoonPlaysGames on Discord VC, we agreed that having that threading is good because then Clients updates realtime in case server enables/disables the ability to change teams.

@F1F7Y
Copy link
Member

F1F7Y commented Sep 13, 2024

I fully agree the button should be up to date, I'm just questioning whether a while true loop is the best way to do it.

@ASpoonPlaysGames
Copy link
Contributor

I fully agree the button should be up to date, I'm just questioning whether a while true loop is the best way to do it.

I'd say that if we can get a convar change callback to handle it then that would be ideal but I dont think we expose that sort of thing to squirrel?

@Zanieon
Copy link
Contributor Author

Zanieon commented Sep 13, 2024

I'd say that if we can get a convar change callback to handle it then that would be ideal but I dont think we expose that sort of thing to squirrel?

There's callbacks for when networked variables changes in Squirrel, but there's none for convars afaik.

@Zanieon Zanieon added almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge and removed needs code review Changes from PR still need to be reviewed in code labels Sep 13, 2024
@Gazyi
Copy link

Gazyi commented Oct 14, 2024

Changing team should kill player and Auto-Titan in playing state.

@Zanieon
Copy link
Contributor Author

Zanieon commented Oct 23, 2024

Changing team should kill player and Auto-Titan in playing state.

Why?

@Gazyi
Copy link

Gazyi commented Oct 23, 2024

Why?

For example, you can just switch team and kill old teammates/steal flag/cap points nearby.
Also, that's how it works in other Source games.

@Zanieon
Copy link
Contributor Author

Zanieon commented Oct 24, 2024

Valve games don't have Squirrel callbacks for changing teams, so it's easier to just kill the player to fix those potential issues.

FSU existed for a long time now and nobody ever complained about its method of switching teams without killing players, for CTF i can just add a team change callback to force flag drop in case player is carrying it.

@ASpoonPlaysGames
Copy link
Contributor

Fairly certain there is already a callback for changing teams it just never gets called? I remember seeing the killfeed code has stuff for team change notifications and somewhere up the callstack for that it comes from a callback?

@Zanieon
Copy link
Contributor Author

Zanieon commented Oct 26, 2024

Fairly certain there is already a callback for changing teams it just never gets called? I remember seeing the killfeed code has stuff for team change notifications and somewhere up the callstack for that it comes from a callback?

That's what i mean, adding that callback for special logic in such case since it is already used to notify players when someone changes teams which you are probably remembering from #789

Copy link
Contributor

@Alystrasz Alystrasz left a comment

Choose a reason for hiding this comment

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

Code looks good, team change is now possible in-game; it's impossible to spam team change more than once every 5 seconds.

@Alystrasz Alystrasz removed the needs testing Changes from the PR still need to be tested label Nov 2, 2024
@Zanieon Zanieon added READY TO MERGE This mergeable right now and removed almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge labels Nov 3, 2024
@Bobbyperson
Copy link
Contributor

Might I suggest that this convar be set to zero automatically in some cases? This would mess with infection and the hidden pretty badly.

@Zanieon
Copy link
Contributor Author

Zanieon commented Nov 5, 2024

Might I suggest that this convar be set to zero automatically in some cases? This would mess with infection and the hidden pretty badly.

If the server is fixed at those gamemodes then all you need to do is just add the convar to the launch arguments, disabling it, the other method is make the gamemode initialization function disable it as well.

@Bobbyperson
Copy link
Contributor

True, some people might not be running dedicated servers though so it might be worthwhile adding it to those functions like you said.

@Zanieon
Copy link
Contributor Author

Zanieon commented Nov 5, 2024

True, some people might not be running dedicated servers though so it might be worthwhile adding it to those functions like you said.

Alright i'll probably make a proper toggle function instead of fiddling with the convar for gamemodes that does custom team switching.

@Zanieon Zanieon added waiting on changes by author Waiting on PR author to implement the suggested changes almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge and removed READY TO MERGE This mergeable right now labels Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge waiting on changes by author Waiting on PR author to implement the suggested changes
Projects
Status: No status
Status: Zanieon Doing
Development

Successfully merging this pull request may close these issues.

7 participants