niklasf / python-chess

A chess library for Python, with move generation and validation, PGN parsing and writing, Polyglot opening book reading, Gaviota tablebase probing, Syzygy tablebase probing, and UCI/XBoard engine communication
https://python-chess.readthedocs.io/en/latest/
GNU General Public License v3.0
2.45k stars 531 forks source link

a show stopper bug on handling castling #38

Closed aichess closed 9 years ago

aichess commented 9 years ago

It seems the move string for castling has changed from '0-0' to 'e1h1' if white castles king side. This is inconsistent with stockfish, which recognize either '0-0' or 'e1g1'.

This is actually a serious bug -- I've just upgraded from 0.7.0, suddenly the engines spits out wrong cp scores after castling. At this stage, I'd say 0.9.0 is in very bad state... Please fix asap.

niklasf commented 9 years ago

Hi @richtian, you can use the new board.parse_uci(str), board.push_uci(str) and board.uci(str, chess960=False) to deal with the castling moves of both formats.

Stockfish also has an UCI_Chess960 option.

Additionally the chess.uci module converts between the formats as nescessary. If you can not use the UCI module in its current state, I'd be intrested to learn how I can improve it.

I realize that not everybody has the time to deal with such huge changes. The 0.8.x branch will still get all bugfixes for a while.

Please have a look at the changelog to avoid unpleasant surprises like this. There are also a lot of other breaking changes in 0.9.0.

aichess commented 9 years ago

Thanks a lot Niklas. Thank you for creating such a useful python package, it's been very useful for my experiment!

On Wed, Jul 1, 2015 at 12:45 PM, Niklas Fiekas notifications@github.com wrote:

Hi @richtian https://github.com/richtian, you can use the new board.parse_uci(str), board.push_uci(str) and board.uci(str, chess960=False) to deal with the castling moves of both formats.

Stockfish also has an UCI_Chess960 option.

Additionally the chess.uci module converts between the formats as nescessary. If you can not use the UCI module in its current state, I'd be intrested to learn how I can improve it.

I realize that not everybody has the time to deal with such huge changes. The 0.8.x branch will still get all bugfixes for a while.

Please have a look at the changelog https://github.com/niklasf/python-chess/blob/master/CHANGELOG.rst to avoid unpleasant surprises like this. There are also a lot of other breaking changes in 0.9.0.

— Reply to this email directly or view it on GitHub https://github.com/niklasf/python-chess/issues/38#issuecomment-117745048 .

niklasf commented 9 years ago

Thanks! I'll keep this issue open as a reminder that it might be useful to make the castling move transition more seamless.

niklasf commented 9 years ago

This has been more useful than not in my own projects. Barring major changes (like support for even more variants) I think I'll keep it this way.

dsjoerg commented 9 years ago

I got bit by this, for example:

from chess import *
board = Board('r2qkbnr/ppp2pp1/2bp3p/4p3/2B1P3/2N2N2/PPP2PPP/R1BQK2R w KQkq - 0 8')
stockfish_move = 'e1g1'
move = Move.from_uci(stockfish_move)
san = board.san(move)
board.push(move)
print(board.fen())

I see from your note above how to work around the problem.

Thanks very much for this excellent library!

To avoid others getting tripped by this issue in the future I suggest renaming from_uci to from_uci960 since e1h1 is not, as far as I know, standard UCI for castling. I just re-read the UCI doc here: http://download.shredderchess.com/div/uci.zip and it seems to suggest that e1g1 is the UCI standard for a white's kingside castle.

ddugovic commented 9 years ago

If I remember correctly, standard UCI defines Chess960 castling in the e1h1 form, but doesn't mention if playing chess960 from the setup rnbqkbnr how to specify castling.

But perhaps I remember incorrectly (or perhaps I'm thinking of polyglot.)

niklasf commented 9 years ago

Standard UCI for kingside castling is indeed e1g1. e1h1 is UCI_Chess960 mode and (yes) polyglot. For the moment I'll keep python-chess using the UCI_Chess960 variant internally. I'll think about the from_uci960 suggestion for the next release.

niklasf commented 9 years ago

Reconsidering this. UCI_Chess960 as default appears to be by far the most counter-intuitive feature. People are also regularly asking about this by mail.

It offers simple Chess960 support, but you have to do something in order to get just standard chess. I guess it should be the other way around.

I plan to add a chess960 property to Board and let False be the default: chess.Board(fen=STARTING_FEN, chess960=False). Suggestions and comments welcome.

dsjoerg commented 9 years ago

@niklasf I like your plan.

aichess commented 9 years ago

+1

On Mon, Aug 31, 2015 at 9:31 PM, David Joerg notifications@github.com wrote:

@niklasf https://github.com/niklasf I like your plan.

— Reply to this email directly or view it on GitHub https://github.com/niklasf/python-chess/issues/38#issuecomment-136546222 .

niklasf commented 9 years ago

python-chess v0.11.0 released with this change :)

aichess commented 9 years ago

Awesome. Thanks a lot again for creating this great library!

Sent from my iPhone

On Sep 6, 2015, at 8:34 AM, Niklas Fiekas notifications@github.com wrote:

python-chess v0.11.0 released with this change.

— Reply to this email directly or view it on GitHub.