Fixed #27 - do not ask for promotion piece when promotion is illegal.

cosinekitty · cosinekitty · commit a92acc968dec · 2021-05-02T20:54:17.000-04:00
All user interfaces for human players had a flaw:
if promotion was illegal because it would leave the mover
in check, there was still a callback to the UI that asked
what piece to promote to. Now we make sure promotion to
queen is legal before even asking. All 4 promotion pieces
are equally legal; there is no case where one is legal
and the others aren't.
diff --git a/src/chess.h b/src/chess.h
@@ -325,6 +325,7 @@ int    ConvertSquareToNybble ( SQUARE );
 
 #define   SPECIAL_MOVE_MASK       0xF0
 
+struct MoveList;
 
 struct Move   // A nice 32-bit structure!
 {
@@ -344,7 +345,7 @@ struct Move   // A nice 32-bit structure!
     }
 
     void Fix ( unsigned long openBookData, ChessBoard & );
-    bool Fix ( const ChessBoard &, int Source, int Dest, SQUARE OptionalProm, ChessUI & );
+    void Fix ( const ChessBoard &, const MoveList& LegalMoves, int Source, int Dest, SQUARE OptionalProm, ChessUI & );
 
     bool ShouldIgnore() const
     {
diff --git a/src/human.cpp b/src/human.cpp
@@ -63,7 +63,7 @@ bool HumanChessPlayer::GetMove (
         }
 
         // This checks for pawn promotion and stuff like that.
-        move.Fix ( board, source, dest, promIndex, userInterface );
+        move.Fix(board, legalMoves, source, dest, promIndex, userInterface);
 
         // Regenerate legal moves in case they edited the board.
         board.GenMoves(legalMoves);
diff --git a/src/misc.cpp b/src/misc.cpp
@@ -310,8 +310,9 @@ SQUARE Move::actualOffsets (
 }
 
 
-bool Move::Fix (
+void Move::Fix (
     const ChessBoard    &Board,
+    const MoveList      &LegalMoves,
     int                  Source,
     int                  Dest,
     SQUARE               OptionalProm,
@@ -323,22 +324,25 @@ bool Move::Fix (
     {
         source = Source;
         dest = Dest;
-        return true;
+        return;
     }
 
     // --- end of Suggest Move code.
 
+    source = 0;
+    dest = 0;
+    score = 0;
+
     if ( Source<OFFSET(2,2) || Source>OFFSET(9,9) ||
-            Dest<OFFSET(2,2) || Dest>OFFSET(9,9) ||
-            Board.GetSquareContents(Source) == OFFBOARD ||
-            Board.GetSquareContents(Dest) == OFFBOARD )
+         Dest<OFFSET(2,2) || Dest>OFFSET(9,9) ||
+         Board.GetSquareContents(Source) == OFFBOARD ||
+         Board.GetSquareContents(Dest) == OFFBOARD )
     {
-        return false;
+        return;
     }
 
-    source = BYTE ( Source );
-    dest   = BYTE ( Dest );
-    score  = 0;
+    source = BYTE(Source);
+    dest   = BYTE(Dest);
 
     // Check for special moves...
 
@@ -347,27 +351,23 @@ bool Move::Fix (
 
     ChessSide side = Board.WhiteToMove() ? SIDE_WHITE : SIDE_BLACK;
 
-    SQUARE move_piece = Board.GetSquareContents ( Source );
+    SQUARE move_piece = Board.GetSquareContents(Source);
     SQUARE prom;
     int    moveVector = Dest - Source;
 
-    if ( Source == OFFSET(6,2) && Dest == OFFSET(8,2) &&
-        (move_piece & WK_MASK) )
+    if ( Source == OFFSET(6,2) && Dest == OFFSET(8,2) && (move_piece & WK_MASK) )
     {
         dest = SPECIAL_MOVE_KCASTLE;
     }
-    else if ( Source == OFFSET(6,2) && Dest == OFFSET(4,2) &&
-              (move_piece & WK_MASK) )
+    else if ( Source == OFFSET(6,2) && Dest == OFFSET(4,2) && (move_piece & WK_MASK) )
     {
         dest = SPECIAL_MOVE_QCASTLE;
     }
-    else if ( Source == OFFSET(6,9) && Dest == OFFSET(8,9) &&
-              (move_piece & BK_MASK) )
+    else if ( Source == OFFSET(6,9) && Dest == OFFSET(8,9) && (move_piece & BK_MASK) )
     {
         dest = SPECIAL_MOVE_KCASTLE;
     }
-    else if ( Source == OFFSET(6,9) && Dest == OFFSET(4,9) &&
-              (move_piece & BK_MASK) )
+    else if ( Source == OFFSET(6,9) && Dest == OFFSET(4,9) && (move_piece & BK_MASK) )
     {
         dest = SPECIAL_MOVE_QCASTLE;
     }
@@ -389,23 +389,25 @@ bool Move::Fix (
             {
                 if (OptionalProm == 0)
                 {
-                    if ( (Board.GetSquareContents(Dest) & enemyMask) == 0 )
-                    {
-                        // We do this because we already know it's not legal.
-                        // This avoids a spurious call to the UI.
-                        prom = Q_INDEX;
-                    }
+                    // Avoid asking for promotion piece if promotion isn't even legal.
+                    // Try promotion to queen to see if that is legal.
+                    // If promoting to a queen is legal, then underpromoting is legal too.
+                    if (moveVector == pawn_dir + EAST)
+                        dest = SPECIAL_MOVE_PROMOTE_CAP_EAST | Q_INDEX;
                     else
-                    {
-                        prom = Ui.PromotePawn ( Dest, side );
-                    }
+                        dest = SPECIAL_MOVE_PROMOTE_CAP_WEST | Q_INDEX;
+
+                    if (LegalMoves.IsLegal(*this))
+                        prom = Ui.PromotePawn(Dest, side);
+                    else
+                        prom = Q_INDEX;     // caller will also notice promotion is illegal
                 }
                 else
                 {
                     prom = OptionalProm;
                 }
 
-                if ( moveVector == pawn_dir + EAST )
+                if (moveVector == pawn_dir + EAST)
                     dest = SPECIAL_MOVE_PROMOTE_CAP_EAST | prom;
                 else
                     dest = SPECIAL_MOVE_PROMOTE_CAP_WEST | prom;
@@ -420,29 +422,22 @@ bool Move::Fix (
             {
                 if (OptionalProm == 0)
                 {
-                    if ( Board.GetSquareContents(Dest) != EMPTY )
-                    {
-                        // We know this isn't a legal move already, because
-                        // there's something blocking the pawn promotion.
-                        // Go ahead and make the move an illegal queen promotion.
-                        // This avoids a spurious call to the UI to ask the
-                        // user what piece to promote it to.
-                        prom = Q_INDEX;
-                    }
+                    // Avoid asking for promotion piece if promotion isn't even legal.
+                    // If promoting to a queen is legal, then underpromoting is legal too.
+                    dest = SPECIAL_MOVE_PROMOTE_NORM | Q_INDEX;
+                    if (LegalMoves.IsLegal(*this))
+                        prom = Ui.PromotePawn(Dest, side);
                     else
-                        prom = Ui.PromotePawn ( Dest, side );
+                        prom = Q_INDEX;     // caller will also notice promotion is illegal
                 }
                 else
                 {
                     prom = OptionalProm;
                 }
-
                 dest = SPECIAL_MOVE_PROMOTE_NORM | prom;
             }
         }
     }
-
-    return true;
 }
 
 

Original file line number	Diff line number	Diff line change
`@@ -325,6 +325,7 @@ int ConvertSquareToNybble ( SQUARE );`
`325`	`325`
`326`	`326`	`#define SPECIAL_MOVE_MASK 0xF0`
`327`	`327`
	`328`	`+struct MoveList;`
`328`	`329`
`329`	`330`	`struct Move // A nice 32-bit structure!`
`330`	`331`	`{`
`@@ -344,7 +345,7 @@ struct Move // A nice 32-bit structure!`
`344`	`345`	`}`
`345`	`346`
`346`	`347`	`void Fix ( unsigned long openBookData, ChessBoard & );`
`347`		`- bool Fix ( const ChessBoard &, int Source, int Dest, SQUARE OptionalProm, ChessUI & );`
	`348`	`+ void Fix ( const ChessBoard &, const MoveList& LegalMoves, int Source, int Dest, SQUARE OptionalProm, ChessUI & );`
`348`	`349`
`349`	`350`	`bool ShouldIgnore() const`
`350`	`351`	`{`
Original file line number	Diff line number	Diff line change
`@@ -63,7 +63,7 @@ bool HumanChessPlayer::GetMove (`
`63`	`63`	`}`
`64`	`64`
`65`	`65`	`// This checks for pawn promotion and stuff like that.`
`66`		`- move.Fix ( board, source, dest, promIndex, userInterface );`
	`66`	`+ move.Fix(board, legalMoves, source, dest, promIndex, userInterface);`
`67`	`67`
`68`	`68`	`// Regenerate legal moves in case they edited the board.`
`69`	`69`	`board.GenMoves(legalMoves);`
Original file line number	Diff line number	Diff line change
`@@ -310,8 +310,9 @@ SQUARE Move::actualOffsets (`
`310`	`310`	`}`
`311`	`311`
`312`	`312`
`313`		`-bool Move::Fix (`
	`313`	`+void Move::Fix (`
`314`	`314`	`const ChessBoard &Board,`
	`315`	`+ const MoveList &LegalMoves,`
`315`	`316`	`int Source,`
`316`	`317`	`int Dest,`
`317`	`318`	`SQUARE OptionalProm,`
`@@ -323,22 +324,25 @@ bool Move::Fix (`
`323`	`324`	`{`
`324`	`325`	`source = Source;`
`325`	`326`	`dest = Dest;`
`326`		`- return true;`
	`327`	`+ return;`
`327`	`328`	`}`
`328`	`329`
`329`	`330`	`// --- end of Suggest Move code.`
`330`	`331`
	`332`	`+ source = 0;`
	`333`	`+ dest = 0;`
	`334`	`+ score = 0;`
	`335`	`+`
`331`	`336`	`if ( Source<OFFSET(2,2) \|\| Source>OFFSET(9,9) \|\|`
`332`		`- Dest<OFFSET(2,2) \|\| Dest>OFFSET(9,9) \|\|`
`333`		`- Board.GetSquareContents(Source) == OFFBOARD \|\|`
`334`		`- Board.GetSquareContents(Dest) == OFFBOARD )`
	`337`	`+ Dest<OFFSET(2,2) \|\| Dest>OFFSET(9,9) \|\|`
	`338`	`+ Board.GetSquareContents(Source) == OFFBOARD \|\|`
	`339`	`+ Board.GetSquareContents(Dest) == OFFBOARD )`
`335`	`340`	`{`
`336`		`- return false;`
	`341`	`+ return;`
`337`	`342`	`}`
`338`	`343`
`339`		`- source = BYTE ( Source );`
`340`		`- dest = BYTE ( Dest );`
`341`		`- score = 0;`
	`344`	`+ source = BYTE(Source);`
	`345`	`+ dest = BYTE(Dest);`
`342`	`346`
`343`	`347`	`// Check for special moves...`
`344`	`348`
`@@ -347,27 +351,23 @@ bool Move::Fix (`
`347`	`351`
`348`	`352`	`ChessSide side = Board.WhiteToMove() ? SIDE_WHITE : SIDE_BLACK;`
`349`	`353`
`350`		`- SQUARE move_piece = Board.GetSquareContents ( Source );`
	`354`	`+ SQUARE move_piece = Board.GetSquareContents(Source);`
`351`	`355`	`SQUARE prom;`
`352`	`356`	`int moveVector = Dest - Source;`
`353`	`357`
`354`		`- if ( Source == OFFSET(6,2) && Dest == OFFSET(8,2) &&`
`355`		`- (move_piece & WK_MASK) )`
	`358`	`+ if ( Source == OFFSET(6,2) && Dest == OFFSET(8,2) && (move_piece & WK_MASK) )`
`356`	`359`	`{`
`357`	`360`	`dest = SPECIAL_MOVE_KCASTLE;`
`358`	`361`	`}`
`359`		`- else if ( Source == OFFSET(6,2) && Dest == OFFSET(4,2) &&`
`360`		`- (move_piece & WK_MASK) )`
	`362`	`+ else if ( Source == OFFSET(6,2) && Dest == OFFSET(4,2) && (move_piece & WK_MASK) )`
`361`	`363`	`{`
`362`	`364`	`dest = SPECIAL_MOVE_QCASTLE;`
`363`	`365`	`}`
`364`		`- else if ( Source == OFFSET(6,9) && Dest == OFFSET(8,9) &&`
`365`		`- (move_piece & BK_MASK) )`
	`366`	`+ else if ( Source == OFFSET(6,9) && Dest == OFFSET(8,9) && (move_piece & BK_MASK) )`
`366`	`367`	`{`
`367`	`368`	`dest = SPECIAL_MOVE_KCASTLE;`
`368`	`369`	`}`
`369`		`- else if ( Source == OFFSET(6,9) && Dest == OFFSET(4,9) &&`
`370`		`- (move_piece & BK_MASK) )`
	`370`	`+ else if ( Source == OFFSET(6,9) && Dest == OFFSET(4,9) && (move_piece & BK_MASK) )`
`371`	`371`	`{`
`372`	`372`	`dest = SPECIAL_MOVE_QCASTLE;`
`373`	`373`	`}`
`@@ -389,23 +389,25 @@ bool Move::Fix (`
`389`	`389`	`{`
`390`	`390`	`if (OptionalProm == 0)`
`391`	`391`	`{`
`392`		`- if ( (Board.GetSquareContents(Dest) & enemyMask) == 0 )`
`393`		`- {`
`394`		`- // We do this because we already know it's not legal.`
`395`		`- // This avoids a spurious call to the UI.`
`396`		`- prom = Q_INDEX;`
`397`		`- }`
	`392`	`+ // Avoid asking for promotion piece if promotion isn't even legal.`
	`393`	`+ // Try promotion to queen to see if that is legal.`
	`394`	`+ // If promoting to a queen is legal, then underpromoting is legal too.`
	`395`	`+ if (moveVector == pawn_dir + EAST)`
	`396`	`+ dest = SPECIAL_MOVE_PROMOTE_CAP_EAST \| Q_INDEX;`
`398`	`397`	`else`
`399`		`- {`
`400`		`- prom = Ui.PromotePawn ( Dest, side );`
`401`		`- }`
	`398`	`+ dest = SPECIAL_MOVE_PROMOTE_CAP_WEST \| Q_INDEX;`
	`399`	`+`
	`400`	`+ if (LegalMoves.IsLegal(*this))`
	`401`	`+ prom = Ui.PromotePawn(Dest, side);`
	`402`	`+ else`
	`403`	`+ prom = Q_INDEX; // caller will also notice promotion is illegal`
`402`	`404`	`}`
`403`	`405`	`else`
`404`	`406`	`{`
`405`	`407`	`prom = OptionalProm;`
`406`	`408`	`}`
`407`	`409`
`408`		`- if ( moveVector == pawn_dir + EAST )`
	`410`	`+ if (moveVector == pawn_dir + EAST)`
`409`	`411`	`dest = SPECIAL_MOVE_PROMOTE_CAP_EAST \| prom;`
`410`	`412`	`else`
`411`	`413`	`dest = SPECIAL_MOVE_PROMOTE_CAP_WEST \| prom;`
`@@ -420,29 +422,22 @@ bool Move::Fix (`
`420`	`422`	`{`
`421`	`423`	`if (OptionalProm == 0)`
`422`	`424`	`{`
`423`		`- if ( Board.GetSquareContents(Dest) != EMPTY )`
`424`		`- {`
`425`		`- // We know this isn't a legal move already, because`
`426`		`- // there's something blocking the pawn promotion.`
`427`		`- // Go ahead and make the move an illegal queen promotion.`
`428`		`- // This avoids a spurious call to the UI to ask the`
`429`		`- // user what piece to promote it to.`
`430`		`- prom = Q_INDEX;`
`431`		`- }`
	`425`	`+ // Avoid asking for promotion piece if promotion isn't even legal.`
	`426`	`+ // If promoting to a queen is legal, then underpromoting is legal too.`
	`427`	`+ dest = SPECIAL_MOVE_PROMOTE_NORM \| Q_INDEX;`
	`428`	`+ if (LegalMoves.IsLegal(*this))`
	`429`	`+ prom = Ui.PromotePawn(Dest, side);`
`432`	`430`	`else`
`433`		`- prom = Ui.PromotePawn ( Dest, side );`
	`431`	`+ prom = Q_INDEX; // caller will also notice promotion is illegal`
`434`	`432`	`}`
`435`	`433`	`else`
`436`	`434`	`{`
`437`	`435`	`prom = OptionalProm;`
`438`	`436`	`}`
`439`		`-`
`440`	`437`	`dest = SPECIAL_MOVE_PROMOTE_NORM \| prom;`
`441`	`438`	`}`
`442`	`439`	`}`
`443`	`440`	`}`
`444`		`-`
`445`		`- return true;`
`446`	`441`	`}`
`447`	`442`
`448`	`443`