-
-
Notifications
You must be signed in to change notification settings - Fork 224
Atomic: fix castling rights #509
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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
|
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 |
|
should I release a scalachess version for lila to use? |
|
Yes 👍 |
|
all perft passed: https://github.com/lenguyenthanh/scalachess-tests |
|
@ornicar I published 15.7.6 and pushed the new version to lila and lila-ws |
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
afterBoard.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.
As the TODO say
Move.finalizeAfteris very brittle, but more generallyMoveseems 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