Improve handling for players leaving/joining multiplayer games during votes #298

GooberRF · 2024-10-24T20:26:11Z

This PR adjusts voting in the following ways:

When all players have left the server, an active vote is now automatically canceled.
When the person who called a vote leaves the server, the vote is now automatically canceled.
Newly joined players can now participate in the vote.
Players who leave during a vote are now automatically removed from influence on the vote outcome - if they didn't vote, they're no longer considered; if they did vote, that vote is removed/ignored.

Previously, the behaviour for players who join/leave during a vote was relatively unintuitive - ie. players who joined during a vote were excluded from participating (despite receiving a reminder to do so), and a vote was still being expected from players who had already left the game.

This new approach is much more intuitive and is, I think, in line with how many players already thought the feature was working.

rafalh

Previously, the behaviour for players who join/leave during a vote was relatively unintuitive

Let me explain it.

players who joined during a vote were excluded from participating (despite receiving a reminder to do so)

In case of votekick of voteban you wouldn't want players who just joined and don't know the reason for the vote to decide. In case of votemap I guess it makes less sense. Maybe it should be configured per vote?

and a vote was still being expected from players who had already left the game

I think it's not true. In on_player_leave there is a call remaining_players.erase(player);, so how is it expected from players leaving to vote?

So the main change in this PR is to allow new players to vote. You could have added on_player_join callback and it would probably be a simpler change...

Players who leave during a vote are now automatically removed from influence on the vote outcome - if they didn't vote, they're no longer considered; if they did vote, that vote is removed/ignored.

There's no change here. It was already like this... Have you read the code?

When the person who called a vote leaves the server, the vote is now automatically canceled.

I'm not sure if it makes sense. Someone could troll people to vote and then leave in the last second. If someone creates a vote to switch to map X, everybody accept, and then the initial vote caller leaves (or lost connection), why would we want the vote to be ended, especially if most people were in favor? Should people restart the vote? After vote already started there are more people involved already and canceling it wastes all the votes. What do you think?

rafalh · 2024-11-30T22:51:12Z

game_patch/multi/server.cpp

    return true;
 }

+std::set<rf::Player*> get_current_player_list(bool include_browsers)


what's the point of include_browsers param if it's always set to false.
Also why would I want to use that function instead of rf::player_list if I didnt want to exclude browsers?
Also the word "current" is not really indicating anything.
Why is this function in another file? Is it going to be used elsewhere? I would be looking for a function named like this in player module, not here.
I think it could be moved to the votes.cpp and named like get_players_allowed_to_vote maybe?

I designed the function to be flexible for use in the future in cases where browsers perhaps should be included. I don't have a use case for that currently, but the added overhead is worth the flexibility in my mind. Happy to adjust the PR if you disagree with that, though.

"current" doesn't serve a purpose, good point. I'll adjust that.

I use this function in other places in my code - the match mode ready system for example - and perhaps other places in the future, because of that I put it in server.cpp because that seemed to be where it would best fit.

Why is it a set? Why should browsers ever be allowed to vote? I do not like it.

Browsers should never be allowed to vote - this function isn't intended to be used only as a component of voting.

As I said I don't see why would anybody call get_player_list(true) if they can just use rf::player_list directly. The only thing this function adds to the table is filtering out browsers. So it should be named accordingly and probably placed in a different place. For now the simplest way forward is to put it into votes.cpp module as a local helper function, because we don't know where it will be useful in the future (and if it will).
I wonder if it works on the client-side. It could skip browsers in spectate mode

rafalh · 2024-11-30T22:52:09Z

game_patch/multi/votes.cpp

+
+            const auto current_player_list = get_current_player_list(false);
+
+            auto yes_votes = std::count_if(players_who_voted.begin(), players_who_voted.end(), [](const auto& pair) {


you repeated that no-trivial vote counting code twice - it should be split into a function and not repeated.
Not sure if getting rid of counters is a good call. It makes counting harder but at least there's one source of truth so maybe it's an improvement.

rafalh · 2024-11-30T22:57:44Z

game_patch/multi/votes.cpp

+            auto no_votes = players_who_voted.size() - yes_votes;
+
+            auto msg = std::format("\xA6 Vote status: Yes: {} No: {} Waiting: {}", yes_votes, no_votes,
+                                   current_player_list.size() - players_who_voted.size());


I think waiting players deserve a variable

In my view it makes more sense to dynamically count based on the current players minus players who have already voted. I don't think such a variable would have a purpose in that approach.

I meant local variable, not a class member. It would increase readability

rafalh · 2024-11-30T23:00:50Z

game_patch/multi/votes.cpp

+
+        const int remaining_players_count =
+            std::count_if(current_player_list.begin(), current_player_list.end(), [this](rf::Player* player) {
+                return players_who_voted.find(player) == players_who_voted.end();


use std::map::contains, we have C++20 enabled

rafalh · 2024-11-30T23:15:33Z

docs/CHANGELOG.md

 - Add level filename to "Level Initializing" console message
 - Properly handle WM_PAINT in dedicated server, may improve performance (DF bug)
 - Fix crash when `verify_level` command is run without a level being loaded
+- Improve handling for players leaving/joining multiplayer games during votes


I think it is too generic. It may be interesting for people to know what exactly changed (e.g. if they can vote after join or not)

I don't really agree - I think the current text is as specific as any player would really care about, but I'm happy to change it to be more specific if you'd like.

How is user reading the release notes know what changed if you only say that something "improved"? Improved in what way?

Honestly, I think "changelog" and "release notes" are being conflated here - simply saying "improved handling" would be suitable IMO for release notes (directed at a general audience), but a more descriptive listing of specifically what changed would be better for a changelog (directed at folks interested in knowing specifically what changes are made).

I'll rephrase this per your suggestion when I have a chance to loop back to this.

GooberRF · 2024-12-01T03:59:02Z

I think you're correct in that it handles removing a "yes" for for someone who votes and then leaves, though - apologies for wrongly stating that the previous code didn't handle that. From memory I don't think it properly handles adjusting the required number of "yes" votes if players leave after a vote has been called, though? I'd need to re-read the code to confirm, but that's my memory based on using the system for years anyway.

In my view, here's how voting should work in all cases with no exceptions:

Vote passes when threshold is met
Vote is cancelled if all players leave, or if the person who called the vote leaves
Pass threshold is calculated as a percentage of the current player count at any given time (ie. players who leave during a vote lower the number of "yes" votes required to pass, and players who join during a vote raise the number)
Players who join during a vote can vote

I think it's more elegant and less prone to logic and timing issues if the voting system simply checks for eligible players from the pool of total players whenever its relevant rather than maintaining arrays of players and shifting them between waiting and voted. I understand that it is a little likely a little less efficient that way, but I'm not overly concerned with that given that it would only be happening while a vote is on-going - not all the time.

I don't think it's a particularly valid concern personally - but if there is a concern about newly joined players not knowing what they are voting for, perhaps the reminder could be updated to indicate the topic of the vote?

I'm not sure if it makes sense. Someone could troll people to vote and then leave in the last second. If someone creates a vote to switch to map X, everybody accept, and then the initial vote caller leaves (or lost connection), why would we want the vote to be ended, especially if most people were in favor? Should people restart the vote? After vote already started there are more people involved already and canceling it wastes all the votes. What do you think?

I think the best approach here should be decided based on the most logical behaviour under normal circumstances rather than by what makes trolling least effective - that said, if trolling were a primary concern, a far more likely vector under the existing approach, I would think, would be joining servers and calling a pointless vote, then leaving. That would block use of the voting system while the vote is on-going and send reminder pings to all players. At least with this new logic, the vote would be cancelled when they leave.

is-this-c · 2024-12-01T06:45:43Z

Vote is cancelled . . . if the person who called the vote leaves

Disagree. Only cancel it if nobody voted yet.

I don't think it's a particularly valid concern personally - but if there is a concern about newly joined players not knowing what they are voting for, perhaps the reminder could be updated to indicate the topic of the vote?

If you are going to let them vote they should be told immediately there is an active vote and what it is for. Moreover it needs to be differential. It cannot be e.g. for vote kick or vote ban.

rafalh · 2024-12-01T12:55:37Z

I think you're correct in that it handles removing a "yes" for for someone who votes and then leaves, though - apologies for wrongly stating that the previous code didn't handle that. From memory I don't think it properly handles adjusting the required number of "yes" votes if players leave after a vote has been called, though? I'd need to re-read the code to confirm, but that's my memory based on using the system for years anyway.

I've checked the code again and I think it is correct. Players were removed from remaining_players set, and that was the place which really counted to determine if vote can be ended and what the result would be. At the same time they were not removed from players_who_voted, which was a small bug, but in most circumstances it wouldn't break anything unless new player joined and had the same memory address assigned (unlikely but possible). It was only used to subtract player's vote when the player left.
What I think has caused you to think that those leaving players affected the voting is because you removed all the variables except players_who_voted and used that single variable for everything. Because it kept those leaving players, it gave you wrong results.

I don't think it's a particularly valid concern personally - but if there is a concern about newly joined players not knowing what they are voting for, perhaps the reminder could be updated to indicate the topic of the vote?

Yes, it is a valid concern. It makes no sense to let people vote when they don't know what they are voting for.

I still don't understand why do you want to cancel the vote when the player creating it leaves. Vote affects everybody on the server, not just that player. If people want to switch the level why the person creating the vote should matter?

GooberRF · 2024-12-01T21:19:09Z

What I think has caused you to think that those leaving players affected the voting is because you removed all the variables except players_who_voted and used that single variable for everything. Because it kept those leaving players, it gave you wrong results.

My comments on this were from before I made any changes whatsoever, and were based on my understanding from having used it in practice for a long time. Based on your tests it seems I was mistaken, but I thought for sure in 1.8.0 it worked as I described. Will have a look later and see if I can figure out why I thought that.

Yes, it is a valid concern. It makes no sense to let people vote when they don't know what they are voting for.

Agreed that it doesn't make sense, I'm just not convinced the concern is big enough to rationalize removing the ability for them to vote. In any event, the most reasonable approach on this is to just let players know when they join if there is a pending vote and if so, what it is - then allow them to consider it if they choose. Perhaps letting them know how long is left on the vote as well would make sense too?

I still don't understand why do you want to cancel the vote when the player creating it leaves. Vote affects everybody on the server, not just that player. If people want to switch the level why the person creating the vote should matter?

My thinking is that voting is effectively the asking of a question. When I call vote map dm02, I am saying "I want to play dm02 with you all, who's with me?". If I leave, that question is no longer relevant. If after I leave, the remaining players decide they still want dm02 even though I have left, they can ask the question themselves.

This approach makes far more sense to me from a UX perspective - but it's ultimately your choice on this PR of course. If you disagree, this part of the PR can be omitted or changed, but the approach I described is what produces the best UX in my view.

rafalh · 2024-12-01T22:02:56Z

Based on your tests it seems I was mistaken

I didn't test anything. I've just read the code. From what I see it only checks counters and remaining_players to determine the vote result. It's possible there is a bug but I don't see it.

Agreed that it doesn't make sense, I'm just not convinced the concern is big enough to rationalize removing the ability for them to vote. In any event, the most reasonable approach on this is to just let players know when they join if there is a pending vote and if so, what it is - then allow them to consider it if they choose. Perhaps letting them know how long is left on the vote as well would make sense too?

As I said I removed the ability to vote mostly because of votekicks/bans where it can hurt innocent people if someone votes without understanding what they vote for. Now that I think about it another (maybe more important) reason is that new people will make the vote longer, because more "yes" votes will be needed to end it. Vote is more likely to end with a draw, especially if they join in last second and don't have enough time to vote. Maybe they shouldn't be counted if they joined a short time before the end?

If you want to enable votes for late-joiners, it should tell them what is going on. Otherwise this PR doesn't have much sense. Why would late-joiner vote if they don't know a vote is in progress?
Giving them precise time until the vote ends is unnecessary in my opinion - we don't do it even at the vote start, so why do it for late-joiners.
I think we need a separate message - "xxx vote started by" would be weird. "xxx vote is in progress" would be good.

My thinking is that voting is effectively the asking of a question. When I call vote map dm02, I am saying "I want to play dm02 with you all, who's with me?". If I leave, that question is no longer relevant. If after I leave, the remaining players decide they still want dm02 even though I have left, they can ask the question themselves.
This approach makes far more sense to me from a UX perspective - but it's ultimately your choice on this PR of course. If you disagree, this part of the PR can be omitted or changed, but the approach I described is what produces the best UX in my view.

In practice if other people have agreed that the map should be changed, why ignore their votes and force them to create a new vote? It doesn't harm anyone if the vote continues. And it's not really "who's with me". In case of "vote map" it's more like "who wants to change the current map". It's not about particular player, but about the server. The vote is still relevant without the player who initially created it. If players disagree, they can deny it, but they may also feel the same. I've even checked how it is implemented in MTA:SA and they don't seem to cancel vote in such case.

GooberRF added 4 commits October 24, 2024 16:05

initial commit

2bee9f8

fix issue detecting browsers

9c97dae

fix vote check issue

472ecfa

clean up

0662542

rafalh requested changes Nov 30, 2024

View reviewed changes

GooberRF closed this Feb 3, 2025

GooberRF deleted the improvedvotes branch February 3, 2025 18:52


		const auto current_player_list = get_current_player_list(false);

		auto yes_votes = std::count_if(players_who_voted.begin(), players_who_voted.end(), [](const auto& pair) {

Improve handling for players leaving/joining multiplayer games during votes #298

Improve handling for players leaving/joining multiplayer games during votes #298

Conversation

GooberRF commented Oct 24, 2024

Uh oh!

rafalh left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GooberRF commented Dec 1, 2024

Uh oh!

is-this-c commented Dec 1, 2024

Uh oh!

rafalh commented Dec 1, 2024

Uh oh!

GooberRF commented Dec 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rafalh commented Dec 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

3 participants

rafalh left a comment •

edited

Loading

GooberRF commented Dec 1, 2024 •

edited

Loading