Skip to content

Conversation

@kraktus
Copy link
Member

@kraktus kraktus commented Jan 24, 2024

In atomic, if you exploded a rook, the castling rights were updated correctly but not the unmovedRooks, so if later one of your piece was taken on the same square (eg: white queen taken on H8), you would lose castling right on that side (White lost kingside castling after H8 capture).

The source of the bug is below, where the unmovedRooks from before were copied, which does not make sense? And in atomic resulted in an issue because the explosion code updated the after Board.

         val h2 = h1.copy(
           lastMove = Option(toUci),
-          unmovedRooks = before.unmovedRooks,
           halfMoveClock =
             if piece.is(Pawn) || captures || promotes then HalfMoveClock.initial
             else h1.halfMoveClock + 1
         )

The following change was unnecessary, but conceptually it makes more sense to have the unmovedRooks and castling rights copied from the original history. It should also prevent.

var castleRights: Castles      = h1.castles
var unmovedRooks: UnmovedRooks = h1.unmovedRooks

As the TODO say Move.finalizeAfter is very brittle, but more generally Move seems to contain far more information than it should, resulting in very weak typing because many functions are needed to "finalise", "finalise-after", etc, using many copies instead of types.

I put two tests, one specific but also the game in question until the unavailable castling to prevent larger regressions.

I have not run perft, will wait for CI.

close lichess-org/lila#14544

Should the tostring value be overriden instead?
In atomic, if you exploded a rook, the castling rights were updated correctly but not the `unmovedRooks`, so if later one of your piece was taken on the same square (eg: white queen taken on H8), you would lose castling right on that side (White lost kingside castling after H8 capture).

The source of the bug is below, where the unmovedRooks from before were copied, which does not make sense? And in atomic resulted in an issue because the explosion code updated the `after` `Board`.
```diff
         val h2 = h1.copy(
           lastMove = Option(toUci),
-          unmovedRooks = before.unmovedRooks,
           halfMoveClock =
             if piece.is(Pawn) || captures || promotes then HalfMoveClock.initial
             else h1.halfMoveClock + 1
         )
```

The following change was unnecessary, but conceptually it makes more sense to have the unmovedRooks and castling rights copied from the original history. It should also prevent.
```
var castleRights: Castles      = h1.castles
var unmovedRooks: UnmovedRooks = h1.unmovedRooks
```

As the TODO say `Move.finalizeAfter` is very brittle, but more generally `Move` seems to contain far more information than it should, resulting in very weak typing because many functions are needed to "finalise", "finalise-after", etc, using many copies instead of types.

I put two tests, one specific but also the game in question until the unavailable castling to prevent larger regressions.

I have not run perft, will wait for CI.

close lichess-org/lila#14544
@kraktus
Copy link
Member Author

kraktus commented Jan 24, 2024

cc @lenguyenthanh It looks like CI is not doing perft either?

@lenguyenthanh
Copy link
Member

lenguyenthanh commented Jan 24, 2024

cc @lenguyenthanh It looks like CI is not doing perft either?

We run perft but with nodeLimit is 1000: https://github.com/lichess-org/scalachess/blob/master/test-kit/src/test/scala/perft/PerftTest.scala#L17

I have full perft tests here: https://github.com/lenguyenthanh/scalachess-tests

@lenguyenthanh lenguyenthanh merged commit ecdb12b into lichess-org:master Jan 24, 2024
@ornicar
Copy link
Collaborator

ornicar commented Jan 24, 2024

should I release a scalachess version for lila to use?

@kraktus
Copy link
Member Author

kraktus commented Jan 24, 2024

Yes 👍

niklasf added a commit to niklasf/chessops that referenced this pull request Jan 24, 2024
@lenguyenthanh
Copy link
Member

all perft passed: https://github.com/lenguyenthanh/scalachess-tests

lenguyenthanh added a commit to lichess-org/lila that referenced this pull request Jan 25, 2024
lenguyenthanh added a commit to lichess-org/lila-ws that referenced this pull request Jan 25, 2024
@lenguyenthanh
Copy link
Member

@ornicar I published 15.7.6 and pushed the new version to lila and lila-ws

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Atomic: Unable to castle

3 participants