SebLague / Chess-Challenge

Create your own tiny chess bot!
https://www.youtube.com/watch?v=Ne40a5LkK6A
MIT License
1.79k stars 1.06k forks source link

GetLegalMoves() returns illegal move #253

Closed NechZ closed 1 year ago

NechZ commented 1 year ago

My bot exclusively considers moves in the GetLegalMoves() Array, however on some occasions, most notably when the Bot's king is cornered, it tries an Illegal move, which is then flagged as one. I don't see that being caused by my code however upon request I would share it (It's not exactly impressive 😢 ).

Illegal move: Null in position: 5R2/p1Np2pp/8/8/2P5/1K5P/6P1/k7 b - - 3 34 Game Over: BlackIllegalMove image

Eldriitch commented 1 year ago

The API is reporting that the illegal move was a null move rather than an incorrect move, so this is almost certainly not down to GetLegalmoves(). It's probably a subtle bug in your own code.

Happypotatoes37 commented 1 year ago

I have the same problem, and I think the problem, for me at least, is that I have something searching multiple moves ahead and it is trying to find a move after the game has ended, resulting in no possible moves.

*Edit: I figured it out, I just needed to add and if to stop it from checking the next move if it was mate. (Code at very bottom if you care)

Code with the error.

    private BestMove GetBestMove(Board board, int depth = 3)
    {
        depth--;
        List<BestMove> bestMoves = new();
        Move[] legalMoves = board.GetLegalMoves();

        bestMoves.Add(new (legalMoves[0]));

        int bestMoveValue = -99999;

        foreach (Move move in legalMoves)
        {
            board.MakeMove(move);

            if (!board.IsDraw())
            {
                if (depth != 0)
                {
                    BestMove nextBestMove = GetBestMove(board, depth);

                    if (-nextBestMove.value > bestMoveValue)
                    {
                        bestMoves.Clear();
                        bestMoveValue = -nextBestMove.value;
                        bestMoves.Add(new BestMove(move, bestMoveValue));
                    }
                    else if (-nextBestMove.value == bestMoveValue)
                    {
                        bestMoves.Add(new BestMove(move, bestMoveValue));
                    }
                }
                else
                {
                    int moveValue = CalculateMaterial(board);

                    moveValue += CastlingRightsLost(board, move);

                    if (board.IsInCheckmate())
                    {
                        board.UndoMove(move);
                        bestMoves.Clear();
                        bestMoves.Add(new(move, 99999));
                    }

                    if (moveValue > bestMoveValue)
                    {
                        bestMoves.Clear();
                        bestMoveValue = moveValue;
                        bestMoves.Add(new(move, bestMoveValue));
                    }
                    else if (moveValue == bestMoveValue)
                    {
                        bestMoves.Add(new(move, bestMoveValue));
                    }
                }
            }

            board.UndoMove(move);
        }

        return bestMoves[new Random().Next(bestMoves.Count)];
    }

Fixed version

    private BestMove GetBestMove(Board board, int depth = 3)
    {
        depth--;
        List<BestMove> bestMoves = new();
        Move[] legalMoves = board.GetLegalMoves();
        bestMoves.Add(new (legalMoves[new Random().Next(legalMoves.Length)]));
        int bestMoveValue = -99999;

        foreach (Move move in legalMoves)
        {
            board.MakeMove(move);

            if (!board.IsDraw())
            {
                if (depth != 0)
                {
                    if (!board.IsInCheckmate())
                    {
                        BestMove nextBestMove = GetBestMove(board, depth);

                        if (-nextBestMove.value > bestMoveValue)
                        {
                            bestMoves.Clear();
                            bestMoveValue = -nextBestMove.value;
                            bestMoves.Add(new BestMove(move, bestMoveValue));
                        }
                        else if (-nextBestMove.value == bestMoveValue)
                        {
                            bestMoves.Add(new BestMove(move, bestMoveValue));
                        }
                    }
                    else
                    {
                        bestMoves.Clear();
                        bestMoves.Add(new(move, 99999));
                        bestMoveValue = 99999;
                    }
                }
                else
                {
                    int moveValue = CalculateMaterial(board);

                    if (board.IsInCheck())
                    {
                        moveValue += 2;
                    }

                    moveValue += CastlingRightsLost(board, move);

                    if (board.IsInCheckmate())
                    {
                        bestMoves.Clear();
                        bestMoves.Add(new(move, 99999));
                        bestMoveValue = 99999;
                    }

                    if (moveValue > bestMoveValue)
                    {
                        bestMoves.Clear();
                        bestMoveValue = moveValue;
                        bestMoves.Add(new(move, bestMoveValue));
                    }
                    else if (moveValue == bestMoveValue)
                    {
                        bestMoves.Add(new(move, bestMoveValue));
                    }
                }
            }

            board.UndoMove(move);
        }

        return bestMoves[new Random().Next(bestMoves.Count)];
    }
Firestorm-253 commented 1 year ago

My bot exclusively considers moves in the GetLegalMoves() Array, however on some occasions, most notably when the Bot's king is cornered, it tries an Illegal move, which is then flagged as one. I don't see that being caused by my code however upon request I would share it (It's not exactly impressive 😢 ).

Illegal move: Null in position: 5R2/p1Np2pp/8/8/2P5/1K5P/6P1/k7 b - - 3 34 Game Over: BlackIllegalMove image

Pls share the relevant code or if https://github.com/SebLague/Chess-Challenge/issues/253#issuecomment-1647103096 already solved your problem close the Issue pls for better visibility, thx :)

Pds314 commented 1 year ago

Yeah this is returning a NullMove instead of a specific illegal move.

Firestorm-253 commented 1 year ago

Yeah this is returning a NullMove instead of a specific illegal move.

Funnily enough i ran into the exact same problem today when making some adaptions to my bot. can't find the issue either. It's always close to a checkmate case.

Firestorm-253 commented 1 year ago

so for me, the bot get's "asked" to make a move even though he's already lost (got checkmated). resulting in returning a NullMove and instead of loosing normally he loses due to the NullMove which is dumb.

Firestorm-253 commented 1 year ago

chessbot

NechZ commented 1 year ago

It is safe to say you should avoid using Move.NullMove as the default move as the bot will try to play it, should all moves be equally worse. Rather use either the first move, or a random move in the GetLegalMoves method. Thanks, this is issue is closed

DjSapsan commented 1 year ago

to avoid such bugs always make a default move (random or first found). And only then search for a better move. If your search is interrupted your bot will do a legal move anyway