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

Ladder1v1: Add support for match_found and game_launch_cancelled commands #1750

Closed
Askaholic opened this issue Jun 4, 2020 · 11 comments
Closed

Comments

@Askaholic
Copy link
Collaborator

When a player queueing for ladder has found a match the server will send a message to both players like:

{
    "command": "match_found"
}

which signals that the client can stop the searching animation.

(on the TMM branch the message will look like this)

{
    "command": "match_found"
    "queue": <queue_name>
}

If the host of the ladder game fails to connect after a timeout, the server will send a message to both players like:

{
    "command": "game_launch_cancelled"
}

which signals that the game was cancelled on the server side, and the client should stop waiting for the game to start. If this happens, the client could automatically kill the player's FA instance if it is running. In order to support future enhancements this should also happen for any joining players who's game instances have been started by the matchmaking code.

This should probably also generate an immediate notification (the ones that pop up on the corner of your screen) and could even enter the player into the queue again.

Wanna have the bug fixed quickly?
Visit Issue hunt...
Issue hunt

@1-alex98
Copy link
Member

1-alex98 commented Jun 4, 2020

or not?

@Askaholic
Copy link
Collaborator Author

I think I might rename game_launch_cancelled to match_cancelled. But that's pretty trivial to change so it can still be implemented as is.

@cleborys
Copy link
Member

In trying to understand the match_cancelled message, I see the following:

  1. Run askaholics-faf-cli against the test server and send game_matchmaking start
  2. Run downloards-faf-client against the tests server and search for ladder matches.
  3. The two players match, the cli user is host, because he started a search first, both receive match_found (here the downlord client should stop the searching animation)
  4. the cli won't be launching a game, so nothing happens for 30 seconds
  5. both receive the match_cancelled message
  6. downlords-faf-client launches the game with the "setting up automatch" perspective, which I assume means it is now host?

I am confused about why point 6 is happening. Is that a bug and I should make an issue? Is that some side-effect of using the cli? Is that a second attempt to start the match with the other player hosting (I like I would have heard about that)?

As a side note, I think the match_found message should also spawn a quick notification, given that it can be 30 seconds between match_found and match_cancelled and it's disorienting if the search animation simply stops and nothing happens for 10 seconds already.

Along the same lines, what should happen if during these 10 seconds the user clicks the "Play" button again? Maybe the play button should still be invisible?
Currently you can happily continue to cancel your search and/or start a new one between match_found and match_cancelled, as from a user perspective nothing is different from not having been matched and waiting for the next queue pop instead.

@cleborys
Copy link
Member

As a first clue towards debugging the above:
If during the 30 seconds of point 4. you cancel your search, you won't launch the game at point 6.
If during the 30 seconds you cancel your search and start a new one, you will still spawn the game at point 6. at 30 seconds after the queue pop (so long before the next queue pop).

@Askaholic
Copy link
Collaborator Author

Both players should always be receiving a game_launch command. The client doesn't know about match_cancelled yet so if we don't send anything it will continue to display that it's searching even when it's not.

https://github.com/FAForever/server/blob/ccf349e3b4bbaee95373d79a0e2f4ee9c5d064a2/server/ladder_service.py#L371

@cleborys
Copy link
Member

cleborys commented Jun 17, 2020

Ok, then I think the finally above the comment you linked should be changed to else as soon as it's implemented client-side?

As I understand the above scenario now the server is currently sending the game_launch to the host, waits 30 seconds but receives no answer, then raises the TimeoutError and sends match_cancelled and game_launch to the guest at the same time.

[edit: after looking at the ladder_service line more I see that rather than changing finally to else one should remove the entire try block which I now see is what you intended all along 👍 I think I was confused because the issue talked of killing the running game so I assumed the guest client should have started the game before getting cancelled]

@cleborys
Copy link
Member

cleborys commented Jun 17, 2020

That would mean that a guest either receives game_launch or match_cancelled, but not both.
Alternatively, if the server sends both game_launches immediately, we can close the game through the client upon receiving match_cancelled as you suggested in the issue description.

@Askaholic
Copy link
Collaborator Author

I’m not sure that sending game launch to both players at the same time will work. There used to be a hardcoded 5 second delay which I assume is because the host needs some time to get the lobby ready before the guest can join it.

@cleborys
Copy link
Member

Are you expecting that match_cancelled will be used in other situations than a host timeout?
If not then maybe we don't need to kill the game as a reaction since that might e.g. interfere with joining a custom game in between match_found and match_cancelled.

@Askaholic Askaholic moved this from To do to In progress in Team Matchmaker Jun 18, 2020
@Geosearchef Geosearchef moved this from In progress to Review in Team Matchmaker Oct 4, 2020
@Geosearchef Geosearchef removed this from Review in Team Matchmaker Oct 4, 2020
@Geosearchef
Copy link
Member

I'm removing this from the TMM project as it's ladder, not TMM related and TMM handles those messages already.

@Geosearchef Geosearchef changed the title Add support for match_found and game_launch_cancelled commands Ladder1v1: Add support for match_found and game_launch_cancelled commands Oct 4, 2020
@Sheikah45
Copy link
Member

Fixed in #2059

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants