Game loop GUI by RuthTurk · Pull Request #247 · ucfai/knightros-gambit

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

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

RuthTurk · 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

    def __str__(self):
        return "CLHumanPlayer"

+class GUIPlayer:


Can you make this GUIHumanPlayer pls?

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

        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

+        """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

+        """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

+        """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

-
-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

+"""
+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

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


This shouldn't be indented

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

+"""
+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

+    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

+        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

+     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

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


space after #

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

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


space after #

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

+"""
+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

+    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

        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


 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

@@ -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

+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

+    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

+            # 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

+    #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

+            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

+    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

+        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?

RuthTurk 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

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

adding docstrings to gui.py

ecd9e30

nashirj reviewed Apr 21, 2022

View reviewed changes

RuthTurk 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

RuthTurk added 2 commits May 30, 2022 14:08

fixing pr comments and styling code

c07d357

updating branch

f74ded9

RuthTurk 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

		"""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)

Uh oh!

Conversation

RuthTurk commented Apr 20, 2022

Uh oh!

RuthTurk commented Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nashirj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nashirj Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nashirj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nashirj commented Aug 22, 2022

Uh oh!

nashirj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

RuthTurk commented Apr 20, 2022 •

edited

Loading

nashirj Apr 20, 2022 •

edited

Loading