4
\$\begingroup\$

Disclaimer: This code is far from complete, I am aware of issues regarding my standards of naming, my lack of comments and readability; however, I am encountering a large issue which is preventing me from editing, testing and completing the game. This is not a functional issue, but a performance issue. If you would like a more detailed explanation, this is what I wrote previously on Stack Overflow:

I am building the following game in tkinter, it's essentially a snake game with two players. Where you would battle the other player until one of you hits the other, the wall or runs out of space. To create the 'game' I am using canvases, which are created in a grid. I think this may be the cause of the following issue: When attempting to close the program, python and the script will completely freeze, and take around 5-10 seconds to close. I am aware that tkinter may not be the best option for game creation, however this is the way I have chosen and would like to know if there is any way to fix my issue.

To recap: Upon trying to close the following program - the window freezes for around 5-10 seconds. It then closes but obviously testing and actual gameplay is hindered.

One note I would like to make is that the game loads up much quicker than it closes which I find to be strange. Any optimisations that you could recommend would also be welcome however I realise that Stack Exchange is more the platform for this.

from tkinter import *
from random import randint
import os

velocity = [1,0]
velocity2 = [-1,0]
grid = []
coord = [[10,10]]
coord2 = [[20,20]]
time = 0 
end = False
colour1 = "#d35400"
colour2 = "#9b59b6"
winner = 0
size1 = 0
size2 = 0
loopIds = []
speed1 = 50
speed2 = 50
powers = ["#2ecc71","#e74c3c","#ecf0f1"]
up1 = 0
up2 = 0
down1 = 0
down2 = 0
points1 = 0
points2 = 0

master = Tk()
master.config(bg="#222")
master.title("Trail Snake")
master.wm_state('zoomed')
master.resizable(0, 0)

def start():
    global coord, coord2, colour1, colour2
    for i in coord:
        grid[i[0]][i[1]].config(bg=colour1)
    for i in coord2:
        grid[i[0]][i[1]].config(bg=colour2)

def genPower():
    global h, w, powers
    power = randint(0,2)
    location = [randint(0,h-1), randint(0,h-1)]
    while grid[location[0]][location[1]].cget("bg") != "#222":
        location = [randint(0,h-1), randint(0,h-1)]
    grid[location[0]][location[1]].config(bg=powers[power])

def clear():
    for n in grid:
        for i in n:
            i.config(bg="#222")

def move():
    global coord, coord2, colour1, colour2, size1, size2
    moveSnake(coord, colour1, size1)
    moveSnake(coord2, colour2, size2)

def moveSnake(snake, colour, size):
    global end, h, w, colour1, winner, velocity, velocity2, loopIds, speed1, speed2, powers, up1, up2, down1, down2
    if colour == colour1:
        movement = velocity
        speed = speed1
    else:
        movement = velocity2
        speed = speed2
    if up1 == 0 and down2 == 0:
        speed1 = 50
    if up2 == 0 and down1 == 0:
        speed2 = 50
    if not(snake[0][0]+movement[0] >= 0 and snake[0][0]+movement[0] <= (h-1) and snake[0][1]+movement[1] >= 0 and snake[0][1]+movement[1] <= (w-1)):
        end = True
        if colour == colour1:
            winner = "Purple"
        else:
            winner = "Orange"
    if not end:
        for i in snake:
            grid[i[0]][i[1]].config(bg="#222")
        snake.insert(0, [snake[0][0] + movement[0], snake[0][1] + movement[1]])
        if size == 0:
            chance = randint(1,20)
            if chance == 1:
                size = randint(1,3)
        if size != 0:
            snake.pop()
            path = snake[0]
            size -= 1
        else:
            path = snake.pop()
        for i in snake:
            if grid[i[0]][i[1]].cget("bg") == "#222":
                grid[i[0]][i[1]].config(bg=colour)
            elif grid[i[0]][i[1]].cget("bg") in powers:
                power = powers.index(grid[i[0]][i[1]].cget("bg"))
                if power == 0:
                    if colour == colour1:
                        if up1 == 0:
                            speed1 = 1
                            up1 += 5
                    else:
                        if up2 == 0:
                            speed2 = 1
                            up2 += 5
                elif power == 1:
                    if colour == colour1:
                        if down1 == 0:
                            speed2 = 200
                            down1 += 10
                    else:
                        if down2 == 0:
                            speed1 = 200
                            down2 += 10
                else:
                    clear()
            else:
                end = True
                if colour == colour1:
                    winner = "Purple"
                else:
                    winner = "Orange"
        grid[path[0]][path[1]].config(bg=colour)
        chance = randint(1,100)
        if chance == 1:
            genPower()
        loopId = master.after(speed, lambda snake=snake, colour=colour, size=size: moveSnake(snake, colour, size))
        loopIds.append(loopId)
    else:
        pass # WINNER  
    
def changeDir(changeDir):
    global velocity
    if velocity[0] == changeDir[1] or velocity[1] == changeDir[0]:
        velocity = changeDir

def changeDir2(changeDir):
    global velocity2
    if velocity2[0] == changeDir[1] or velocity2[1] == changeDir[0]:
        velocity2 = changeDir
    
def displayGame():
    global h, w
    try:
        main.destroy()

    except: pass

    global wrapper, scoreCount, totalScore, bottom, game
    
    wrapper = Frame(master, bg="#3498db")
    wrapper.pack(fill=X)

    gameTitle = Label(wrapper ,text="Trail Snake", bg="#3498db", fg="#fff", height=1, pady=10, font=('century gothic',(18)))
    gameTitle.pack(side="top")

    gameWrapper = Frame(bg="#222")
    gameWrapper.pack(expand=True, fill=X, padx=200)

    game = Frame(gameWrapper, bg="#eee", highlightbackground="#2980b9", highlightthickness=5)
    game.pack(side="right")
    game.propagate(0)

    scoresMessage = Label(gameWrapper, anchor="w", fg="#fff", text="Scores this round: (5 games played)", font=('century gothic',(20)), bg="#222")
    scoresMessage.pack(anchor="w")

    playerOneScore = Frame(gameWrapper, padx=10, pady=10, bg="#9b59b6")
    playerOneScore.pack(anchor="w")

    playerScoreIncrease = Label(playerOneScore, text="+1", bg="#8e44ad")
    playerScoreIncrease.pack(side="left")

    WINDOW_WIDTH = master.winfo_screenwidth() / 3
    WINDOW_HEIGHT = master.winfo_screenheight() / 2

    game.config(width=WINDOW_WIDTH, height=WINDOW_HEIGHT)
    
    SW = game.winfo_screenwidth()
    SH = game.winfo_screenheight()

    h = round(((SH-10) / 15)) - 10
    w = round(((SW-10) / 15))
    w = round(w / 1.8)

    for n in range(0, h):
        grid.append([])
        for i in range(0,w):
            grid[n].append(Canvas(game, bg="#222", height="15", width="15", highlightthickness="0"))
            grid[n][i].grid(row=n, column=i)

    start()
    move()

def restart(event=None):
    global coord, coord2, grid, velocity, velocity2, score, end, loopIds, time, timerId, speed1, speed2, up1, up2, down1, down2

    for n in grid:
        for i in n:
            i.config(bg="#222")
    for i in loopIds:
        master.after_cancel(i)
    loopIds = []
    velocity = [1,0]
    velocity2 = [-1,0]
    coord = [[10,10]]
    coord2 = [[40,50]]
    score = 0 
    end = False
    time = 0
    speed1 = 50
    speed2 = 50
    up1 = 0
    up2 = 0
    down1 = 0
    down2 = 0
    
    master.after_cancel(timerId)

    start()
    move()
    
def mainMenu():

    global main
    
    main = Frame(master, pady=20, padx=20, bg="#333")
    main.pack(expand=True, padx=20, pady=20)
    
    mainLbl = Label(main ,text="Click Start To Begin!", bg="#3498db",anchor="n", fg="#fff", height=1, width=40, pady=20, font=('century gothic',(18)))
    mainLbl.pack(pady=(0,0), fill=X)
    
    startGame = Button(main, text="Start", command=displayGame, bg="#2ecc71", borderwidth=0, relief=FLAT, height=2, fg="#fff", font=('century gothic',(14)))
    startGame.pack(pady=(10,0), fill=X)

mainMenu()

master.bind("<Up>", lambda Event=None: changeDir2([-1,0]))
master.bind("<Down>", lambda Event=None: changeDir2([1,0]))
master.bind("<Left>", lambda Event=None: changeDir2([0,-1]))
master.bind("<Right>", lambda Event=None: changeDir2([0,1]))

master.bind("<w>", lambda Event=None: changeDir([-1,0]))
master.bind("<s>", lambda Event=None: changeDir([1,0]))
master.bind("<a>", lambda Event=None: changeDir([0,-1]))
master.bind("<d>", lambda Event=None: changeDir([0,1]))
master.bind("<F5>", restart)

master.bind("<F6>", lambda x: master.destroy())
master.mainloop()
\$\endgroup\$
11
  • \$\begingroup\$ So to be clear, everything works correctly, it's just very slow to close? \$\endgroup\$ Commented Mar 8, 2018 at 23:50
  • \$\begingroup\$ Yeah, sorry for not explaining properly! I have a couple kinks to work out, like orange snake ALWAYS winning on a head on collision against purple, but the closing is more of a propriety for me. \$\endgroup\$ Commented Mar 9, 2018 at 0:28
  • \$\begingroup\$ Look at some of the points mentioned in codereview.stackexchange.com/questions/24267/… \$\endgroup\$ Commented Mar 9, 2018 at 3:44
  • \$\begingroup\$ For clarity, you’re closing with f6 and it’s taking long, right? Did you try debugging to see what happens after f6? \$\endgroup\$ Commented Mar 9, 2018 at 18:38
  • \$\begingroup\$ @GürkanÇetin Not sure how I'd check this? [debugging after f6] However, what you've said is correct. \$\endgroup\$ Commented Mar 9, 2018 at 19:11

2 Answers 2

Reset to default
3
\$\begingroup\$

tkinter imports

The following line unnecessarily imports many unused items:

from tkinter import *

It is common to use the following:

import tkinter as tk

This requires you to prefix everything from tkinter with tk, such as:

master = tk.Tk()
main = tk.Frame(master, pady=20, padx=20, bg="#333")

However, the benefit is that the code is more self-documenting in the sense that we don't need to guess where Frame came from.

Unused

This line is unused and can be deleted:

import os

These lines are not needed and can be deleted:

else:
    pass # WINNER

You could add a comment near the if not end: line regarding the winner.

Simpler

These lines:

chance = randint(1,100)
if chance == 1:

can be combined:

if randint(1,100) == 1:

In this line:

for n in range(0, h):

there is no need for the start since 0 is the default:

for n in range(h):

Naming

Regarding the disclaimer, using good coding practices helps everyone to understand the code better, making it more likely to help with the performance issue. Also, code reviews on this site are targeted at everyone who reads them, not just the OP.

The PEP 8 style guide recommends snake_case (oddly enough) for function and variable names.

genPower would be gen_power.

Constants should use all caps:

size1 = 0
size2 = 0

would be:

SIZE1 = 0
SIZE2 = 0

Also, you should specify what 1 and 2 refer to.

try/except

Do not use bare except. Use a specific exception type.

except: pass

Also, that should be 2 lines, not 1.

except:
    pass
\$\endgroup\$
3
\$\begingroup\$

We can unpack each nested list, making accessing the indices "manually" like this unnecessary.

def start():
    global coord, coord2, colour1, colour2
    for i in coord:
        grid[i[0]][i[1]].config(bg=colour1)
    for i in coord2:
        grid[i[0]][i[1]].config(bg=colour2)

Better would be the following, with x and y used in place of more meaningful names.

def start():
    global coord, coord2, colour1, colour2
    for x, y in coord:
        grid[a][b].config(bg=colour1)
    for x, y in coord2:
        grid[x][y].config(bg=colour2)

There are multiple places in your code this can be applied.

Taking a look at this snippet:

        for i in snake:
            if grid[i[0]][i[1]].cget("bg") == "#222":
                grid[i[0]][i[1]].config(bg=colour)
            elif grid[i[0]][i[1]].cget("bg") in powers:
                power = powers.index(grid[i[0]][i[1]].cget("bg"))
                if power == 0:
                    if colour == colour1:
                        if up1 == 0:
                            speed1 = 1
                            up1 += 5
                    else:
                        if up2 == 0:
                            speed2 = 1
                            up2 += 5
                elif power == 1:
                    if colour == colour1:
                        if down1 == 0:
                            speed2 = 200
                            down1 += 10
                    else:
                        if down2 == 0:
                            speed1 = 200
                            down2 += 10
                else:
                    clear()
            else:
                end = True
                if colour == colour1:
                    winner = "Purple"
                else:
                    winner = "Orange"

There's an opportunity to apply the earlier for x, y in ...: lesson, but also to save grid[i[0]][i[1]].cget("bg") to a variable rather than repeating this.

Also, rather than check if this value is in powers and then find the index again, we can simply call index and handle the ValueError if it's not found.

And there is an opportunity to use the conditional expression for assigning winner.

So that we get:

        for x, y in snake:
            bg = grid[x][y].cget("bg")
            if bg == "#222":
                grid[x][y].config(bg=colour)
                continue
                
            try:
                power = powers.index(bg)
                
                if power == 0:
                    if colour == colour1:
                        if up1 == 0:
                            speed1 = 1
                            up1 += 5
                    else:
                        if up2 == 0:
                            speed2 = 1
                            up2 += 5
                elif power == 1:
                    if colour == colour1:
                        if down1 == 0:
                            speed2 = 200
                            down1 += 10
                    else:
                        if down2 == 0:
                            speed1 = 200
                            down2 += 10
                else:
                    clear()
            except ValueError:
                end = True
                winner = "Purple" if colour == colour1 else "Orange"

But even here it's possible to refine this. The else branches followed by an if can just be an elif.

        for x, y in snake:
            bg = grid[x][y].cget("bg")
            if bg == "#222":
                grid[x][y].config(bg=colour)
                continue
                
            try:
                power = powers.index(bg)
                
                if power == 0:
                    if colour == colour1:
                        if up1 == 0:
                            speed1 = 1
                            up1 += 5
                    elif up2 == 0:
                        speed2 = 1
                        up2 += 5
                elif power == 1:
                    if colour == colour1:
                        if down1 == 0:
                            speed2 = 200
                            down1 += 10
                    elif down2 == 0:
                        speed1 = 200
                        down2 += 10
                else:
                    clear()           
            except ValueError:
                end = True
                winner = "Purple" if colour == colour1 else "Orange"
\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.