Feature/#1032 working veto system#1071
Conversation
This reverts commit 6f137fd.
WalkthroughAdds a matchmaker veto system: new VetoService and PlayerVetoes, database/schema changes for veto fields and map pool map-versioning, wiring of veto_service into LobbyConnection and LadderService, map selection and anti-repetition logic updates, and extensive test and fixture updates to exercise veto behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Player
participant LobbyConnection
participant VetoService
participant LadderService
participant MapPool
Player->>LobbyConnection: set_player_vetoes(veto_payload)
LobbyConnection->>VetoService: set_player_vetoes(player, new_vetoes)
VetoService->>VetoService: _adjust_vetoes(new_vetoes)
VetoService-->>LobbyConnection: notify client (vetoes_info) if adjusted
Player->>LobbyConnection: queue_for_matchmaking(queue)
LobbyConnection->>LadderService: start_search(player, queue)
alt when veto-aware selection needed
LadderService->>VetoService: generate_initial_weights_for_match(players, matchmaker_queue_map_pool)
VetoService->>VetoService: aggregate player veto tokens, calculate dynamic caps
VetoService-->>LadderService: initial_weights per map_version_id
LadderService->>MapPool: choose_map(played_map_ids, initial_weights)
MapPool->>MapPool: apply_antirepetition_adjustment(initial_weights,...)
MapPool-->>LadderService: selected Map (includes map_pool_map_version_id)
end
LadderService-->>Player: game_launch_options(map_pool_map_version_id=...)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas needing extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (5)📓 Common learnings📚 Learning: 2025-10-26T15:11:39.923ZApplied to files:
📚 Learning: 2025-10-26T14:43:05.147ZApplied to files:
📚 Learning: 2025-10-26T14:43:30.226ZApplied to files:
📚 Learning: 2025-10-29T18:56:18.770ZApplied to files:
🧬 Code graph analysis (1)server/ladder_service/veto_system.py (8)
🪛 Ruff (0.14.2)server/ladder_service/veto_system.py135-135: Avoid specifying long messages outside the exception class (TRY003) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (10)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
tests/unit_tests/test_veto_system.py (1)
160-227: Consider simplifying inline test case comments.The inline comments for each parameterized test case are very detailed, which aids understanding but makes the parameter list quite verbose. Since the test logic itself validates correctness and the docstring of
calculate_dynamic_tokens_per_mapexplains the algorithm, you might consider either:
- Moving detailed explanations to a module-level docstring
- Using shorter inline comments focusing on the distinguishing feature of each case
However, the current approach is acceptable if it serves as living documentation for the algorithm's expected behavior.
server/lobbyconnection.py (1)
1392-1406: Minor inconsistency in message field access.The code uses
.get()formatchmaker_queue_map_pool_idbut direct dictionary access ([]) formap_pool_map_version_idandveto_tokens_applied. Consider using consistent access patterns—either all.get()with defaults or all[]access—to make the expected message structure more explicit and handle missing fields uniformly.Apply this diff for consistency:
async def command_set_player_vetoes(self, message): assert self.player is not None vetoes = {} for v in message["vetoes"]: - matchmaker_queue_map_pool = v.get("matchmaker_queue_map_pool_id") + matchmaker_queue_map_pool = v["matchmaker_queue_map_pool_id"] map_pool_map_version_id = v["map_pool_map_version_id"] veto_tokens_applied = v["veto_tokens_applied"] if matchmaker_queue_map_pool not in vetoes: vetoes[matchmaker_queue_map_pool] = {} vetoes[matchmaker_queue_map_pool][map_pool_map_version_id] = veto_tokens_applied await self.veto_service.set_player_vetoes(self.player, vetoes)server/matchmaker/map_pool.py (1)
88-108: Consider reducing debug logging verbosity.The method contains 5 debug log statements (lines 88, 94, 104, 105, 107). While debug logs are typically disabled in production, this level of verbosity could impact debuggability by creating noise. Consider consolidating these into 1-2 more structured debug statements, or using a dedicated tracing level if available.
server/ladder_service/ladder_service.py (1)
572-591: Map selection with veto integration looks correct.The code properly:
- Retrieves the appropriate map pool for the players' rating
- Generates veto-aware initial weights using
veto_service.generate_initial_weights_for_match- Passes these weights to
map_pool.choose_mapfor antirepetition-adjusted selectionMinor points for consideration:
- Lines 576-590 contain 3 debug statements that could be consolidated
- Static analysis suggests the RuntimeError at line 574 could use a shorter message or be abstracted to an inner function (TRY301, TRY003)
server/ladder_service/veto_system.py (4)
19-37: Consider adding a setter method for better encapsulation.The
_vetoesattribute is accessed directly throughout the codebase (lines 70, 72, 79, 139-140), which bypasses encapsulation. Consider adding a setter method or making the attribute public if direct access is intended.For example, add a setter method:
class PlayerVetoes: def __init__(self): self._vetoes: dict[BracketID, VetosMap] = {} def get_vetoes_for_bracket(self, bracket_id: BracketID) -> VetosMap: return self._vetoes.get(bracket_id, {}) + + def set_vetoes(self, vetoes: dict[BracketID, VetosMap]) -> None: + self._vetoes = vetoes + + def get_all_vetoes(self) -> dict[BracketID, VetosMap]: + return self._vetoes def to_dict(self) -> dict:Then update callers to use these methods instead of direct attribute access.
73-78: Consider extracting complex boolean logic for clarity.This nested boolean expression is hard to read and reason about. Extracting it into a helper method would improve maintainability.
Consider refactoring to:
+ def _were_veto_tokens_reduced( + original_vetoes: dict[BracketID, VetosMap], + adjusted_vetoes: dict[BracketID, VetosMap], + pool_maps_by_bracket: dict[BracketID, set[int]] + ) -> bool: + """Check if any map's veto tokens were reduced during adjustment.""" + for bracket, bracket_vetoes in original_vetoes.items(): + valid_maps = pool_maps_by_bracket.get(bracket, set()) + for map_id, original_tokens in bracket_vetoes.items(): + if map_id in valid_maps: + adjusted_tokens = adjusted_vetoes.get(bracket, {}).get(map_id, 0) + if original_tokens > adjusted_tokens: + return True + return False + if adjusted_vetoes != player.vetoes._vetoes: - tokens_amount_for_some_map_was_reduced = any( - map_id in pool_maps_by_bracket.get(bracket, set()) - and original_tokens > adjusted_vetoes.get(bracket, {}).get(map_id, 0) - for bracket, bracket_vetoes in player.vetoes._vetoes.items() - for map_id, original_tokens in bracket_vetoes.items() - ) + tokens_amount_for_some_map_was_reduced = _were_veto_tokens_reduced( + player.vetoes._vetoes, + adjusted_vetoes, + pool_maps_by_bracket + )
158-158: Brittle tuple unpacking risks silent breakage.Unpacking
MatchmakerQueueMapPoolVetoDataby position is fragile—if field order changes in the NamedTuple, this code breaks silently.Use named attribute access instead:
- for bracket_id, map_ids, total_tokens, max_per_map, _ in self.pools_veto_data: + for pool_data in self.pools_veto_data: + bracket_id = pool_data.matchmaker_queue_map_pool_id + map_ids = pool_data.map_pool_map_version_ids + total_tokens = pool_data.veto_tokens_per_player + max_per_map = pool_data.max_tokens_per_map + bracket_vetoes = _adjust_vetoes_for_bracket( new_vetoes.get(bracket_id, {}), map_ids, total_tokens, max_per_map, )
175-181: Brittle tuple unpacking risks silent breakage.Similar to line 158, unpacking
MatchmakerQueueMapPoolby position is fragile. The use of*_to capture middle fields makes this especially prone to errors if the NamedTuple structure changes.Use named attribute access:
- ( - pool_id, - pool, - *_, - max_tokens_per_map, - minimum_maps_after_veto - ) = matchmaker_queue_map_pool + pool_id = matchmaker_queue_map_pool.id + pool = matchmaker_queue_map_pool.map_pool + max_tokens_per_map = matchmaker_queue_map_pool.max_tokens_per_map + minimum_maps_after_veto = matchmaker_queue_map_pool.minimum_maps_after_veto
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
server/__init__.py(3 hunks)server/config.py(1 hunks)server/db/models.py(1 hunks)server/ladder_service/ladder_service.py(11 hunks)server/ladder_service/veto_system.py(1 hunks)server/lobbyconnection.py(4 hunks)server/matchmaker/__init__.py(1 hunks)server/matchmaker/map_pool.py(2 hunks)server/matchmaker/matchmaker_queue.py(4 hunks)server/players.py(3 hunks)server/types.py(5 hunks)tests/data/test-data.sql(3 hunks)tests/integration_tests/conftest.py(5 hunks)tests/integration_tests/test_game.py(1 hunks)tests/integration_tests/test_matchmaker.py(2 hunks)tests/integration_tests/test_matchmaker_vetoes.py(1 hunks)tests/integration_tests/test_server_instance.py(2 hunks)tests/integration_tests/test_servercontext.py(1 hunks)tests/unit_tests/conftest.py(4 hunks)tests/unit_tests/test_ladder_service.py(9 hunks)tests/unit_tests/test_lobbyconnection.py(2 hunks)tests/unit_tests/test_map_pool.py(7 hunks)tests/unit_tests/test_matchmaker_queue.py(6 hunks)tests/unit_tests/test_veto_system.py(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: K-ETFreeman
PR: FAForever/server#1033
File: server/ladder_service/veto_system.py:113-124
Timestamp: 2025-10-26T15:11:39.923Z
Learning: In server/ladder_service/veto_system.py, the `+ [-1]` appended to map_pool_map_version_ids in the extract_pools_veto_config method was dead code from an old design idea to allow players to ban "last played map" as an anti-repetition feature. This feature was never implemented and the code was removed.
Learnt from: K-ETFreeman
PR: FAForever/server#1033
File: server/matchmaker/map_pool.py:80-109
Timestamp: 2025-10-26T14:43:30.226Z
Learning: In server/matchmaker/map_pool.py, the choose_map method does not need a guard against all-zero final_weights because the veto system validation logic (including minimum_maps_after_veto constraints) prevents pools from being configured where all maps can be fully banned, ensuring at least some maps always have non-zero weights.
Learnt from: K-ETFreeman
PR: FAForever/server#1033
File: server/ladder_service/veto_system.py:257-280
Timestamp: 2025-10-26T14:43:05.147Z
Learning: In server/ladder_service/veto_system.py, the field max_tokens_per_map is guaranteed to be non-negative through external constraints (not enforced at the database schema level in the visible code).
📚 Learning: 2025-10-26T15:11:39.923Z
Learnt from: K-ETFreeman
PR: FAForever/server#1033
File: server/ladder_service/veto_system.py:113-124
Timestamp: 2025-10-26T15:11:39.923Z
Learning: In server/ladder_service/veto_system.py, the `+ [-1]` appended to map_pool_map_version_ids in the extract_pools_veto_config method was dead code from an old design idea to allow players to ban "last played map" as an anti-repetition feature. This feature was never implemented and the code was removed.
Applied to files:
tests/unit_tests/test_veto_system.pytests/integration_tests/test_matchmaker_vetoes.pyserver/players.pyserver/db/models.pyserver/__init__.pytests/unit_tests/test_ladder_service.pytests/unit_tests/test_lobbyconnection.pytests/data/test-data.sqltests/integration_tests/test_matchmaker.pytests/unit_tests/conftest.pyserver/lobbyconnection.pytests/unit_tests/test_matchmaker_queue.pyserver/types.pytests/integration_tests/test_game.pyserver/matchmaker/map_pool.pyserver/ladder_service/veto_system.pyserver/matchmaker/matchmaker_queue.pytests/integration_tests/conftest.pyserver/ladder_service/ladder_service.pytests/unit_tests/test_map_pool.py
📚 Learning: 2025-10-26T14:43:05.147Z
Learnt from: K-ETFreeman
PR: FAForever/server#1033
File: server/ladder_service/veto_system.py:257-280
Timestamp: 2025-10-26T14:43:05.147Z
Learning: In server/ladder_service/veto_system.py, the field max_tokens_per_map is guaranteed to be non-negative through external constraints (not enforced at the database schema level in the visible code).
Applied to files:
tests/unit_tests/test_veto_system.pytests/integration_tests/test_matchmaker_vetoes.pyserver/db/models.pytests/unit_tests/test_lobbyconnection.pyserver/ladder_service/veto_system.pyserver/ladder_service/ladder_service.py
📚 Learning: 2025-10-26T14:43:30.226Z
Learnt from: K-ETFreeman
PR: FAForever/server#1033
File: server/matchmaker/map_pool.py:80-109
Timestamp: 2025-10-26T14:43:30.226Z
Learning: In server/matchmaker/map_pool.py, the choose_map method does not need a guard against all-zero final_weights because the veto system validation logic (including minimum_maps_after_veto constraints) prevents pools from being configured where all maps can be fully banned, ensuring at least some maps always have non-zero weights.
Applied to files:
tests/unit_tests/test_veto_system.pytests/integration_tests/test_matchmaker_vetoes.pyserver/db/models.pytests/unit_tests/test_ladder_service.pytests/data/test-data.sqlserver/types.pyserver/matchmaker/map_pool.pyserver/ladder_service/veto_system.pyserver/matchmaker/matchmaker_queue.pyserver/ladder_service/ladder_service.pytests/unit_tests/test_map_pool.py
📚 Learning: 2025-10-26T14:51:28.358Z
Learnt from: K-ETFreeman
PR: FAForever/server#1033
File: server/types.py:46-47
Timestamp: 2025-10-26T14:51:28.358Z
Learning: In server/types.py, the MapPoolMap Protocol intentionally requires map_pool_map_version_id to be non-Optional (int) because maps in a map pool context must have this ID. The concrete implementations (Map and NeroxisGeneratedMap) use Optional[int] to support usage outside of map pools, but when used as MapPoolMap they must be instantiated with a non-None value.
Applied to files:
tests/unit_tests/test_ladder_service.pytests/integration_tests/test_matchmaker.pyserver/types.pyserver/matchmaker/map_pool.pyserver/matchmaker/matchmaker_queue.pytests/unit_tests/test_map_pool.py
🧬 Code graph analysis (19)
tests/unit_tests/test_veto_system.py (3)
server/ladder_service/veto_system.py (3)
_cap_tokens(323-335)_is_valid_veto_config_for_queue(257-280)calculate_dynamic_tokens_per_map(211-254)server/matchmaker/map_pool.py (2)
MapPool(13-111)MatchmakerQueueMapPool(114-121)tests/conftest.py (1)
queue_factory(373-395)
tests/integration_tests/test_matchmaker_vetoes.py (8)
server/players.py (1)
PlayerState(24-31)tests/utils/event_loop.py (1)
fast_forward(52-69)tests/integration_tests/conftest.py (3)
connect_and_sign_in(559-569)lobby_server(310-311)get(410-414)tests/integration_tests/test_game.py (4)
client_response(124-127)end_game_as_draw(171-198)gen_vetoes(201-209)queue_player_for_matchmaking(212-226)tests/conftest.py (2)
player_service(307-310)database(198-200)server/player_service.py (1)
get_player(189-190)server/ladder_service/veto_system.py (1)
to_dict(26-37)server/ladder_service/ladder_service.py (1)
update_data(100-154)
server/players.py (3)
server/decorators.py (1)
with_logger(15-29)server/ladder_service/veto_system.py (1)
PlayerVetoes(19-37)server/factions.py (1)
Faction(10-30)
server/__init__.py (3)
tests/integration_tests/conftest.py (2)
ladder_service(51-67)veto_service(79-83)tests/unit_tests/conftest.py (2)
ladder_service(65-81)veto_service(93-97)server/ladder_service/veto_system.py (1)
VetoService(41-254)
tests/unit_tests/test_ladder_service.py (4)
server/matchmaker/map_pool.py (2)
MapPool(13-111)MatchmakerQueueMapPool(114-121)server/matchmaker/matchmaker_queue.py (2)
MatchmakerQueue(46-304)add_map_pool(83-87)server/ladder_service/ladder_service.py (1)
LadderService(71-795)server/types.py (5)
Map(55-73)map_pool_map_version_id(47-47)NeroxisGeneratedMap(79-150)of(93-117)id(45-45)
tests/unit_tests/test_lobbyconnection.py (3)
tests/integration_tests/conftest.py (2)
ladder_service(51-67)veto_service(79-83)tests/unit_tests/conftest.py (2)
ladder_service(65-81)veto_service(93-97)server/ladder_service/veto_system.py (1)
VetoService(41-254)
tests/integration_tests/test_servercontext.py (2)
tests/integration_tests/conftest.py (1)
veto_service(79-83)tests/unit_tests/conftest.py (1)
veto_service(93-97)
tests/integration_tests/test_server_instance.py (2)
tests/integration_tests/conftest.py (1)
veto_service(79-83)tests/unit_tests/conftest.py (1)
veto_service(93-97)
tests/unit_tests/conftest.py (5)
tests/integration_tests/conftest.py (3)
ladder_service(51-67)violation_service(71-75)veto_service(79-83)server/ladder_service/veto_system.py (1)
VetoService(41-254)tests/conftest.py (3)
player_service(307-310)database(198-200)game_service(346-361)server/player_service.py (2)
PlayerService(41-252)initialize(52-56)server/ladder_service/ladder_service.py (2)
LadderService(71-795)initialize(96-98)
server/lobbyconnection.py (3)
tests/integration_tests/conftest.py (3)
ladder_service(51-67)veto_service(79-83)get(410-414)tests/unit_tests/conftest.py (2)
ladder_service(65-81)veto_service(93-97)server/ladder_service/veto_system.py (2)
VetoService(41-254)set_player_vetoes(128-146)
tests/unit_tests/test_matchmaker_queue.py (2)
server/matchmaker/map_pool.py (2)
MapPool(13-111)MatchmakerQueueMapPool(114-121)server/matchmaker/matchmaker_queue.py (1)
add_map_pool(83-87)
tests/integration_tests/test_game.py (3)
tests/integration_tests/conftest.py (4)
send_message(630-631)lobby_server(310-311)connect_and_sign_in(559-569)read_until_command(532-545)e2e_tests/websocket_protocol.py (1)
send_message(41-48)e2e_tests/fafclient.py (2)
send_message(44-49)read_until_command(61-67)
server/matchmaker/__init__.py (2)
server/matchmaker/map_pool.py (2)
MapPool(13-111)MatchmakerQueueMapPool(114-121)server/matchmaker/matchmaker_queue.py (1)
MatchmakerQueue(46-304)
server/matchmaker/map_pool.py (1)
server/types.py (7)
weight(50-50)Map(55-73)id(45-45)map_pool_map_version_id(47-47)get_map(52-52)get_map(72-73)get_map(129-150)
server/ladder_service/veto_system.py (7)
server/core/service.py (1)
Service(13-54)server/decorators.py (1)
with_logger(15-29)server/exceptions.py (1)
ClientError(10-21)server/matchmaker/map_pool.py (1)
MatchmakerQueueMapPool(114-121)server/player_service.py (1)
all_players(72-73)server/players.py (3)
Player(35-185)write_message(133-143)send_message(121-131)server/types.py (3)
MatchmakerQueueMapPoolVetoData(35-40)id(45-45)map_pool_map_version_id(47-47)
server/matchmaker/matchmaker_queue.py (1)
server/matchmaker/map_pool.py (2)
MapPool(13-111)MatchmakerQueueMapPool(114-121)
tests/integration_tests/conftest.py (2)
server/ladder_service/veto_system.py (1)
VetoService(41-254)server/ladder_service/ladder_service.py (2)
LadderService(71-795)initialize(96-98)
server/ladder_service/ladder_service.py (8)
tests/integration_tests/conftest.py (3)
ladder_service(51-67)violation_service(71-75)veto_service(79-83)server/ladder_service/veto_system.py (3)
VetoService(41-254)update_pools_veto_config(49-90)generate_initial_weights_for_match(170-209)server/matchmaker/map_pool.py (3)
MapPool(13-111)MatchmakerQueueMapPool(114-121)choose_map(80-108)server/matchmaker/matchmaker_queue.py (3)
MatchmakerQueue(46-304)shutdown(279-281)get_map_pool_for_rating(89-97)tests/conftest.py (4)
players(298-303)database(198-200)game_service(346-361)game(209-210)server/players.py (1)
Player(35-185)server/types.py (8)
GameLaunchOptions(23-32)Map(55-73)MapPoolMap(43-52)NeroxisGeneratedMap(79-150)id(45-45)map_pool_map_version_id(47-47)of(93-117)weight(50-50)server/player_service.py (1)
all_players(72-73)
tests/unit_tests/test_map_pool.py (2)
server/types.py (6)
Map(55-73)map_pool_map_version_id(47-47)weight(50-50)NeroxisGeneratedMap(79-150)of(93-117)id(45-45)server/matchmaker/map_pool.py (2)
choose_map(80-108)apply_antirepetition_adjustment(29-78)
🪛 Ruff (0.14.2)
tests/integration_tests/test_game.py
173-173: Avoid specifying long messages outside the exception class
(TRY003)
server/matchmaker/map_pool.py
86-86: Avoid specifying long messages outside the exception class
(TRY003)
108-108: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
server/ladder_service/veto_system.py
135-135: Avoid specifying long messages outside the exception class
(TRY003)
server/ladder_service/ladder_service.py
574-574: Abstract raise to an inner function
(TRY301)
574-574: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: unit-test
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (66)
tests/data/test-data.sql (2)
78-142: LGTM!The new test users and their leaderboard ratings are properly configured to test different rating brackets (951, 952, 1151, 1152), which aligns well with the veto system's rating-based map pool selection.
304-324: LGTM!The test data correctly reflects the new schema changes. The veto configurations are well-designed to test different scenarios:
- Rating-based map pool brackets with varying veto rules
- Different
max_tokens_per_mapvalues including 0 (likely unlimited)server/types.py (3)
32-32: LGTM!Adding
map_pool_map_version_idtoGameLaunchOptionsis appropriate for tracking which map pool map version was selected for a game.
46-47: LGTM!The protocol requires
map_pool_map_version_idas non-Optional whileMapusesOptional[int], which is correct based on learnings. When used in map pool contexts, instances must have a non-None value.Based on learnings.
Also applies to: 61-61
85-85: LGTM!The
map_pool_map_version_idfield is consistently added throughoutNeroxisGeneratedMapand properly propagated through the generation flow.Also applies to: 93-93, 116-116, 149-149, 157-157
tests/integration_tests/test_servercontext.py (1)
62-62: LGTM!Adding
veto_servicemock to the test fixture follows the established pattern for other service dependencies.server/matchmaker/__init__.py (1)
7-7: LGTM!Exposing
MatchmakerQueueMapPoolin the public API is appropriate for the veto system integration.Also applies to: 15-15
tests/integration_tests/test_server_instance.py (1)
36-36: LGTM!The
veto_servicewiring follows the established pattern for service dependency injection in integration tests.Also applies to: 53-54
tests/integration_tests/test_matchmaker.py (1)
48-64: LGTM!The test correctly validates that
map_pool_map_version_idis included in game launch messages and has an expected value from the test data map pool.server/players.py (2)
5-5: LGTM!The logging setup using the
@with_loggerdecorator follows the established pattern in the codebase.Also applies to: 9-9, 11-11, 34-34, 36-36
59-61: LGTM!The lazy import of
PlayerVetoesavoids potential circular dependency issues, and initializing vetoes for all players is a reasonable trade-off for simplicity.server/db/models.py (1)
298-306: Verify that a corresponding Flyway migration exists in the external database repository.This project uses Flyway for database migrations, but migration files are maintained in an external repository (not in
FAForever/server). The schema change tomatchmaker_queue_map_pooladds three non-nullable columns without defaults, which requires a corresponding Flyway migration (.sqlfile) to handle existing data.Confirm that a
V*.sqlmigration file has been created in the FAForever/db repository to accompany this schema change. The test data in this PR has been correctly updated with values for these new columns.tests/unit_tests/test_veto_system.py (2)
12-121: LGTM!The test comprehensively validates
_is_valid_veto_config_for_queueacross various configurations including edge cases such as minimum_maps_after_veto exceeding pool size, zero values, and different token-per-map settings.
123-158: LGTM!The test thoroughly validates
_cap_tokensbehavior across all relevant scenarios, including edge cases where caps are zero or constraints are binding.tests/integration_tests/conftest.py (3)
51-66: LGTM!The
ladder_servicefixture is correctly updated to accept and wireveto_serviceinto theLadderServiceconstructor, matching the updated service signature.
78-83: LGTM!The
veto_servicefixture follows standard async fixture patterns with proper initialization and shutdown lifecycle management.
151-186: LGTM!The
veto_serviceis correctly wired through thelobby_server_factoryfixture, added to the service override dictionary to ensure it's available during lobby server initialization.tests/unit_tests/test_lobbyconnection.py (2)
18-18: LGTM!The import of
VetoServiceis correctly added to support the mock creation in the test fixture.
83-109: LGTM!The
veto_servicemock is correctly added to thelobbyconnectionfixture usingcreate_autospec, following the established pattern for other service dependencies in unit tests.tests/unit_tests/conftest.py (4)
13-15: LGTM!The imports for
VetoServiceandPlayerServiceare correctly added to support the new fixture implementations.
64-81: LGTM!The
ladder_servicefixture is correctly updated to accept and wireveto_serviceinto theLadderServiceconstructor, with all parameters explicitly listed for clarity.
92-97: LGTM!The
veto_servicefixture follows standard async fixture patterns with proper dependency injection and lifecycle management.
20-61: No issues found—the PlayerService mock-to-real transition is necessary and correct.The change is intentional and aligns with the codebase architecture:
VetoServicerequires a realPlayerServiceinstance in its constructor (confirmed inserver/ladder_service/veto_system.py)- The parent fixture
player_serviceintests/conftest.pyalready creates a realPlayerService(database)instanceladder_and_game_service_contextnow creates a realPlayerServicefor consistency, enablingveto_serviceinitialization- Tests do not mock or assert on
player_servicebehavior—they rely on derived fixtures (ladder_service,veto_service)- Proper lifecycle management is maintained with
initialize()andshutdown()callsThe transition is a necessary architectural fix to support the veto system integration, not a regression risk.
server/config.py (1)
140-142: LGTM! Ensure these values are coordinated with the implementation.The configuration changes support the new veto and anti-repetition system:
LADDER_ANTI_REPETITION_LIMITreduced from 2 to 1 (tighter restriction on map repetition)- New threshold list and repeat factor parameters for weight-based anti-repetition logic
Verify that these values align with the implementation in
server/matchmaker/map_pool.pyand related modules.server/__init__.py (3)
129-129: LGTM!The import of
VetoServiceis correctly placed and follows the existing pattern for service imports.
161-161: LGTM!Adding
VetoServiceto__all__appropriately exposes it as part of the public API, consistent with other services likeLadderService,PlayerService, etc.
200-210: LGTM!The dependency injection of
veto_serviceintoLobbyConnectionfollows the established pattern for other services and correctly wires the veto system into the connection factory.server/lobbyconnection.py (1)
56-56: LGTM!The integration of
VetoServiceintoLobbyConnectionfollows the established dependency injection pattern used for other services likegame_service,ladder_service, etc.Also applies to: 113-113, 123-123
tests/integration_tests/test_game.py (3)
171-199: LGTM!The
end_game_as_drawhelper correctly simulates a draw outcome by transitioning all clients to launching state, awaiting launch confirmation, then broadcasting sequential draw results for each army followed by GameEnded.Note: The static analysis hint about "long messages outside exception class" at line 173 can be safely ignored—the message is clear and appropriate for this test utility function.
201-209: LGTM!The
gen_vetoeshelper provides a clean way to generate veto payloads for testing. The format matches the expected structure for theset_player_vetoescommand.
212-226: LGTM!The integration of veto support into
queue_player_for_matchmakingis well-designed. Sending theset_player_vetoesmessage before starting the matchmaking search correctly simulates the veto flow.tests/unit_tests/test_map_pool.py (2)
27-36: LGTM!The test updates correctly reflect the introduction of
map_pool_map_version_idacross Map and NeroxisGeneratedMap structures. The changes are consistent and align with the broader map versioning system introduced in this PR.Also applies to: 42-51, 65-65, 92-101, 112-124, 129-163, 169-175
185-294: LGTM!The parametrized test provides comprehensive coverage of the antirepetition adjustment logic with well-chosen scenarios including:
- Equal play distribution
- Proportional weight redistribution
- Threshold-based behavior
- Repeat factor effects
- Edge cases (empty history, maps not in pool)
- Order independence
The use of
pytest.approxfor floating-point comparisons is appropriate.tests/unit_tests/test_matchmaker_queue.py (2)
11-16: LGTM!The import of
MatchmakerQueueMapPoolis correctly added and follows the existing import style.
294-301: LGTM!The refactoring to use the
MatchmakerQueueMapPoolwrapper simplifies the API and encapsulates pool-to-rating bounds and veto configuration. The changes are consistent across all tests and the default veto values are appropriate for tests that don't exercise veto logic.Also applies to: 310-317, 329-336, 348-355, 376-391
server/matchmaker/map_pool.py (3)
1-11: LGTM!The new imports support the antirepetition adjustment logic and the
MatchmakerQueueMapPooltype.
29-78: Antirepetition algorithm looks solid.The weight transfer logic correctly redistributes probability from played maps to unplayed or less-played maps using threshold-based targeting. The algorithm handles edge cases appropriately, and the use of
notzero_weightsensures division-by-zero is avoided at line 68.
114-121: LGTM!Clean public API type that encapsulates map pool configuration with veto-related metadata.
tests/integration_tests/test_matchmaker_vetoes.py (7)
13-33: Test logic is sound.The veto assignment test correctly verifies that vetoes are adjusted according to pool constraints. The ping/pong workaround at lines 23-25 is an acceptable pattern for ensuring no
vetoes_infomessage is sent when vetoes match expectations.
36-58: LGTM!The test correctly verifies that vetoed maps are excluded from selection across 20 iterations.
60-81: LGTM!The test verifies dynamic token calculation correctly selects map 8 when other maps are vetoed.
83-108: LGTM!The test correctly verifies that partial vetoes allow probabilistic selection of non-fully-vetoed maps. The assertion at line 107 ensures both maps 10 and 11 are selected over 20 iterations.
110-136: LGTM!The test correctly verifies veto behavior in team matchmaking (2v2), ensuring map 10 is selected when maps 9 and 11 are vetoed by player pairs.
138-161: LGTM!The test correctly verifies that pool configuration changes (reducing veto tokens) trigger forced updates and cancel ongoing searches. The try/finally block ensures database state is restored.
163-186: LGTM!The test correctly verifies that map pool changes (map removal) trigger silent veto updates without canceling searches. Database cleanup in the finally block is appropriate.
tests/unit_tests/test_ladder_service.py (4)
24-53: LGTM!The test correctly includes
veto_serviceinLadderServiceinitialization, matching the updated constructor signature.
55-117: LGTM!The test assertions correctly navigate the new
MatchmakerQueueMapPoolstructure usingqueue.map_pools[pool_id][1]to access theMapPool, and verifymap_pool_map_version_idvalues.
442-477: LGTM!The test correctly uses
MatchmakerQueueMapPoolto wrap the map pool configuration, providing all necessary fields includingmap_pool_map_version_id.
854-1025: LGTM!The map selection tests correctly use
mockerto patchchoose_map, and wrap map pools inMatchmakerQueueMapPoolwith appropriate fields. TheMapinstances don't includemap_pool_map_version_idsince they're mocked and never actually used, which is acceptable for these unit tests.server/matchmaker/matchmaker_queue.py (3)
49-70: LGTM!The constructor correctly accepts
MatchmakerQueueMapPoolinstances and stores them bymap_pool.id.
83-87: LGTM!The
add_map_poolmethod correctly accepts and storesMatchmakerQueueMapPoolinstances.
89-97: LGTM!The unpacking pattern correctly extracts
map_pool,min_rating, andmax_ratingfromMatchmakerQueueMapPoolwhile ignoring the id and veto-related fields.server/ladder_service/ladder_service.py (5)
79-92: LGTM!The constructor correctly accepts and stores
veto_service, integrating the veto system into the ladder service.
100-155: LGTM!The
update_datamethod correctly integrates veto system updates. After updating map pools, it callsveto_service.update_pools_veto_configand cancels searches for players whose vetoes were force-adjusted due to configuration changes. This ensures consistency between player vetoes and pool configurations.
156-220: LGTM!The
fetch_map_poolsmethod correctly retrievesmap_pool_map_version_idfrom the database and propagates it throughMapandNeroxisGeneratedMapconstruction.
222-280: LGTM!The
fetch_matchmaker_queuesmethod correctly retrieves veto configuration fields (veto_tokens_per_player,max_tokens_per_map,minimum_maps_after_veto) from the database and includes them in the queue map pool tuples.
635-644: LGTM!The
make_game_optionsfunction correctly includesmap_pool_map_version_idinGameLaunchOptions, enabling version-aware map loading during game launch.server/ladder_service/veto_system.py (9)
1-16: LGTM: Clean imports and type aliases.The imports are well-organized and the type aliases improve code readability throughout the module.
40-47: LGTM: Standard service initialization.The service follows the standard pattern with proper dependency injection and logger setup.
69-69: TODO: Should force adjustment logic be reconsidered?The TODO suggests exploring alternatives to force-adjusting player veto selections. This might impact user experience when pool configs change.
Is this TODO intended to be addressed in this PR or tracked separately? The current implementation removes players from matchmaking if their veto tokens are reduced, which seems reasonable, but the comment suggests there might be concerns about this approach.
92-126: LGTM: Robust config extraction with validation.The method properly validates veto configurations and falls back to safe defaults when invalid setups are detected, with appropriate error logging.
128-146: LGTM: Proper validation and adjustment flow.The method correctly validates incoming veto data, adjusts it to fit constraints, and notifies the player if adjustments were made. The inline error message at line 135 is acceptable despite the static analysis hint—the message is clear and this simple case doesn't warrant a custom exception class.
152-156: TODO: Consider validating instead of adjusting vetoes.The TODO suggests that client-side validation might be preferable to server-side adjustment. This would provide better user feedback about invalid selections.
Should this design decision be addressed now or tracked separately? The current auto-adjustment approach might hide validation errors from users, making it harder for them to understand why their veto selections were changed.
183-209: LGTM: Correct weight calculation with fallback handling.The veto accumulation and weight calculation logic is sound. The dynamic token calculation fallback (lines 192-201) and error handling for impossible setups provide good resilience.
Based on learnings: The validation logic ensures at least some maps always have non-zero weights, so the error case at line 195 should indeed be rare or impossible.
211-254: LGTM: Complex but well-documented algorithm.The dynamic token calculation is mathematically sound. The algorithm correctly groups maps by veto token count and finds the minimum threshold
Tthat satisfies the constraint. The docstring clearly explains the approach, and the special case handling (zero tokens → return 1) is appropriate.
257-335: LGTM: Comprehensive validation and adjustment helpers.All helper functions implement correct validation and adjustment logic:
_is_valid_veto_config_for_queueproperly checks configuration constraints_is_valid_vetoesvalidates data structure and types_adjust_vetoes_for_bracketcorrectly applies token limits_cap_tokensproperly constrains token valuesBased on learnings: The assertions about non-negative values are guaranteed by external constraints.
|
Hi @K-ETFreeman, I can have a go at reviewing your CR this weekend. I'm a bit busy and might forget about this. If this happens please do ping me on discord (@GuteAlteKanone) next week. Since I'm new to this repository could you do the following things until the weekend?
|
This PR adds veto system to the server. i attach the guide to the matchmaker team (its a bit outdated in anti-repetition part tho), so you can better to understand the context tests are written in C:\FreemanProduction\faf\server\tests\integration_tests\test_matchmaker_vetoes.py diagram looks fine.
all the inner logic is hidden in veto_system.py |
|
Also about # TODO comments (by Askaholic): even if we try to remove immediate check (and only check when match found) that will lead to some confusion:
so if we start to only checking vetoes on match start, this will lead to: so i think current approach is correct. if you agree with me, free to delete those TODO comments |
| return f"MapPool({self.id}, {self.name}, {list(self.maps.values())})" | ||
|
|
||
|
|
||
| class MatchmakerQueueMapPool(NamedTuple): |
There was a problem hiding this comment.
It's not obvious to me what this class is supposed to do. I think to understand that I'd have to read the full PR first.
It seems to connect a map pool with information that the veto system needs. Why do we need this class? Why is this not either a part of the MapPool or of the MatchmakerQueue itself?
edit: I just saw that matchmaker_queue_map_pool in db/models.py already existed before your PR.
If we think we need this class, then please improve:
- The naming of the class. The current name concatenates the names of two existing classes. But how does this relate to MatchmakerQueue-s? That's not obvious to me. edit: if this stays the only finding of my review, let's not change the name. If I ask you to change other parts of the CR too and you need to test that anyways, then I'd say let's also rename this thing.
- give it a docstring
There was a problem hiding this comment.
I've pasted some parts of your PR into Claude sonnet and asked it to come up with more concrete suggestions. Here's the output. Please double-check whether this even makes sense and is not completely hallucinated. The LLMs are good at generating stuff that sounds convincing but might be subtly or even fundamentally wrong. Often they're subtly wrong and it's easy to miss that.
| class MatchmakerQueueMapPool(NamedTuple): | |
| class QueueMapPoolConfig(NamedTuple): | |
| """ | |
| Configuration for using a map pool in a matchmaker queue. | |
| Defines rating-based restrictions and veto system parameters for how | |
| a map pool should be used within a specific queue. A single MapPool | |
| can be reused across multiple queues with different configurations. | |
| This corresponds to a row in the `matchmaker_queue_map_pool` database table. | |
| Attributes: | |
| id: Database ID of this configuration (matchmaker_queue_map_pool.id) | |
| map_pool: The map pool to use | |
| min_rating: Minimum displayed rating to use this map pool (None = no limit) | |
| max_rating: Maximum displayed rating to use this map pool (None = no limit) | |
| veto_tokens_per_player: Number of maps each player can veto | |
| max_tokens_per_map: Maximum veto tokens that can be applied to any single map | |
| minimum_maps_after_veto: Minimum number of maps that must remain selectable after vetoes | |
| """ |
There was a problem hiding this comment.
yes as you saw later this is just representation of the DB entity
https://faforever.github.io/db/tables/matchmaker_queue_map_pool.html
map_pool too
https://faforever.github.io/db/tables/map_pool.html
the name is intentionally the same to link them, i dont understand why it should be changed
MapPool has the same name as its DB representation, so MatchmakerQueueMapPool should be MatchmakerQueueMapPool as well, not QueueMapPoolConfig
why needed? to not have code repetition
its used in multiple places, if you remove that -> you'll need to write tuple of 7 elements instead of single word of this class
There was a problem hiding this comment.
why needed? to not have code repetition
its used in multiple places, if you remove that -> you'll need to write tuple of 7 elements instead of single word of this class
I meant "what purpose does it serve"? :) I think I got it now though.
| Column("map_pool_id", Integer, ForeignKey("map_pool.id"), nullable=False), | ||
| Column("min_rating", Integer), | ||
| Column("max_rating", Integer), | ||
| Column("id", Integer, primary_key=True), |
There was a problem hiding this comment.
Regarding my earlier comment. I just saw that this was not introduced by your PR but instead already existed in the code base.
I pasted your version of this definition into claude sonnet and prompted to generate a comment for the review, based on my earlier comment. This is its output:
The table name matchmaker_queue_map_pool makes it look like a simple join table, but it's actually a first-class entity with its own attributes (rating ranges, veto configuration). This deserves a more descriptive name that reflects what it represents in the domain: a configured assignment of a map pool to a queue.
Consider renaming to something like queue_map_pool_config or map_pool_assignment to better communicate its purpose. At minimum, please add a comment explaining what this table represents.
Code Suggestion:
# Configures how a map pool is used within a matchmaker queue.
# Each row represents an assignment of a map pool to a queue with specific
# parameters for rating eligibility and the veto system. A queue can have
# multiple assignments (e.g., different map pools for different rating ranges),
# and a map pool can be assigned to multiple queues with different configurations.
matchmaker_queue_map_pool = Table(
"matchmaker_queue_map_pool", metadata,
Column("id", Integer, primary_key=True),
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), # Minimum rating to use this map pool assignment
Column("max_rating", Integer), # Maximum rating to use this map pool assignment
Column("veto_tokens_per_player", Integer, nullable=False), # Number of maps each player can veto
Column("max_tokens_per_map", Integer, nullable=False), # Max veto tokens applicable to one map
Column("minimum_maps_after_veto", Float, nullable=False), # Minimum maps remaining after vetoes
)
There was a problem hiding this comment.
Since this already existed before your PR I'd say let's not rename this, but definitely do add the source comment. First check whether it makes sense though. The LLMs are good at generating stuff that sounds convincing but might be subtly or even fundamentally wrong.
There was a problem hiding this comment.
https://faforever.github.io/db/tables/matchmaker_queue_map_pool.html
this is just database entity
i am not sure why any comment is needed here because no other table has this sorta comments either
There was a problem hiding this comment.
Because the other database entities have descriptive names. This name is not helpful. Instead of renaming it, I'd suggest simply adding a source comment what this is for.
| veto_tokens_per_player: int = 0 | ||
| max_tokens_per_map: float = 0 | ||
| minimum_maps_after_veto: float = 1 |
There was a problem hiding this comment.
This is duplicated with
class MatchmakerQueueMapPoolVetoData(NamedTuple):
...
veto_tokens_per_player: int
max_tokens_per_map: float
minimum_maps_after_veto: float
Can we get rid of one of them?
There was a problem hiding this comment.
First thought: Maybe remove this from MatchmakerQueueMapPoolVetoData and there only keep
matchmaker_queue_map_pool_id and map_pool_map_version_ids?
There was a problem hiding this comment.
First thought: Maybe remove this from MatchmakerQueueMapPoolVetoData and there only keep matchmaker_queue_map_pool_id and map_pool_map_version_ids?
no, this wont work
MatchmakerQueueMapPoolVetoData is a data structure that extracts only the veto-relevant information from MatchmakerQueueMapPool. It's used by the VetoService for efficient veto validation and adjustment.
you can look at line 57 in veto_system.py:
pools_vetodata = self.extract_pools_veto_config(queues)
if self.pools_veto_data != pools_vetodata:
... update stuff
pools_vetodata is list of MatchmakerQueueMapPoolVetoData
and when we do self.pools_veto_data != pools_vetodata we check, if anything veto-related was changed in the pool. If was changed -> we're doing some actions, if not -> we're not
These are fundamental to MatchmakerQueueMapPoolVetoData and cannot be removed. You cannot directly replace it with MatchmaerQueueMapPool either without overcomplicating everything:
is it possible to replace it with MatchmakerQueueMapPool by any chance? if yes, what fixes that will require to preserve the same behaviour?
⎿ ⧉ Selected 1 lines from server\ladder_service\veto_system.py in Visual Studio Code● Looking at the code, I can see why MatchmakerQueueMapPoolVetoData exists as a separate structure. Let me analyze if replacement is feasible.
● Search(pattern: "self.pools_veto_data", path: "server\ladder_service\veto_system.py", output_mode: "content")
⎿ Found 5 lines (ctrl+o to expand)● Short answer: It's technically possible but not recommended. The current design is intentional and solves several important problems.
Key Issues with Using MatchmakerQueueMapPool
- Equality Comparison Breaks (line 59)
if self.pools_veto_data != pools_vetodata:
Problem: MatchmakerQueueMapPool contains a MapPool object, which has:
- A mutable dict: self.maps = {map_.id: map_ for map_ in maps}
- This makes equality comparison unreliable
Impact: The change detection would fail because:
- Two different MapPool instances are never equal (even with same data)
- The condition would always be True, triggering unnecessary updates every time
- All players would get forced veto adjustments on every config refresh
- Tuple Unpacking Requires Major Refactoring (line 158)
Current: unpacks 5 fields in specific order
for bracket_id, map_ids, total_tokens, max_per_map, _ in self.pools_veto_data:
Would become: unpacks 7 fields, different order, need to extract map IDs
for pool_config in self.pools_veto_data:
bracket_id = pool_config.id
map_ids = [m.map_pool_map_version_id for m in pool_config.map_pool.maps.values()]
total_tokens = pool_config.veto_tokens_per_player
max_per_map = pool_config.max_tokens_per_map
- Map ID Extraction Everywhere (line 64)
Current: direct access
pool_maps_by_bracket = {
pool_data.matchmaker_queue_map_pool_id: set(pool_data.map_pool_map_version_ids)
for pool_data in self.pools_veto_data
}Would become: repeated extraction
pool_maps_by_bracket = {
pool_data.id: set(m.map_pool_map_version_id for m in pool_data.map_pool.maps.values())
for pool_data in self.pools_veto_data
}
- Memory and Complexity
- Stores unnecessary fields (min_rating, max_rating)
- Keeps full MapPool objects in memory (with all map data)
- Creates tight coupling between veto logic and map pool structure
What Would Be Required
If you really wanted to do this, you'd need:
- Replace equality check with manual field comparison:
def _veto_configs_equal(old_pools, new_pools):
if len(old_pools) != len(new_pools):
return False
for old, new in zip(old_pools, new_pools):
if (old.id != new.id or
old.veto_tokens_per_player != new.veto_tokens_per_player or
old.max_tokens_per_map != new.max_tokens_per_map or
old.minimum_maps_after_veto != new.minimum_maps_after_veto or
set(m.map_pool_map_version_id for m in old.map_pool.maps.values()) !=
set(m.map_pool_map_version_id for m in new.map_pool.maps.values())):
return False
return Trueif not _veto_configs_equal(self.pools_veto_data, pools_vetodata):
- Update all tuple unpacking
- Extract map IDs everywhere they're needed
- Update type hints and field access patterns
Why Current Design is Better
The MatchmakerQueueMapPoolVetoData structure is a value object pattern that:
- ✅ Is immutable and comparable
- ✅ Contains only veto-relevant data
- ✅ Enables reliable change detection
- ✅ Decouples veto logic from MapPool implementation
- ✅ Is lightweight for caching
Verdict: The current design is correct. The data duplication is intentional and beneficial. Replacing it would add complexity, reduce
reliability, and violate separation of concerns.
There was a problem hiding this comment.
Basically, MatchmakerQueueMapPool exists because its representation of what we get from the DB (full info about the mappool + his matchmaker-and-veto settings), MatchmakerQueueMapPoolVetoData is distilled version of it which contains only veto-related entities which simplifies a bunch of stuff
There was a problem hiding this comment.
Interesting. Thanks for the explanation. :)
server/ladder_service/veto_system.py
Outdated
| BracketID = int | ||
| MapPoolMapVersionId = int | ||
| VetoTokensApplied = int | ||
| VetosMap = dict[MapPoolMapVersionId, VetoTokensApplied] |
There was a problem hiding this comment.
nit: consider turning these into classes such that type checking can distinguish between them. Since e.g. the first three are int, I assume mypy can't distinguish them.
There was a problem hiding this comment.
You right that mypy can't distinguish them
But these are just for the readability purposes
Can be straight up deleted but reading dict[int, dict[int,int]] can be not very pleasant experience
Same pattern is already used in typedefs.py
turning these into classes - you mean NewType?
i dont see it used anywhere in the codebase, and looks like its an overkill here
more importantly: why its "VetosMap" when it should be "VetoesMap"? the real bug found
There was a problem hiding this comment.
I mean something like
class BracketID(int):
pass
The change is trivial and the advantage is as I said that mypy can help you find errors early.
| from server.players import Player | ||
| from server.types import MatchmakerQueueMapPoolVetoData | ||
|
|
||
| BracketID = int |
There was a problem hiding this comment.
You right, its not obvious
I guess i'll just replace it with MatchmakerQueueMapPoolId
There was a problem hiding this comment.
or maybe not. bracket is simpler. ugh
There was a problem hiding this comment.
i guess i just explain the connection in the doc
its "Bracket" because MatchmakerQueueMapPoolId has range of ratings (and maplist), so like "Rating bracket" -> bracket
There was a problem hiding this comment.
Yeah, I think the whole matchmaker_queue_map_pool thing in the DB was badly named in the beginning. As you say, it should have been Bracket all along. That makes way more sense. But let's not go ahead and do all kinds of renaming stuff. That's not worth it. Since the documentation you wrote/updated explains this, we're fine.
| @@ -0,0 +1,335 @@ | |||
| import logging | |||
There was a problem hiding this comment.
Please add a reference to https://github.com/user-attachments/files/23255986/veto-system-tutorial.html as a module docstring. Also from how I read that document, what we're implementing actually isn't a veto system. A veto is an "unilaterally stopping an action". We don't do that here. Naming it a "veto system" is confusing, I'd change that in the announcement. Ideally also in the code.
This is more of a preference system were players can influence which maps will be made less likely. But they don't have an actual veto vote. Which totally makes sense, given that our matchmaking queues don't have hundreds of players.
Brutus5000
left a comment
Brutus5000
left a comment
There was a problem hiding this comment.
Even though I don't understand it in detail, this PR looks solid to me.
Please fix the 2 minor things, then I am willing to merge it.
In case further review findings of other reviewers, we can still fix them afterwards.
|
veto-system-developer-guide.html i will put it here for future generations |
|
|
||
| self.LADDER_1V1_OUTCOME_OVERRIDE = True | ||
| self.LADDER_ANTI_REPETITION_LIMIT = 2 | ||
| self.LADDER_ANTI_REPETITION_LIMIT = 1 |
There was a problem hiding this comment.
You said there were issues with this being 2. Add a unit test which: enforces this being 1 until the underlying issue is fixed. Add a short description to the unit test of what the issue was.
|
Do add a link to this to
|
4 participants
the work is done, it seems
Summary by CodeRabbit
New Features
Improvements
Configuration Updates