Conversation
There was a problem hiding this comment.
Pull request overview
Refactors match adjudication by extracting the individual adjudication trackers into dedicated headers and introducing a centralized Adjudicator used by Match.
Changes:
- Replaced per-tracker state in
Matchwith a singleAdjudicatormember. - Moved
DrawTracker,ResignTracker,MaxMovesTracker, andTbAdjudicationTrackerintomatchmaking/adjudication/. - Routed tracker updates and adjudication decisions through
AdjudicatorfromMatch::playMove/Match::adjudicate.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/matchmaking/match/match.hpp | Removes embedded tracker classes and stores a single Adjudicator; adds adjudication header include. |
| app/src/matchmaking/match/match.cpp | Constructs and uses Adjudicator for tracker updates and adjudication decisions. |
| app/src/matchmaking/adjudication/adjudicator.hpp | Adds new centralized adjudication orchestrator (TB/resign/draw/max-moves). |
| app/src/matchmaking/adjudication/draw_tracker.hpp | Extracted draw adjudication tracker into its own header. |
| app/src/matchmaking/adjudication/resign_tracker.hpp | Extracted resign adjudication tracker into its own header. |
| app/src/matchmaking/adjudication/max_moves_tracker.hpp | Extracted max-moves adjudication tracker into its own header. |
| app/src/matchmaking/adjudication/tb_adjudication_tracker.hpp | Extracted Syzygy tablebase adjudication tracker into its own header. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| int resign_moves_white = 0; | ||
|
|
||
| // config | ||
| // the score muust be above this threshold to resign |
There was a problem hiding this comment.
Typo in comment: "muust" → "must".
| // the score muust be above this threshold to resign | |
| // the score must be above this threshold to resign |
| void update(const std::optional<engine::Score>& score, const chess::Board& board, | ||
| const chess::Color side_to_move) noexcept { |
There was a problem hiding this comment.
Adjudicator::update takes side_to_move even though the board already contains board.sideToMove(). This makes it possible for callers to accidentally pass a mismatched value and corrupt adjudication state; consider removing the parameter and using board.sideToMove() internally (or at least assert they match).
| // If result is WIN (for side to move), then side to move wins. | ||
| // In Match::adjudicate(us, them), 'us' is the one NOT to move (passed as first arg). | ||
| // Wait, let's keep it simple. | ||
| // The TB result is relative to the side to move (board.sideToMove()). | ||
|
|
There was a problem hiding this comment.
The TB adjudication block contains a long “thinking out loud” narrative (e.g., "Wait, let's keep it simple" / multiple contradictory explanations). This makes the header hard to maintain and invites future confusion; please replace it with a short, definitive explanation of the us/them contract and the WIN/LOSE mapping.
| [[nodiscard]] std::optional<Result> adjudicate(const chess::Board& board, | ||
| const std::optional<engine::Score>& score) const noexcept { | ||
| // Start with TB adjudication | ||
| if (tb_config_.enabled && tb_adjudication_tracker_.adjudicatable(board)) { |
There was a problem hiding this comment.
New adjudication behavior is now centralized in Adjudicator::adjudicate, but there are no unit tests covering the cross-tracker decision logic and the result_us mapping. Please add doctest coverage for at least: TB WIN/LOSE mapping vs board.sideToMove(), resign adjudication, and precedence (TB > resign > draw > maxmoves).
| @@ -9,141 +9,9 @@ | |||
| #include <matchmaking/syzygy.hpp> | |||
There was a problem hiding this comment.
#include <matchmaking/syzygy.hpp> appears to be unused in this header after moving the TB tracker into matchmaking/adjudication/. Dropping it would reduce compile-time coupling (and avoid pulling in tablebase-related dependencies from every Match include).
| #include <matchmaking/syzygy.hpp> |
No description provided.