bhlangonijr / chesslib

chess library for legal move generation, FEN/PGN parsing and more
Apache License 2.0
223 stars 78 forks source link

isCastleMove on GameContext is true for any moves like e1g1 #36

Closed dlbbld closed 3 years ago

dlbbld commented 3 years ago

When making any of the castling king or rook moves with any piece (e.g. Qe1g1), isCastleMove on GameContext is true. It does not trigger any problems in the code but is confusing when using the method. I expect it to be only true for castling.

final Board board = new Board();
board.loadFromFen("8/5k2/8/8/8/8/5K2/4Q3 w - - 0 1");

final Move whiteQueenMoveE1G1 = new Move("e1g1", Side.WHITE);
final boolean gameContextCastleMove = board.getContext().isCastleMove(whiteQueenMoveE1G1);
System.out.println("gameContext.isCastleMove: Expected=false, Actual=" + gameContextCastleMove);

The output is as below, with latest version 1.1.23, which I think is a bit confusing:

gameContext.isCastleMove: Expected=false, Actual=true
bhlangonijr commented 3 years ago

HI @dlbbld, GameContext doesn't have any information about the chessboard state and pieces locations. It's meant to be a helper class for verifying castling move in a uniform way depending on the chess variant loaded (currently it does only support normal chess). In a particular variant, Individual castling moves for king and rooks are not only used for classifying the type of the move but also for verifying position legality and other things. Having such helper class is a convenience to avoid adding a lot of if/else statements when handling these situations on different variants That said, you must always use this method in conjunction with Board methods to validate that the moving piece is a king. It doesn't make sense to use it alone the way it was designed for. I understand you want something to qualify the move based on the chessboard state but this is not really what this specific method is meant for. We can introduce such method in the future. In the meanwhile you can build some helper method that takes Board and Move objects as arguments returning whether it's a castle move or not.

dlbbld commented 3 years ago

Hi Carlos Yes, I understand, so this is implementation specific. MoveBackup.getCastleMove() flags now if the move was a castling move, that is enough for me.

bhlangonijr commented 3 years ago

In MoveBackup you can verify if move played is en passant, promotion or castling but not very intuitively. These flags are dependent on the chessboard state and the Move played so it'd nice to come up with something more concise and intuitive indeed. If you have some suggestion you can open a new ticket. I'll close this ticket for now.

dlbbld commented 3 years ago

Yes, right, it took me some time to figure out that most information about the last move is in the MoveBackup. Also the captured piece if any and the moving piece. I think this is fine for now.