search

lichess-org / dartchess

Dart chess library for native platforms
https://pub.dev/packages/dartchess
GNU General Public License v3.0
33 stars 17 forks source link

Castling bug (all variants) #25

Closed dbergan closed 1 month ago

dbergan commented 1 year ago

_getCastlingSide() creates all sorts of "legal" castling moves where the king is moved onto another piece of its own color.

e.g. depending on the setup, I saw things like Ka2, Kd4, and Kf3 (moving onto his own pawn or knight) resulting in a kingside castle, and Ke1 (moving onto its own square) resulting in a queenside castle. In one position I had 13 legal moves that resulted in a castle. They all involved moving onto another friendly piece or e1.

veloce commented 1 year ago

I'm not sure to understand what is the bug here. _getCastlingSide() is a private method and by itself it doesn't generate "legal" castling moves. Legality is tested using unit tests and especially perft tests, and afaik there is no bug detected here.

I saw your PR so I'll try to look at it, but in general issues should be raised when you find a bug with the public API of dartchess with a test proving sth is wrong that I can reproduce.

dbergan commented 1 year ago

Hi Vincent!

That makes sense. Let me try again.

Take a classical game of chess at this fen:

r3k2r/pppbbppp/2n2n2/3pp2Q/3PP2q/2N2N2/PPPBBPPP/R3K2R w KQkq - 10 8

It's white's move and if you query whether or not e1a2 is a legal move...

position.isLegal(NormalMove(from: 4, to: 8))

it returns true. In fact all of these are considered legal moves:

e1e1, e1a2, e1b2, e1c2, e1d2, e1e2, e1f2, e1g2, e1h2, e1c3, e1f3, e1d4, e1e4, e1h5

If you play the move e1a2 (or any of these options), White castles.

And on the next move, it's the same situation for Black.

e8h4, e8d5, e8e5, e8c6, e8f6, e8a7, e8b7, e8c7, e8d7, e8e7, e8f7, e8g7, e8h7, e8e8 are all considered legal.

In my pull request, the tests for this setup start at line 90 of position_test.dart.

The cause of this bug is in _getCastlingSide(), which I provided a fix for in my pull request. And that's why I titled my issue that way.

Thanks for your patience with me and helping me understand the proper way to communicate here.

Have a great day!

Kind regards, David

veloce commented 1 month ago

After all this time, thank you again for reporting this @dbergan . I got sidetracked and forgot about it since lichess app is not impacted by this bug (it doesn't use Position.isLegal).

I've fixed the bug, using your fix in the PR.

dbergan commented 1 month ago

Glad it was helpful.

Kind regards, David