Support for multiple player ratings #486

Wesmania · 2019-08-21T19:59:29Z

This PR will eventually add support for more player ratings in preparation for team matchmaking. For now though, it's a few misc cleanups.

norraxx · 2019-08-21T21:08:35Z

server/players.py

+        if global_rating is not None:
+            self.global_rating = global_rating
+        if ladder_rating is not None:
+            self.ladder_rating = ladder_rating


Bad naming for global_rating, self.global_rating and self._global_rating, how about to create and use set_global_rating()?

This commit does not change existent naming, and the name property backed by a _name attribute is pretty common practice. The property already acts like a setter and has no side effects beyond setting the backing value anyway.

norraxx · 2019-08-21T21:13:24Z

server/players.py

@@ -76,6 +79,39 @@ def __init__(
        self._game = lambda: None
        self._game_connection = lambda: None

+    @property


Don't use @property, it doesn't give you anything, only hiding call functions + add wrappers, without proper IDE impossible to detect with naked eye.
Use property only if you're trying to not to break same API during refactoring.
Better to use set_ methods. It's more explicit.

It has no side effects beyond setting the backing value. Sounds like a perfectly fine usage of @property to me.

In fact it has. If you see below, you'll find condition that decides what to do with Rating instance. It is hiding behavior.

I was talking about side effects. The only thing setting this value ever affects is the value returned by the getter. This is a valid use case for a property, what you're describing restricts @property to trivial cases.

norraxx · 2019-08-21T21:14:55Z

server/players.py

@@ -35,18 +35,21 @@ def __init__(
        permission_group: int = 0,
        lobby_connection: "LobbyConnection" = None
    ):
-        super().__init__(player_id, login)
+        self._faction = 0


typing for local variables + typing for incoming variables like clan, global_rating, ladder_rating.

I had no reason to touch this code beyond copying it. Faction could use an enum anyway rather than just an obvious typing hint.

You can make it better ;-) you're refactoring, isn't it?

Whenever possible, one commit is one feature.

norraxx · 2019-08-21T21:19:01Z

server/players.py

+        return self._global_rating
+
+    @global_rating.setter
+    def global_rating(self, value: Rating):


wrong typing, use Union[Tuple[int, int], Rating] https://docs.python.org/3/library/typing.html#typing.Union

This is copied from BasePlayer, so I didn't bother touching it. Frankly you know my opinion on type hints, in trivial cases like this one they bring exactly zero benefit.

norraxx · 2019-08-21T21:20:57Z

server/players.py

+            self._global_rating = value
+
+    @property
+    def ladder_rating(self):


missing return typing, if it would be just variable, not property, code would be much easier to read, and for outside world it behaves the same

As above, didn't bother changing copied code. It deserves to be a named tuple anyway, we can change it then.

tests/conftest.py

norraxx · 2019-08-21T21:30:11Z

server/players.py

+        if isinstance(value, Rating):
+            self._global_rating = (value.mu, value.sigma)
+        else:
+            self._global_rating = value


if value is None, you may achieve bad behavior.

This is the original behaviour and it was not my intention to change it. It's not set to None internally and if there's an inconsistency with how other code uses it, then you can show and fix it.

Maybe it would be a good opportunity to add assert value is not None

I don't see benefit from moving code from one place to another, if we can make it better.

@norraxx Because it's a lot of extra work and a lot higher chance that you mess something up on accident. I'd be happy if you wanted to make a PR though :)

norraxx · 2019-08-21T21:33:21Z

server/players.py

@@ -35,18 +35,21 @@ def __init__(
        permission_group: int = 0,
        lobby_connection: "LobbyConnection" = None
    ):
-        super().__init__(player_id, login)
+        self._faction = 0


I wouldn't use fraction 0, it doesn't looks like it's undefined. Maybe use None?

Commit has no intention to change original behaviour.

Askaholic · 2019-08-29T05:27:15Z

server/games/custom_game.py

@@ -1,6 +1,8 @@
 import time

 from .game import Game, ValidityState
+from server.rating import RatingType


For now this makes sense, but it won't anymore once you finish multiple rating support :P

You want something else than an enum? Is it supposed to be populated from the db?

Askaholic · 2019-08-29T05:29:32Z

server/games/game.py

@@ -242,6 +247,16 @@ def connections(self):

    @property
    def teams(self):
+        """
+        A dictionary of lists representing teams


It returns a set not a dict. It would look like {2, 3} assuming that there are 2 teams

Copypasted from BaseGame, didn't touch it yet.

Can you please fix or delete this comment, because it's wrong

server/matchmaker/search.py

Askaholic · 2019-08-29T05:35:06Z

server/matchmaker/search.py

-        return rounded_mu - 100, rounded_mu + 100
+    @property
+    def boundary_75(self):
+        """ Achieves roughly 75% quality. FIXME - why is it MORE restrictive??? """


I guess my TODO comment was poorly worded. But how are these boundaries going to make sense in a team context?

Dunno, I didn't want to change behaviour here yet.

Askaholic · 2019-08-29T05:37:45Z

server/rating.py

+from trueskill import Rating
+
+
+# Values correspond to legacy table names. This will be fixed when db gets


Askaholic · 2019-08-30T07:30:52Z

Popped this onto the test server real quick and got the following error:

INFO     Aug 30  07:29:05 root                           Using StatsD server: 
ERROR    Aug 30  07:29:05 root                           Failure booting server connect() got an unexpected keyword argument 'loop'
Traceback (most recent call last):
  File "server.py", line 68, in <module>
    loop=loop
TypeError: connect() got an unexpected keyword argument 'loop'

Wesmania · 2019-08-31T13:01:21Z

Server launch issue should be fixed now.