bhlangonijr / chesslib

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

Board.equals bug #40

Closed kindex closed 4 years ago

kindex commented 4 years ago

Code

            if (result) {
                result = getSideToMove().equals(board.getSideToMove());
                result = result || getCastleRight(Side.WHITE).equals(board.getCastleRight(Side.WHITE));
                result = result || getCastleRight(Side.BLACK).equals(board.getCastleRight(Side.BLACK));
                result = result || getEnPassant().equals(board.getEnPassant());
            }

returns wrong result. Operators || should be replaced with &&

bhlangonijr commented 4 years ago

Yes, thank you for pointing that out. This has passed unnoticed as this method is hardly if ever used (at least by myself). The fix should go in the next release.

dlbbld commented 4 years ago

For me, two boards are only the same if they behave the same in the future. To have the same behaviour for threefold repetition and fifty-move rule, you need to include the Board.history list in the comparison additionally. What do you think about this suggestion?

bhlangonijr commented 4 years ago

Hi @dlbbld Daniel, yes and no: We should take into consideration the fifty-move rule; but Board#history is a different story: It's storing all the unique hash ids for the intermediate positions leading to the current state. Due to the effect of transposition you may reach the same chess position by playing different moves. We should take into account the fifty-move counter when comparing Boards, but not the history. Agreed?

bhlangonijr commented 4 years ago

Fixed in 1.1.24

dlbbld commented 4 years ago

In that respect I suggest comparing the history as a list, that is the same size, same order, same unique hash. That is equivalent (I guess so) to same starting position and same moves played.

dlbbld commented 4 years ago

In my opinions two boards which are equal using board.equals() must behave the same. With the current implementation of equal this is not the case, for example one board can indicate threefold repetition, the other not, as the initial position and move history are not compared. For the sake of an example see below. I suggest to reevaluate this.

final Board board1 = new Board();
board1.doMove(new Move("b1c3", Side.WHITE));
board1.doMove(new Move("b8c6", Side.BLACK));
board1.doMove(new Move("c3b1", Side.WHITE));
board1.doMove(new Move("c6b8", Side.BLACK));
board1.doMove(new Move("b1c3", Side.WHITE));
board1.doMove(new Move("b8c6", Side.BLACK));
board1.doMove(new Move("c3b1", Side.WHITE));

final Board board2 = new Board();
board2.loadFromFen(board1.getFen());

System.out.println(board1.equals(board2)); // true

board1.doMove(new Move("c6b8", Side.BLACK));
board2.doMove(new Move("c6b8", Side.BLACK));

System.out.println(board1.isRepetition()); // true
System.out.println(board2.isRepetition()); // false
bhlangonijr commented 4 years ago

@dlbbld would you open another ticket for your suggestion so we can keep track of that? This specific one is reporting a clear bug on the board equals method. You are suggesting something differently: include board history when comparing two boards.