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

Feature/#1032 working veto system #1033

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from
11 changes: 7 additions & 4 deletions server/db/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -287,10 +287,13 @@

matchmaker_queue_map_pool = Table(
"matchmaker_queue_map_pool", metadata,
Column("matchmaker_queue_id", Integer, ForeignKey("matchmaker_queue.id"), nullable=False),
Column("map_pool_id", Integer, ForeignKey("map_pool.id"), nullable=False),
Column("min_rating", Integer),
Column("max_rating", Integer),
Column("matchmaker_queue_id", Integer, ForeignKey("matchmaker_queue.id"), nullable=False),
Column("map_pool_id", Integer, ForeignKey("map_pool.id"), nullable=False),
Column("min_rating", Integer),
Column("max_rating", Integer),
Column("veto_tokens_per_player", Integer, nullable=False),
Column("max_tokens_per_map", Integer, nullable=False),
Column("minimum_maps_after_veto", Float, nullable=False),
)

teamkills = Table(
Expand Down
74 changes: 69 additions & 5 deletions server/ladder_service/ladder_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
Search
)
from server.metrics import MatchLaunch
from server.player_service import PlayerService
from server.players import Player, PlayerState
from server.types import GameLaunchOptions, Map, NeroxisGeneratedMap

Expand All @@ -59,11 +60,13 @@ def __init__(
self,
database: FAFDatabase,
game_service: GameService,
player_service: PlayerService,
violation_service: ViolationService,
):
self._db = database
self._informed_players: set[Player] = set()
self.game_service = game_service
self.player_service = player_service
self.queues = {}
self.violation_service = violation_service

Expand All @@ -75,6 +78,7 @@ async def initialize(self) -> None:
self._update_cron = aiocron.crontab("*/10 * * * *", func=self.update_data)

async def update_data(self) -> None:
prev_pools_veto_data = self.get_pools_veto_data()
async with self._db.acquire() as conn:
map_pool_maps = await self.fetch_map_pools(conn)
db_queues = await self.fetch_matchmaker_queues(conn)
Expand All @@ -100,7 +104,7 @@ async def update_data(self) -> None:
queue.team_size = info["team_size"]
queue.rating_peak = await self.fetch_rating_peak(info["rating_type"])
queue.map_pools.clear()
for map_pool_id, min_rating, max_rating in info["map_pools"]:
for map_pool_id, min_rating, max_rating, veto_tokens_per_player, max_tokens_per_map, minimum_maps_after_veto in info["map_pools"]:
map_pool_name, map_list = map_pool_maps[map_pool_id]
if not map_list:
self._logger.warning(
Expand All @@ -112,19 +116,27 @@ async def update_data(self) -> None:
queue.add_map_pool(
MapPool(map_pool_id, map_pool_name, map_list),
min_rating,
max_rating
max_rating,
veto_tokens_per_player,
max_tokens_per_map,
minimum_maps_after_veto
)
# Remove queues that don't exist anymore
for queue_name in list(self.queues.keys()):
if queue_name not in db_queues:
self.queues[queue_name].shutdown()
del self.queues[queue_name]
current_pools_veto_data = self.get_pools_veto_data()
if (current_pools_veto_data != prev_pools_veto_data):
for player in self.player_service.all_players:
await player.update_vetoes(current_pools_veto_data)

async def fetch_map_pools(self, conn) -> dict[int, tuple[str, list[Map]]]:
result = await conn.execute(
select(
map_pool.c.id,
map_pool.c.name,
map_pool_map_version.c.id.label("map_pool_map_version_id"),
map_pool_map_version.c.weight,
map_pool_map_version.c.map_params,
map_version.c.id.label("map_id"),
Expand All @@ -150,6 +162,7 @@ async def fetch_map_pools(self, conn) -> dict[int, tuple[str, list[Map]]]:
map_list.append(
Map(
id=row.map_id,
map_pool_map_version_id=row.map_pool_map_version_id,
folder_name=folder_name,
ranked=row.ranked,
weight=row.weight,
Expand All @@ -161,7 +174,7 @@ async def fetch_map_pools(self, conn) -> dict[int, tuple[str, list[Map]]]:
map_type = params["type"]
if map_type == "neroxis":
map_list.append(
NeroxisGeneratedMap.of(params, row.weight)
NeroxisGeneratedMap.of(params, row.weight, row.map_pool_map_version_id)
)
else:
self._logger.warning(
Expand Down Expand Up @@ -191,6 +204,9 @@ async def fetch_matchmaker_queues(self, conn):
matchmaker_queue_map_pool.c.map_pool_id,
matchmaker_queue_map_pool.c.min_rating,
matchmaker_queue_map_pool.c.max_rating,
matchmaker_queue_map_pool.c.veto_tokens_per_player,
matchmaker_queue_map_pool.c.max_tokens_per_map,
matchmaker_queue_map_pool.c.minimum_maps_after_veto,
game_featuredMods.c.gamemod,
leaderboard.c.technical_name.label("rating_type")
)
Expand Down Expand Up @@ -219,7 +235,10 @@ async def fetch_matchmaker_queues(self, conn):
info["map_pools"].append((
row.map_pool_id,
row.min_rating,
row.max_rating
row.max_rating,
row.veto_tokens_per_player,
row.max_tokens_per_map,
row.minimum_maps_after_veto
))
except Exception:
self._logger.warning(
Expand Down Expand Up @@ -523,7 +542,23 @@ def get_displayed_rating(player: Player) -> float:
pool = queue.get_map_pool_for_rating(rating)
if not pool:
raise RuntimeError(f"No map pool available for rating {rating}!")
game_map = pool.choose_map(played_map_ids)

pool, _, _, _, max_tokens_per_map, minimum_maps_after_veto = queue.map_pools[pool.id]
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use *_ instead of 3 _ to unpack unneeded variables


vetoesMap = defaultdict(int)
Copy link
Contributor

Choose a reason for hiding this comment

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

why is it suddenly in camelCase?


for m in pool.maps.values():
for player in all_players:
vetoesMap[m.map_pool_map_version_id] += player.vetoes.get(m.map_pool_map_version_id, 0)

if (max_tokens_per_map == 0):
max_tokens_per_map = self.calculate_dynamic_tokens_per_map(minimum_maps_after_veto, vetoesMap.values())
Copy link
Contributor

Choose a reason for hiding this comment

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

calculate_dynamic_tokens_per_map expects list[int], but vetoesMap.values() are actually not a list. if you want to annotate dict's values, you can use ValuesView from collections.abc

Copy link
Author

Choose a reason for hiding this comment

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

good notice
i'll replace calculate_dynamic_tokens_per_map tokens type with Iterable[int]

if (max_tokens_per_map == 0):
self._logger.error("calculate_dynamic_tokens_per_map received impossible vetoes setup, all vetoes cancelled for a match")
vetoesMap = {}
max_tokens_per_map = 1

game_map = pool.choose_map(played_map_ids, vetoesMap, max_tokens_per_map)

game = self.game_service.create_game(
game_class=LadderGame,
Expand Down Expand Up @@ -673,6 +708,35 @@ async def launch_match(
if player not in connected_players
])

def calculate_dynamic_tokens_per_map(self, M: float, tokens: list[int]) -> float:
Copy link
Contributor

Choose a reason for hiding this comment

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

are you trying to achieve sum(weights) = M?

Copy link
Author

@K-ETFreeman K-ETFreeman Dec 31, 2024

Choose a reason for hiding this comment

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

calculate_dynamic_tokens_per_map finds the minimal possible max_tokens_per_map while still respecting M

Copy link
Contributor

@Gatsik Gatsik Dec 31, 2024

Choose a reason for hiding this comment

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

but when do you decide that max_tokens_per_map can't be reduced anymore?

Copy link
Author

@K-ETFreeman K-ETFreeman Dec 31, 2024

Choose a reason for hiding this comment

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

Its just some math
it solves series of equations, if vetoes set correctly, one of them is guaranteed to be correct answer
if vetoes set incorrectly, it will return 0

Copy link
Author

Choose a reason for hiding this comment

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

i'll prepare some explanation how math works

sorted_tokens = sorted(tokens)
if (sorted_tokens.count(0) >= M):
return 1

result = 1
last = 0
index = 0
while (index < len(sorted_tokens)):
(index, last) = next(((i, el) for i, el in enumerate(sorted_tokens) if el > last), (len(sorted_tokens) - 1, sorted_tokens[-1]))
index += 1
divider = index - M
if (divider <= 0):
continue

result = sum(sorted_tokens[:index]) / divider
upperLimit = sorted_tokens[index] if index < len(sorted_tokens) else float("inf")
if (result <= upperLimit):
return result

return 0

def get_pools_veto_data(self) -> list[tuple[list[int], int, int]]:
result = []
for queue in self.queues.values():
for pool, _, _, veto_tokens_per_player, max_tokens_per_map, _ in queue.map_pools.values():
result.append(([map.map_pool_map_version_id for map in pool.maps.values()], veto_tokens_per_player, max_tokens_per_map))
return result

async def get_game_history(
self,
players: list[Player],
Expand Down
4 changes: 4 additions & 0 deletions server/lobbyconnection.py
Original file line number Diff line number Diff line change
Expand Up @@ -1315,6 +1315,10 @@ async def command_set_party_factions(self, message):

self.party_service.set_factions(self.player, list(factions))

async def command_set_player_vetoes(self, message):
converted = {v["map_pool_map_version_id"]: v["veto_tokens_applied"] for v in message["vetoes"]}
await self.player.update_vetoes(self.ladder_service.get_pools_veto_data(), converted)

async def send_warning(self, message: str, fatal: bool = False):
"""
Display a warning message to the client
Expand Down
19 changes: 12 additions & 7 deletions server/matchmaker/map_pool.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,13 @@ def __init__(
def set_maps(self, maps: Iterable[MapPoolMap]) -> None:
self.maps = {map_.id: map_ for map_ in maps}

def choose_map(self, played_map_ids: Iterable[int] = ()) -> Map:
def choose_map(self, played_map_ids: Iterable[int] = (), vetoesMap=None, max_tokens_per_map=1) -> Map:
if vetoesMap is None:
vetoesMap = {}
"""
Select a random map who's id does not appear in `played_map_ids`. If
all map ids appear in the list, then pick one that appears the least
amount of times.
Select a random map using veto system weights.
The maps which are least played from played_map_ids
and not vetoed by any player are getting x2 weight multiplier.
"""
if not self.maps:
self._logger.critical(
Expand All @@ -50,10 +52,13 @@ def choose_map(self, played_map_ids: Iterable[int] = ()) -> Map:
least_common = least_common[:i]
break

weights = [self.maps[id_].weight for id_, _ in least_common]
least_common_ids = {id_ for id_, _ in least_common}

map_id = random.choices(least_common, weights=weights, k=1)[0][0]
return self.maps[map_id].get_map()
# Multiply weight by 2 if map is least common and not vetoed by anyone
mapList = list((map.map_pool_map_version_id, map, 2 if (map.id in least_common_ids) and (vetoesMap.get(map.map_pool_map_version_id, 0) == 0) else 1) for id, map in self.maps.items())
Copy link
Contributor

Choose a reason for hiding this comment

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

don't you feel like this line is a bit long?)

Copy link
Author

Choose a reason for hiding this comment

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

long lines for long PP


weights = [max(0, (1 - vetoesMap.get(id, 0) / max_tokens_per_map) * map.weight * least_common_multiplier) for id, map, least_common_multiplier in mapList]
return random.choices(mapList, weights=weights, k=1)[0][1]

def __repr__(self) -> str:
return f"MapPool({self.id}, {self.name}, {list(self.maps.values())})"
11 changes: 7 additions & 4 deletions server/matchmaker/matchmaker_queue.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ def __init__(
rating_type: str,
team_size: int = 1,
params: Optional[dict[str, Any]] = None,
map_pools: Iterable[tuple[MapPool, Optional[int], Optional[int]]] = (),
map_pools: Iterable[tuple[MapPool, Optional[int], Optional[int], int, int, float]] = (),
):
self.game_service = game_service
self.name = name
Expand Down Expand Up @@ -78,12 +78,15 @@ def add_map_pool(
self,
map_pool: MapPool,
min_rating: Optional[int],
max_rating: Optional[int]
max_rating: Optional[int],
Copy link
Contributor

Choose a reason for hiding this comment

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

why are all of those arguments not inside MapPool 🤔 ?

Copy link
Author

@K-ETFreeman K-ETFreeman Dec 31, 2024

Choose a reason for hiding this comment

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

because its not in the mapPool in the database, but in matchmaker_queue_map_pool (queue bracket), so should be consistent between repos
and yea its that way in db due to some reasons which were discussed long ago (f.e. u can use same pool for multiple brackets)

Copy link
Author

@K-ETFreeman K-ETFreeman Dec 31, 2024

Choose a reason for hiding this comment

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

veto_tokens_per_player: int,
max_tokens_per_map: int,
minimum_maps_after_veto: float,
) -> None:
self.map_pools[map_pool.id] = (map_pool, min_rating, max_rating)
self.map_pools[map_pool.id] = (map_pool, min_rating, max_rating, veto_tokens_per_player, max_tokens_per_map, minimum_maps_after_veto)

def get_map_pool_for_rating(self, rating: float) -> Optional[MapPool]:
for map_pool, min_rating, max_rating in self.map_pools.values():
for map_pool, min_rating, max_rating, _, _, _ in self.map_pools.values():
if min_rating is not None and rating < min_rating:
continue
if max_rating is not None and rating > max_rating:
Expand Down
41 changes: 41 additions & 0 deletions server/players.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ def __init__(
lobby_connection: Optional["LobbyConnection"] = None
) -> None:
self._faction = Faction.uef
self._vetoes = {}

self.id = player_id
self.login = login
Expand Down Expand Up @@ -89,6 +90,46 @@ def faction(self, value: Union[str, int, Faction]) -> None:
else:
self._faction = Faction.from_value(value)

@property
def vetoes(self) -> dict[int, int]:
return self._vetoes

@vetoes.setter
def vetoes(self, value: dict[int, int]) -> None:
if not isinstance(value, dict):
raise ValueError("Vetoes must be a dictionary")
if not all(isinstance(key, int) and isinstance(val, int) and val >= 0 for key, val in value.items()):
raise ValueError("Incorrect vetoes dictonary")
self._vetoes = value

async def update_vetoes(self, pools_vetodata: list[tuple[list[int], int, int]], current: dict = None) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

you could create a container (NamedTuple) for this tuple[int], int, int structure and avoid those long type hints and maybe call it MapPoolVeto

also, if you put tokens and ratings information in MapPool (in database they are in the same table, why are they separated here?) you won't even need this additional structure and you will be able to pass MapPool and use its properties

finally, this can also possibly remove get_pools_veto_data method since it just rearranges MapPool's internals

Copy link
Author

Choose a reason for hiding this comment

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

vetoes are not in map pool in database https://faforever.github.io/db/tables/map_pool.html
vetoes are here: https://faforever.github.io/db/tables/matchmaker_queue_map_pool.html
the same place where min and max rating, outside of map pool itself

Copy link
Author

@K-ETFreeman K-ETFreeman Dec 31, 2024

Choose a reason for hiding this comment

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

also i am not sure wdym removing get_pools_veto_data
it gives only necessary data and readable name for it, allows to avoid any code duplication and keeps the code more clean
i am using it in multiple places so if we remove it code will become just worse

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, i see

What i mean is that now matchmaker_queue_map_pool has additional 3 new fields and it may be viable to create corresponding data container (see, for example, FeaturedMod).
This way you preserve readable names and also make it easier to understand what's passed to other functions

For example, in my opinion

async def update_vetoes(self, list[MatchmakerQueueMapPool], current: dict | None) -> None

is easier to follow than to find calls to update_vetoes and discover what's inside of list[tuple[list[int], int, int]]

also,

map_pools: Iterable[MatchmakerQueueMapPool]

is easier to understand than

map_pools: Iterable[tuple[MapPool, Optional[int], Optional[int], int, int, float]] = ()

where MatchmakerQueueMapPool is

class MatchmakerQueueMapPool(NamedTuple):
    map_pool: MapPool
    min_rating: int | None
    max_rating: int | None
    veto_tokens_per_player: int
    max_tokens_per_map: int
    minimum_maps_after_veto: float

finally, this way long unpacking can be avoided and instead of

for pool, *_, veto_tokens_per_player, max_tokens_per_map, _ in queue.map_pools.values():

you can easily access named attributes

for pool in queue.map_pools.values():
    pool.veto_tokens_per_player...

Copy link
Author

@K-ETFreeman K-ETFreeman Dec 31, 2024

Choose a reason for hiding this comment

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

well, yea, but this going into refactoring existing code zone rather than implementing new features so i was not sure should i do it here or not

Copy link
Member

Choose a reason for hiding this comment

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

Using and changing to a NamedTuple should be fine and probably preferred as Gatsik says

Copy link
Author

@K-ETFreeman K-ETFreeman Jan 1, 2025

Choose a reason for hiding this comment

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

cannot put named tuple MatchmakerQueueMapPool right inside types.py due to circular dependencies happening (map_pool.py uses types and types uses map_pool.py in this case)
is it ok no place it right inside map_pool.py?

if current is None:
current = self.vetoes
fixedVetoes = {}
vetoDatas = []
for (map_pool_map_version_ids, veto_tokens_per_player, max_tokens_per_map) in pools_vetodata:
tokens_sum = 0
for map_id in map_pool_map_version_ids:
new_tokens_applied = max(current.get(map_id, 0), 0)
if (tokens_sum + new_tokens_applied > veto_tokens_per_player):
new_tokens_applied = veto_tokens_per_player - tokens_sum
if (max_tokens_per_map > 0 and new_tokens_applied > max_tokens_per_map):
new_tokens_applied = max_tokens_per_map
if (new_tokens_applied == 0):
continue
vetoDatas.append({"map_pool_map_version_id": map_id, "veto_tokens_applied": new_tokens_applied})
fixedVetoes[map_id] = new_tokens_applied
tokens_sum += new_tokens_applied
if fixedVetoes == self.vetoes == current:
return
self.vetoes = fixedVetoes
if self.lobby_connection is None:
return
await self.lobby_connection.send({
"command": "vetoes_changed",
"vetoesData": vetoDatas
})

def power(self) -> int:
"""An artifact of the old permission system. The client still uses this
number to determine if a player gets a special category in the user list
Expand Down
6 changes: 5 additions & 1 deletion server/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ class Map(NamedTuple):
ranked: bool = False
# Map pool only
weight: int = 1
map_pool_map_version_id: Optional[int] = None

@property
def file_path(self):
Expand All @@ -64,6 +65,7 @@ class NeroxisGeneratedMap(NamedTuple):
spawns: int
map_size_pixels: int
weight: int = 1
map_pool_map_version_id: Optional[int] = None

_NAME_PATTERN = re.compile(
"neroxis_map_generator_([0-9.]+)_([a-z2-7]+)_([a-z2-7]+)"
Expand All @@ -75,7 +77,7 @@ def is_neroxis_map(cls, folder_name: str) -> bool:
return cls._NAME_PATTERN.fullmatch(folder_name) is not None

@classmethod
def of(cls, params: dict, weight: int = 1):
def of(cls, params: dict, weight: int = 1, map_pool_map_version_id: Optional[int] = None):
"""Create a NeroxisGeneratedMap from params dict"""
assert params["type"] == "neroxis"

Expand All @@ -98,6 +100,7 @@ def of(cls, params: dict, weight: int = 1):
spawns,
map_size_pixels,
weight,
map_pool_map_version_id,
)

@staticmethod
Expand Down Expand Up @@ -137,6 +140,7 @@ def get_map(self) -> Map:
# the map argument in unit tests.
MAP_DEFAULT = Map(
id=None,
map_pool_map_version_id=None,
folder_name="scmp_007",
ranked=False,
)
Loading