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

Remove search boundaries #848

Closed
wants to merge 1 commit into from
Closed

Conversation

BlackYps
Copy link
Collaborator

I guess they were used by the python client for the "A person in your rating range is searching for a game. Click here to join them." message. However, the ranges are unreliable, because it just gives a hardcoded range and the trueskill quality also depends on the deviation. Also we alter the acceptable quality for every unsucessful queue pop. For team queues we use a different system entirely.
The java client doesn't use them for anything at the moment.

@BlackYps BlackYps added this to the v2.0 milestone Oct 13, 2021
@codecov
Copy link

codecov bot commented Oct 13, 2021

Codecov Report

Merging #848 (4fab2d7) into develop (7be92f1) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted Files Coverage Δ
server/matchmaker/matchmaker_queue.py 92.02% <ø> (ø)
server/matchmaker/search.py 96.22% <ø> (-0.21%) ⬇️

@Askaholic
Copy link
Collaborator

Askaholic commented Oct 13, 2021

Btw there is a v2-develop branch where I’m going to merge breaking changes to. It’s a bit behind though since we had some more releases after I created it, so when I find the time I’m going to fast forward it again. But, any v2 prs should be made against that branch.

boundaries = queue["boundary_80s"]

if queue["queue_name"] == "tmm2v2":
assert boundaries == [[300, 700]]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe check the num_players here instead

@@ -488,7 +488,7 @@ async def test_abort(mocker, lobbyconnection):
async def test_send_game_list(mocker, database, lobbyconnection, game_stats_service):
games = mocker.patch.object(lobbyconnection, "game_service") # type: GameService
game1, game2 = mock.create_autospec(Game(42, database, mock.Mock(), game_stats_service)), \
mock.create_autospec(Game(22, database, mock.Mock(), game_stats_service))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is pre-commit doing this? I think gatsik’s formatting pr has the same stuff.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. I stopped running the pre-commit hooks, because flake8 failed because of the type hints server/matchmaker/matchmaker_queue.py:45:23: F821 undefined name 'GameService'. I still left the whitespace changes from the hook in because I wasn't sure which way it is supposed to be.

@Gatsik
Copy link
Contributor

Gatsik commented Oct 21, 2021

Please do not merge this, python client still uses it and will use it in the future for its ladder notifications.
When someone sees this notification they could be almost sure that they will get a match if they start searching (worked perfectly for me).

@BlackYps
Copy link
Collaborator Author

It won't be merged before v2.0. v2.0 will probably have some more breaking protocol changes, which will break python client functionality.

@Gatsik
Copy link
Contributor

Gatsik commented Oct 22, 2021

Python client is currently ready for every single server's pull request to be merged and deployed today, except the match confirmation thing, but I guess it will be fixed in time. If something else will be added later to v2.0 changes, I think this would be fixed too

@Askaholic
Copy link
Collaborator

Superseded by #816

@Askaholic Askaholic closed this Jan 17, 2022
@Askaholic Askaholic deleted the feature/remove-search-boundaries branch January 17, 2022 21:42
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

Successfully merging this pull request may close these issues.

None yet

3 participants