Resolve issue with duo/trio/quad team forming logic - #4758
Conversation
WalkthroughTeam assignment now receives explicit Duos, Trios, and Quads context. Human placement prefers fuller teams in these modes. Game assignment and lobby previews pass the mode context. Tests cover existing behavior and nation placement. ChangesTeam packing behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/game/TeamAssignment.ts (1)
107-120: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPut friend groups before friendless humans.
The new tie-breaker cannot preserve a friend pair when a friendless human is processed first. In Duos,
[friendless, A↔B]fills the first team withfriendless + A, forcingBonto another team. Partition humans with present friends before friendless humans before placement, and add this regression case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/game/TeamAssignment.ts` around lines 107 - 120, The team-placement flow in TeamAssignment must prioritize humans with existing friends before friendless humans. Partition or order the input before placement so friend-connected players are processed first, then add a regression case covering a friendless player preceding an A↔B pair and verify the pair remains together.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/core/game/TeamAssignment.ts`:
- Around line 107-120: The team-placement flow in TeamAssignment must prioritize
humans with existing friends before friendless humans. Partition or order the
input before placement so friend-connected players are processed first, then add
a regression case covering a friendless player preceding an A↔B pair and verify
the pair remains together.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: be14334f-cc5f-4e20-9b93-807172f79f7a
📒 Files selected for processing (4)
src/client/components/LobbyPlayerView.tssrc/core/game/GameImpl.tssrc/core/game/TeamAssignment.tstests/TeamAssignment.test.ts
|
Looking at the review from coderabbit, I can see this is outside the scope of the original ticket because it's behaviour that was already present in the code before I made changes. I can update the PR to include a fix to this flagged issue also, if this is preferred? |
| export function assignTeamsLobbyPreview( | ||
| players: PlayerInfo[], | ||
| teams: Team[], | ||
| isDuosTriosQuads: boolean, |
There was a problem hiding this comment.
instead let's pass in playerTeams directly here.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/game/TeamAssignment.ts (1)
107-107: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPlace players with present friends before friendless players.
The new tie-breaker only selects a fuller team for the current player. It does not reserve a slot for a later friend. With two teams and
maxTeamSize = 2, input[friendless, A, B]assignsfriendlessandAtogether, then sends friendBto the other team. The placement loop preservesotherPlayersorder before adding nations. (raw.githubusercontent.com)When
isDuosTriosQuadsis true, partitionotherPlayersinto players with present friends and friendless players. Keep the friend-connected group first. Preserve the existing order for explicitly numbered team counts. Add a regression test for the friendless-first case.Suggested ordering change
+ const hasPresentFriend = (p: PlayerInfo): boolean => + p.clientID !== null && (friendGraph.get(p.clientID)?.size ?? 0) > 0; + const friendConnectedPlayers = otherPlayers.filter(hasPresentFriend); + const friendlessPlayers = otherPlayers.filter((p) => !hasPresentFriend(p)); + const orderedOtherPlayers = isDuosTriosQuads + ? friendConnectedPlayers.concat(friendlessPlayers) + : otherPlayers; - for (const p of otherPlayers.concat(nationPlayers)) { + for (const p of orderedOtherPlayers.concat(nationPlayers)) {Based on the PR objective, friend pairs must precede friendless humans. As per coding guidelines, add tests for this
src/core/change.Also applies to: 119-120
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/game/TeamAssignment.ts` at line 107, Update the player ordering used by the placement loop in TeamAssignment so that, when isDuosTriosQuads is true, players with present friends are processed before friendless players while preserving each group’s existing order; leave explicitly numbered team counts unchanged. Add a regression test covering the friendless-first input case to verify friends are placed together.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/core/game/TeamAssignment.ts`:
- Line 107: Update the player ordering used by the placement loop in
TeamAssignment so that, when isDuosTriosQuads is true, players with present
friends are processed before friendless players while preserving each group’s
existing order; leave explicitly numbered team counts unchanged. Add a
regression test covering the friendless-first input case to verify friends are
placed together.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 09007994-adad-4383-b27c-33f9aafff7c1
📒 Files selected for processing (2)
src/client/components/LobbyPlayerView.tssrc/core/game/TeamAssignment.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/components/LobbyPlayerView.ts
2 participants
Add approved & assigned issue number here:
Resolves #4253
Description:
This resolves an issue with duos/trios/quads where humans would be automatically assigned to the emptiest teams, thereby resulting in combinations of humans + nations fighting each other, whereas the preferred behaviour is for humans to be put together where possible.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
sjg.