Replace party `ready` check with match confirmation #607

Askaholic · 2020-06-18T20:52:12Z

Overview

For the purposes of a matchmaker it makes more sense to perform the "are players ready to start" check when a match is found instead of when the party is entered into the queue. I suggest that we add a match_info message that look something like this:

{
    "command": "match_info",
    "expires_at": "2020-06-18 20:30:26.677218",
    "players_total": 4,
    "players_ready": 1,
    "ready": false
}

Where expires_at is a timestamp in ISO format and ready refers the the ready state of the player that the message is being sent to (this way the client can synchronize its UI state with the server).

The client can display this information and provide a "Ready up" button. When a player clicks the button, the client will send a match_ready command that will look something like this:

{
    "command": "match_ready"
}

Once all players have readied up, the server will start the match launching procedure. If some players fail to ready up by the expiry time (say 20 seconds from when the match was found) then the match will be cancelled, the server will send a match_cancelled message and return both searches to the matchmaker queue (notifying the client with a search_info command).

Implementation

I suggest writing a small wrapper around a future that will keep track of which players have readied up:

class MatchOffer:
    def __init__(self, players, expires_at):
        ...

    def ready_player(self, player):
        ...

    async def wait_ready(self):
        ...

    def to_dict(self):
        """Return contents of `match_info` message."""

and adding a weak reference to it on the Player objects. Then the player object can also get a method

def ready_match(self):
    ...

which will check if the player has a match offer and proxy the call to ready_player.

Multiqueue support

To support players joining multiple queues at the same time we'll also need to modify the Search object and add a property which signals whether the search should be included in the matching phase, or if it is currently "busy" waiting for players to accept a match offer.

The text was updated successfully, but these errors were encountered:

)

* Add dummy implementation for client command `match_ready` (#607) * Add test * Fx test * Fix test 2 * Fix test 3

Katharsas · 2021-08-11T22:34:54Z

@Askaholic
Wouldnt it make sense to have the "ready" flag in the message from the client too?

That way it would be possible for the client to detect sombody dropping out (for example by closing the client) and then instesd of everybody having to wait for the timeout, the server could immediately cancel the match.

Sheikah45 · 2021-08-11T22:41:50Z

There is a ready and unready message

Sheikah45 · 2021-08-11T22:44:06Z

Wait yeah I see this spec is different. I thought that the messages were the ready_party and unready_party messages as these were put into the client? Not that it matters to much as the name and text can be changed but just making sure.

Askaholic · 2021-08-11T23:02:44Z

Sounds like a reasonable idea to me.

Sheikah45 · 2021-08-12T00:04:16Z

So can the ready and unready party commands be removed as they aren't currently used anyway

Askaholic · 2021-08-12T01:22:29Z

Yea I think so. We were going to repurpose them to solve the issue of not knowing when your party members are in game, but I don't think that will happen anymore. I'd rather solve that issue the right way.

Katharsas · 2021-08-12T18:21:01Z

Soo... what exactly is the conclusion? Keep match_ready? Add flag? Remove the others? And while we are at it, i find the command name a bit confusing, but i guess its really not that important.

Askaholic · 2021-08-12T20:36:55Z

Now would be the time to change it so feel free to suggest how you think it should work. A flag is probably better that two separate commands.

Katharsas · 2021-08-12T21:00:58Z

Yeah i feel it would be cleaner with a flag, so like this:

{
    "command": "match_ready"
    "ready": true
}

Katharsas · 2021-08-21T22:32:54Z

What format is this 2020-06-18 20:30:26.677218 exactly? Are there other commands where the client receives a date in this format? From what i can see, an ISO 8601 needs a T intead of a space.

Askaholic · 2021-08-22T00:04:28Z

Yea it should have a T. Not sure where I got that from, but we should stick to standard ISO format. One thing we learned from using ISO timestamps for matchmaker queue pops is that they lead to bad data sometimes when users system time is misconfigured. So we might want to consider a time delta instead.

Sheikah45 · 2021-08-22T01:00:04Z

Both never hurts in case of weird delays as well

Katharsas · 2021-08-22T01:24:23Z

Isnt delta time a bit overkill here?
The worst thing that happens is that client doesnt understand that match is expired, should not be a big problem i think. I don't think its reasonable to expect any networking heavy software to behave properly if system time is wrong.

Askaholic · 2021-08-22T06:33:07Z

Well do you want people complaining on the forums that the match confirmation box is displaying a time of 2 hours or do you not want that to happen...

Katharsas · 2021-08-22T19:55:19Z

Sendiing both relative and absolute times whenever you need to send a time is really the solution to that problem? How often does this problem happen? Should we not make a general check if system clock is correct instead of duplicating all our time fields wherever we go?

The misconfigured time is a problem that lies on a completely different abstraction level than matchmaker code should ever concern itself with, so sending two fields here is just messy.

If the time problem is really a relevant problem than that can be handled with a check that will not start the client at all if time is misconfigured.

Sheikah45 · 2021-08-22T19:58:08Z

It is for better user experience. Otherwise you have to tell every user who has the issue to set their clock correctly. And locking people out of the client because of a mis-configured system time is definitely not a good thing to do.

Katharsas · 2021-08-22T20:04:06Z

Telling the user to fix his time settings is much more reasonable than fixing broken time sync in every time related API call that will ever be added to this client. Fixing it in this issue means that you will fix it again and again ein every API to come! Your are increasing, not just code but API complexity, to workaround user level problems, and you are doing it in a way that it will infect all future APIs as well!

Katharsas · 2021-08-22T20:10:14Z

Here is an example for how a much better solution could look like:

Have a sync endpoint that you can use to find out by how much the user clock is wrong.
Correct all calls that get current time by that drift.
Your API can stay simple and clean as it should

Dont get me wrong,, i am not in favor of doing that, i think a simple user facing error is fully reasonable, but at least you don't infect unrelated API endpoints with time problem workarounds.

Katharsas · 2021-08-22T21:20:11Z

I guess we could just have a relative time in seconds. But having two times (if they conflict you would use the relative time anyway) is just not right.

Askaholic · 2022-01-01T07:44:42Z

We wouldn't even need to add a time sync endpoint. We could just send the current time in the welcome message.

Askaholic added the enhancement label Jun 18, 2020

Askaholic added this to To do in Team Matchmaker Jun 18, 2020

This was referenced Jun 18, 2020

TMM: Add match confirmation (ready check) for matchmaker games FAForever/downlords-faf-client#1783

Open

Feature/#607 match confirmation #608

Open

Askaholic moved this from To do to In progress in Team Matchmaker Jun 20, 2020

Askaholic self-assigned this Jun 20, 2020

Askaholic moved this from In progress to Review in Team Matchmaker Jun 28, 2020

Askaholic added the requires client changes label Aug 3, 2020

Askaholic mentioned this issue Mar 5, 2021

Things to change for a protocol v2 #573

Open

31 tasks

Katharsas added a commit to Katharsas/server that referenced this issue Aug 8, 2021

Add dummy implementation for client command match_ready (FAForever#607

2519b86

)

Katharsas mentioned this issue Aug 8, 2021

Add dummy implementation for client command match_ready (#607) #820

Merged

Katharsas added a commit to Katharsas/server that referenced this issue Aug 8, 2021

Add dummy implementation for client command match_ready (FAForever#607

ba0e6c9

)

Askaholic pushed a commit that referenced this issue Aug 9, 2021

Add dummy implementation for client command match_ready (#607) (#820)

9955891

* Add dummy implementation for client command `match_ready` (#607) * Add test * Fx test * Fix test 2 * Fix test 3

Katharsas mentioned this issue Aug 11, 2021

Send match_ready to server immediatley on match_found (#1783) FAForever/downlords-faf-client#2324

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace party `ready` check with match confirmation #607

Replace party `ready` check with match confirmation #607

Askaholic commented Jun 18, 2020 •

edited

Katharsas commented Aug 11, 2021 •

edited

Sheikah45 commented Aug 11, 2021

Sheikah45 commented Aug 11, 2021

Askaholic commented Aug 11, 2021

Sheikah45 commented Aug 12, 2021

Askaholic commented Aug 12, 2021

Katharsas commented Aug 12, 2021 •

edited

Askaholic commented Aug 12, 2021

Katharsas commented Aug 12, 2021

Katharsas commented Aug 21, 2021 •

edited

Askaholic commented Aug 22, 2021

Sheikah45 commented Aug 22, 2021

Katharsas commented Aug 22, 2021 •

edited

Askaholic commented Aug 22, 2021

Katharsas commented Aug 22, 2021 •

edited

Sheikah45 commented Aug 22, 2021

Katharsas commented Aug 22, 2021

Katharsas commented Aug 22, 2021 •

edited

Katharsas commented Aug 22, 2021

Askaholic commented Jan 1, 2022

Replace party ready check with match confirmation #607

Replace party ready check with match confirmation #607

Comments

Askaholic commented Jun 18, 2020 • edited

Overview

Implementation

Multiqueue support

Katharsas commented Aug 11, 2021 • edited

Sheikah45 commented Aug 11, 2021

Sheikah45 commented Aug 11, 2021

Askaholic commented Aug 11, 2021

Sheikah45 commented Aug 12, 2021

Askaholic commented Aug 12, 2021

Katharsas commented Aug 12, 2021 • edited

Askaholic commented Aug 12, 2021

Katharsas commented Aug 12, 2021

Katharsas commented Aug 21, 2021 • edited

Askaholic commented Aug 22, 2021

Sheikah45 commented Aug 22, 2021

Katharsas commented Aug 22, 2021 • edited

Askaholic commented Aug 22, 2021

Katharsas commented Aug 22, 2021 • edited

Sheikah45 commented Aug 22, 2021

Katharsas commented Aug 22, 2021

Katharsas commented Aug 22, 2021 • edited

Katharsas commented Aug 22, 2021

Askaholic commented Jan 1, 2022

Replace party `ready` check with match confirmation #607

Replace party `ready` check with match confirmation #607

Askaholic commented Jun 18, 2020 •

edited

Katharsas commented Aug 11, 2021 •

edited

Katharsas commented Aug 12, 2021 •

edited

Katharsas commented Aug 21, 2021 •

edited

Katharsas commented Aug 22, 2021 •

edited

Katharsas commented Aug 22, 2021 •

edited

Katharsas commented Aug 22, 2021 •

edited