Keeping dice is a set, not a list

Holding dice did nothing unless they happened to be tapped left to right.

The screen appends each die as it is tapped, so holding the third and then the
first produced [2, 0]. legalMoves only ever generated ascending subsets, and
RollDice is a data class over a List, so [2, 0] was a different move from
[0, 2] and simply was not among the legal ones. MatchController drops a move it
cannot find there, silently -- hence a highlighted die, a Throw that did
nothing, and no error to go on.

A keep is a set of positions. Holding the third die and then the first is the
same act as holding the first and then the third, and a set says so: the two
are now equal, and the order they were built up in cannot matter. That kills
the whole class of bug rather than sorting at the one call site that happened
to show it.

Positions rather than faces still stands -- two fives are two different dice.

Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01Urn6eEBA6XzrnfoibL7faC

app/src/main/java/org/prolibertate/games/game/yahtzee/YahtzeeAi.kt

@@ -28,9 +28,8 @@ object YahtzeeAi : GameAi<YahtzeeState, YahtzeeMove> {
         if (rolls.isEmpty()) return best ?: writes.first()
         if (bestScore >= WORTH_KEEPING) return best!!
 
-        val keep = whatToKeep(state.dice)
+        val keep = whatToKeep(state.dice).toSet()
-        return rolls.firstOrNull { it.keep.sorted() == keep.sorted() }
+        return rolls.firstOrNull { it.keep == keep } ?: rolls.first()
-            ?: rolls.first()
     }
 
     /**

app/src/main/java/org/prolibertate/games/game/yahtzee/YahtzeeModel.kt

@@ -132,10 +132,16 @@ sealed interface YahtzeeMove
  * Positions rather than faces: two fives are two different dice, and a player
  * who keeps one of them and rerolls the other is doing something a set of faces
  * could not express.
+ *
+ * A set rather than a list, because holding the third die and then the first is
+ * the same act as holding the first and then the third. As a list those were two
+ * different moves, and only the ascending one was ever offered as legal — so
+ * keeping dice in any other order was silently dropped and the throw did
+ * nothing.
  */
 @Serializable
 @SerialName("roll")
-data class RollDice(val keep: List<Int> = emptyList()) : YahtzeeMove
+data class RollDice(val keep: Set<Int> = emptySet()) : YahtzeeMove
 
 @Serializable
 @SerialName("score")

app/src/main/java/org/prolibertate/games/game/yahtzee/YahtzeeRules.kt

@@ -53,7 +53,7 @@ object YahtzeeRules : GameRules<YahtzeeState, YahtzeeMove> {
             // The first throw of a turn is all five dice: there is nothing on
             // the table yet to keep.
             moves += if (!state.hasRolled) {
-                listOf(RollDice(emptyList()))
+                listOf(RollDice(emptySet()))
             } else {
                 keepSubsets().map { RollDice(it) }
             }
@@ -67,9 +67,9 @@ object YahtzeeRules : GameRules<YahtzeeState, YahtzeeMove> {
     }
 
     /** Every set of dice positions a player might hold back — thirty-two of them. */
-    private fun keepSubsets(): List<List<Int>> =
+    private fun keepSubsets(): List<Set<Int>> =
         (0 until (1 shl DICE_COUNT)).map { mask ->
-            (0 until DICE_COUNT).filter { mask and (1 shl it) != 0 }
+            (0 until DICE_COUNT).filter { mask and (1 shl it) != 0 }.toSet()
         }
 
     override fun applyMove(state: YahtzeeState, seat: Int, move: YahtzeeMove): YahtzeeState {
@@ -83,7 +83,6 @@ object YahtzeeRules : GameRules<YahtzeeState, YahtzeeMove> {
 
     private fun applyRoll(state: YahtzeeState, move: RollDice): YahtzeeState {
         require(state.rollsUsed < ROLLS_PER_TURN) { "No rolls left this turn" }
-        require(move.keep.distinct().size == move.keep.size) { "Cannot keep a die twice" }
         require(move.keep.all { it in 0 until DICE_COUNT }) { "No such die" }
         require(state.hasRolled || move.keep.isEmpty()) { "Nothing to keep before the first roll" }
         return state.copy(
@@ -92,7 +91,7 @@ object YahtzeeRules : GameRules<YahtzeeState, YahtzeeMove> {
         )
     }
 
-    private fun throwDice(state: YahtzeeState, keep: List<Int>): List<Int> {
+    private fun throwDice(state: YahtzeeState, keep: Set<Int>): List<Int> {
         val random = Random(
             state.seed +
                 state.round * 10_000L +

app/src/main/java/org/prolibertate/games/ui/game/YahtzeeScreen.kt

@@ -113,7 +113,7 @@ fun YahtzeeScreen(
             if (yours && canRoll) {
                 Button(
                     onClick = {
-                        controller.submit(RollDice(kept.toList()))
+                        controller.submit(RollDice(kept.toSet()))
                         // What was kept applied to the throw just made; the next
                         // one starts from whatever is on the table now.
                         kept.clear()

app/src/test/java/org/prolibertate/games/game/yahtzee/YahtzeeRulesTest.kt

@@ -118,7 +118,7 @@ class YahtzeeRulesTest {
         val state = YahtzeeRules.initialState(config())
         assertEquals(0, YahtzeeRules.currentSeat(state))
         val legal = YahtzeeRules.legalMoves(state, 0)
-        assertEquals(listOf(RollDice(emptyList())), legal)
+        assertEquals(listOf(RollDice(emptySet())), legal)
         // Nothing may be written in before the dice have been thrown.
         assertThrows { YahtzeeRules.applyMove(state, 0, ScoreIn(YahtzeeCategory.CHANCE)) }
     }
@@ -142,15 +142,15 @@ class YahtzeeRulesTest {
         val again = YahtzeeRules.applyMove(state, 0, RollDice())
         assertEquals(once.dice, again.dice)
 
-        val keptOnce = YahtzeeRules.applyMove(once, 0, RollDice(listOf(0, 1)))
+        val keptOnce = YahtzeeRules.applyMove(once, 0, RollDice(setOf(0, 1)))
-        val keptAgain = YahtzeeRules.applyMove(once, 0, RollDice(listOf(0, 1)))
+        val keptAgain = YahtzeeRules.applyMove(once, 0, RollDice(setOf(0, 1)))
         assertEquals(keptOnce.dice, keptAgain.dice)
     }
 
     @Test
     fun `kept dice stay put and the rest are thrown again`() {
         val state = YahtzeeRules.applyMove(YahtzeeRules.initialState(config()), 0, RollDice())
-        val kept = YahtzeeRules.applyMove(state, 0, RollDice(listOf(0, 2, 4)))
+        val kept = YahtzeeRules.applyMove(state, 0, RollDice(setOf(0, 2, 4)))
         assertEquals(state.dice[0], kept.dice[0])
         assertEquals(state.dice[2], kept.dice[2])
         assertEquals(state.dice[4], kept.dice[4])
@@ -169,10 +169,40 @@ class YahtzeeRulesTest {
         assertTrue(YahtzeeRules.legalMoves(state, 0).all { it is ScoreIn })
     }
 
+    @Test
+    fun `the order dice were tapped in does not matter`() {
+        // The screen adds each die as it is tapped, so holding the third and
+        // then the first once gave [2, 0]. As a list that was a different move
+        // from [0, 2], and only the ascending one was ever offered as legal, so
+        // the controller dropped it and the throw did nothing at all.
+        val state = YahtzeeRules.applyMove(YahtzeeRules.initialState(config()), 0, RollDice())
+        assertEquals(RollDice(setOf(2, 0)), RollDice(setOf(0, 2)))
+
+        val legal = YahtzeeRules.legalMoves(state, 0)
+        assertTrue(
+            "a keep must be legal however it was built up",
+            legal.contains(RollDice(setOf(2, 0))),
+        )
+        // And it has to actually hold those dice.
+        val thrown = YahtzeeRules.applyMove(state, 0, RollDice(setOf(2, 0)))
+        assertEquals(state.dice[0], thrown.dice[0])
+        assertEquals(state.dice[2], thrown.dice[2])
+        assertEquals(2, thrown.rollsUsed)
+    }
+
+    @Test
+    fun `every way of keeping dice is on offer`() {
+        // Thirty-two subsets of five dice, and each of them exactly once.
+        val state = YahtzeeRules.applyMove(YahtzeeRules.initialState(config()), 0, RollDice())
+        val rolls = YahtzeeRules.legalMoves(state, 0).filterIsInstance<RollDice>()
+        assertEquals(32, rolls.size)
+        assertEquals(32, rolls.map { it.keep }.distinct().size)
+    }
+
     @Test
     fun `nothing may be kept before the first throw`() {
         val state = YahtzeeRules.initialState(config())
-        assertThrows { YahtzeeRules.applyMove(state, 0, RollDice(listOf(0))) }
+        assertThrows { YahtzeeRules.applyMove(state, 0, RollDice(setOf(0))) }
     }
 
     @Test
@@ -277,7 +307,7 @@ class YahtzeeRulesTest {
         val state = YahtzeeRules.applyMove(YahtzeeRules.initialState(config()), 0, RollDice())
         assertEquals(state, YahtzeeRules.decodeState(YahtzeeRules.encodeState(state)))
 
-        val roll: YahtzeeMove = RollDice(listOf(0, 3))
+        val roll: YahtzeeMove = RollDice(setOf(0, 3))
         assertEquals(roll, YahtzeeRules.decodeMove(YahtzeeRules.encodeMove(roll)))
         val write: YahtzeeMove = ScoreIn(YahtzeeCategory.LARGE_STRAIGHT)
         assertEquals(write, YahtzeeRules.decodeMove(YahtzeeRules.encodeMove(write)))