Game loop GUI #247

RuthShryock · 2022-04-20T16:38:17Z

Might need to reconfigure some of the invalid moves, pawn promotion situation, and ending the game.

RuthShryock · 2022-04-20T16:39:30Z

addresses #84

nashirj

I'm leaving you a partial review, I'll continue reviewing gui.py, pieces.py and chessboard.py in the meantime and make a second review for those files.

nashirj · 2022-04-20T18:48:47Z

firmware/raspi/player.py

@@ -69,6 +70,29 @@ def select_move(self, board):
    def __str__(self):
        return "CLHumanPlayer"

+class GUIPlayer:


Can you make this GUIHumanPlayer pls?

nashirj · 2022-04-20T18:49:17Z

firmware/raspi/player.py

@@ -69,6 +70,29 @@ def select_move(self, board):
    def __str__(self):
        return "CLHumanPlayer"

+class GUIPlayer:
+    """"GUI" class that allows playing with the chessboard through the gui.


"""Human class that allows playing with the chessboard through the gui. """

nashirj · 2022-04-20T18:49:54Z

firmware/raspi/player.py

+        """returns None until a new move is selected on the GUI
+        (which can only be done if it"s the GUI player"s turn)


Pls capitalize first word (Return) and change the double quote in it"s to a single quote

nashirj · 2022-04-20T18:50:07Z

firmware/raspi/player.py

+        """returns None until a new move is selected on the GUI
+        (which can only be done if it"s the GUI player"s turn)
+        """
+        #Note next move can be None


Please add a single space between # and start of comment and remove the newline between the docstring and the start of the function implementation

nashirj · 2022-04-20T18:52:31Z

firmware/raspi/game.py

+        """if self.board.engine.chess_board.move_stack:
+            return None"""
+        #this was always defaulting to true and not allowing access to move


You are right, this should instead be:

if not self.board.engine.chess_board.move_stack: return None

nashirj · 2022-04-20T18:52:44Z

firmware/raspi/game.py

-
-if __name__ == '__main__':
-    print("No main for this file, please use `cliinterface.py`")


Please add these three lines back

…with use of the verbose flag

…r invalid moves

nashirj

Works really well! One thing I noticed (aside from the comments I left you) is that castling does not work yet for the GUI player. When the king is selected, if castling is available, the square for castling should be highlighted for the king. I attached a screenshot of lichess with a position that has castling legal so you can see what it looks like.

nashirj · 2022-04-21T23:13:53Z

firmware/raspi/chessboard.py

+"""
+adapted code from:
+*    Title: chess_tk
+*    Author: Guatam Sharma
+*    Date: May 19, 2016
+*    Availability: https://github.com/saavedra29/chess_tk
+"""


Please update to:

"""Module containing class to display current board state. Also contains some custom Exceptions. Based on https://github.com/saavedra29/chess_tk """

nashirj · 2022-04-21T23:14:29Z

firmware/raspi/game.py

+    if __name__ == '__main__':
+        print("No main for this file, please use `cliinterface.py`")


This shouldn't be indented

nashirj · 2022-04-21T23:15:08Z

firmware/raspi/gui.py

+"""
+adapted code from:
+*    Title: chess_tk
+*    Author: Guatam Sharma
+*    Date: May 19, 2016
+*    Availability: https://github.com/saavedra29/chess_tk
+"""


"""Module containing main GUI code to interact with game.py. Based on https://github.com/saavedra29/chess_tk """

nashirj · 2022-04-21T23:17:48Z

firmware/raspi/chessboard.py

+    fullmove_number = 1
+    history = []
+
+    def __init__(self, pat=None): # pylint: disable=super-init-not-called unused-argument


Instead of having this pylint disable comment, please add

super().__init__()

as the first line of the function and remove the pylint comment

nashirj · 2022-04-21T23:19:07Z

firmware/raspi/chessboard.py

+        piece = self[p1]
+        try:
+            dest = self[p2]
+        except: # pylint: disable=W0702


Instead of having this pylint disable comment, please change the line except: to

except Exception as e:

and remove the pylint comment

nashirj · 2022-05-08T05:29:16Z

firmware/raspi/gui.py

+     while True:
+        start = simpledialog.askstring(
+            title="Choose Piece Color",
+            prompt="Choose piece color ([w]hite, [b]lack, or [r]andom):").lower()


pls add space after last colon in prompt variable

nashirj · 2022-05-08T05:57:14Z

firmware/raspi/gui.py

+    root=tk.Tk()
+    root.title("Knightr0\'s Gambit")
+
+    #initialize gui


space after #

nashirj · 2022-05-08T05:57:20Z

firmware/raspi/gui.py

+    textbox=tk.Text(root)
+    textbox.pack()
+
+    #redirects print statements to gui


space after #

nashirj · 2022-05-08T06:01:17Z

firmware/raspi/pieces.py

+"""
+adapted code from:
+*    Title: chess_tk
+*    Author: Guatam Sharma
+*    Date: May 19, 2016
+*    Availability: https://github.com/saavedra29/chess_tk
+
+"""


"""Module containing helper GUI code for gui.py. Based on https://github.com/saavedra29/chess_tk"""

nashirj · 2022-05-08T06:02:14Z

firmware/raspi/pieces.py

+    return module.__dict__[piece](color)
+
+class Piece(object):
+    """Chess piece class condainting color and shortname.


condainting -> containing

nashirj · 2022-08-22T00:49:11Z

Let me know if I can help with this PR in any way

nashirj

Thanks for working on this, overall looks really good. Left some comments but also have some requests for behavior changes:

If you click a piece and then click it again without making a move, it should no longer be highlighted.
The castling behavior doesn't work as expected; the GUI doesn't update the rook movement until after the next player's turn. Can we make it so that if the king makes a castling move, both the king and the rook positions are updated simultaneously?
About ten moves into the game playing as white (I tried twice), the game stops responding - it's white's turn, and after I click for where I want to go, the game freezes.

nashirj · 2022-09-09T00:17:50Z

firmware/raspi/game.py


        if arduino_status.status == status.ArduinoStatus.EXECUTING_MOVE:
            # Wait for move in progress to finish executing
-            time.sleep(1) # reduce the amount of polling while waiting for move to finish
+            time.sleep(.01) # reduce the amount of polling while waiting for move to finish


Can you please make this a static class variable called SLEEP_TIME? And then update this and line 309 to use that variable.

nashirj · 2022-09-09T00:18:04Z

firmware/raspi/game.py

@@ -171,4 +172,4 @@ def process(self, player):
        raise ValueError("We shouldn't reach this point in the function.")

 if __name__ == '__main__':
-    print("No main for this file, please use `cliinterface.py`")
+    print("No main for this file, please use `cliinterface.py`")


Please add a newline at the end of the file.

nashirj · 2022-09-09T00:18:36Z

firmware/raspi/gui.py

@@ -0,0 +1,353 @@
+"""Module containing main GUI code to interact with game.py.


Can you please name this file gui_interface.py for consistency with cli_interface.py?

nashirj · 2022-09-09T00:19:34Z

firmware/raspi/gui.py

+import tkinter as tk
+from tkinter import simpledialog


If you're importing tkinter, you don't need to also import simpledialog, you can do tk.simpledialog (I think).

Simpledialog is in a separate module from tkinter (according to this SO) so it needs to be imported separately I believe

nashirj · 2022-09-09T00:20:39Z

firmware/raspi/gui.py

+    color1 = "#949494"
+    color2 = "#f8f6f1"


I am guessing these are for the piece colors? Can you please rename them to something like COLOR_BLACK and COLOR_WHITE or something like that?

nashirj · 2022-09-09T01:19:04Z

firmware/raspi/gui.py

+            # Need to empty move queue to play out final captures, etc (if any) before ending
+            while self.game.board.msg_queue:
+                self.game.process(None)
+            #gui player can no longer make moves


Can you please update all your comments to have a space between the # and the beginning of the comment? And please capitalize the first word of the comments.

nashirj · 2022-09-09T01:20:53Z

firmware/raspi/pieces.py

+    #def moves_available(self, pos):
+        #return super(King, self).moves_available(pos.upper(), True, True, 1)


If not using, can remove.

nashirj · 2022-09-09T01:21:08Z

firmware/raspi/pieces.py

+            if board.alpha_notation(newLoc) not in board.occupied(self.color):
+                allowed_moves.append(newLoc)
+
+        # Checks if each rook can castsle


Fix spelling please

nashirj · 2022-09-09T01:21:38Z

firmware/raspi/pieces.py

+    shortname = "k"
+    canCastle = True


Please update all camel case variable names to be snake case instead: e.g. can_castle instead of canCastle.

nashirj · 2022-09-09T01:23:23Z

firmware/raspi/pieces.py

+        else:
+            startpos, direction, enemy = 6, -1, "white"
+        allowed_moves = []
+        # Moving


The function name is moves_available (which makes it seem like it would return the allowed_moves) but this comment makes it seem like this function is moving the pieces, is that what's happening?

RuthShryock added 7 commits April 11, 2022 18:36

initial game loop gui code

77ec111

adding GUIPlayer methods and styling code

5ec03c5

initial game loop gui code

117ee4c

adding GUIPlayer methods and styling code

6581f57

updating branch

30f4776

removing info frame

4dc02b5

reverted change in boardinterface.py

3140930

RuthShryock requested a review from nashirj April 20, 2022 16:38

adding docstrings to gui.py

ecd9e30

nashirj reviewed Apr 21, 2022

View reviewed changes

RuthShryock and others added 9 commits April 21, 2022 18:59

formatting changes

e8dceb8

updating methods in gui.py to match with game.py methods

8d8aa2e

changing loop structure

b6c1a21

fixed loop

2c8eee4

Reduced length of time.sleep and made printing board status optional …

6bf3a6f

…with use of the verbose flag

Reduce poll time, and also remove some print statements

8f92c5b

Fix merge conflicts

6622816

adjusting for human player to play as black and fixed highlighting fo…

507ac33

…r invalid moves

removing print line

08ab6e2

nashirj reviewed May 8, 2022

View reviewed changes

RuthShryock added 2 commits May 30, 2022 14:08

fixing pr comments and styling code

c07d357

updating branch

f74ded9

RuthShryock and others added 4 commits August 25, 2022 14:07

merging

2af1212

Added castling feature

db1b27e

fixing random turn at start

5457e78

updating branch

21c7a54

nashirj reviewed Sep 9, 2022

View reviewed changes

fixed highlighting after click and styling comments

5937e34

updating branch

d6a5066

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Game loop GUI #247

Game loop GUI #247

RuthShryock commented Apr 20, 2022

RuthShryock commented Apr 20, 2022 •

edited

Loading

nashirj left a comment

nashirj Apr 20, 2022

nashirj Apr 20, 2022

nashirj Apr 20, 2022

nashirj Apr 20, 2022 •

edited

Loading

nashirj Apr 20, 2022

nashirj Apr 20, 2022

nashirj left a comment

nashirj Apr 21, 2022

nashirj Apr 21, 2022

nashirj Apr 21, 2022

nashirj Apr 21, 2022

nashirj Apr 21, 2022

nashirj May 8, 2022

nashirj May 8, 2022

nashirj May 8, 2022

nashirj May 8, 2022

nashirj May 8, 2022

nashirj commented Aug 22, 2022

nashirj left a comment

nashirj Sep 9, 2022

nashirj Sep 9, 2022

nashirj Sep 9, 2022

nashirj Sep 9, 2022

RuthShryock Oct 7, 2022

nashirj Sep 9, 2022

nashirj Sep 9, 2022

nashirj Sep 9, 2022

nashirj Sep 9, 2022

nashirj Sep 9, 2022

nashirj Sep 9, 2022

		"""returns None until a new move is selected on the GUI
		(which can only be done if it"s the GUI player"s turn)


		if __name__ == '__main__':
		print("No main for this file, please use `cliinterface.py`")

		@@ -0,0 +1,353 @@
		"""Module containing main GUI code to interact with game.py.

		#def moves_available(self, pos):
		#return super(King, self).moves_available(pos.upper(), True, True, 1)

Game loop GUI #247

Are you sure you want to change the base?

Game loop GUI #247

Conversation

RuthShryock commented Apr 20, 2022

RuthShryock commented Apr 20, 2022 • edited Loading

nashirj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nashirj Apr 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nashirj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nashirj commented Aug 22, 2022

nashirj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RuthShryock commented Apr 20, 2022 •

edited

Loading

nashirj Apr 20, 2022 •

edited

Loading