Simple enemy movement simulation on Tkinter

Question

Kind of a noob at Python here, and this is one of my first "big"(big for a beginner like me) project that I undertook with Tkinter and Pynput. Basically, this code will simulate an enemy's movement pattern based on some conditions that I made(you will be able to see the different "phases" being printed out on the console). You can then control the player using the arrow keys.

I would like some advice on what I should improve on for future projects. Should I add more comments? Is the code structured well? etc.

import math
import tkinter as tk
from pynput import keyboard
class Application:
 def __init__(self, master, height = 800, width = 800, updatesPerSecond = 10, safeCircle = True):
 self.height = height
 self.width = width
 self.root = master
 self.updatesPerSecond = updatesPerSecond
 self.player = Player()
 self.enemy = Enemy()
 self.safeCircle = safeCircle
 self.canvas = tk.Canvas(self.root, height = self.height, width = self.width)
 self.canvas.pack()
 self.player_rectangle = self.canvas.create_rectangle(self.player.x-self.player.hLength, self.player.y-self.player.hLength, self.player.x+self.player.hLength, self.player.y+self.player.hLength)
 self.enemy_rectangle = self.canvas.create_rectangle(self.enemy.x-self.player.hLength, self.enemy.y-self.player.hLength, self.enemy.x+self.player.hLength, self.enemy.y+self.player.hLength)
 if self.safeCircle:
 self.safe_circle = self.canvas.create_oval(self.player.x-self.enemy.safe_distance, self.player.y-self.enemy.safe_distance, self.player.x+self.enemy.safe_distance, self.player.y+self.enemy.safe_distance)
 self.keypress_list = []
 self.listener = keyboard.Listener(on_press = self.on_press, on_release = self.on_release)
 self.listener.start()
 self.player_movement()
 self.enemy_movement()
 def player_movement(self):
 if "down" in self.keypress_list:
 self.player.update_y(self.player.speed)
 if "up" in self.keypress_list:
 self.player.update_y(-self.player.speed)
 if "left" in self.keypress_list:
 self.player.update_x(-self.player.speed)
 if "right" in self.keypress_list:
 self.player.update_x(self.player.speed)
 self.player.boundary_check(self.height, self.width)
 self.canvas.coords(self.player_rectangle, self.player.x-self.player.hLength, self.player.y-self.player.hLength, self.player.x+self.player.hLength, self.player.y+self.player.hLength)
 if self.safeCircle:
 self.canvas.coords(self.safe_circle, self.player.x-self.enemy.safe_distance, self.player.y-self.enemy.safe_distance, self.player.x+self.enemy.safe_distance, self.player.y+self.enemy.safe_distance)
 self.root.after(1000//self.updatesPerSecond, self.player_movement)
 def enemy_movement(self):
 self.enemy.update_pos(self.player)
 self.enemy.boundary_check(self.height, self.width)
 self.canvas.coords(self.enemy_rectangle, self.enemy.x-self.enemy.length/2, self.enemy.y-self.enemy.length/2, self.enemy.x+self.enemy.length/2, self.enemy.y+self.enemy.length/2)
 self.root.after(1000//self.updatesPerSecond, self.enemy_movement)
 def key_test(self, key):
 try:
 return key.name
 except:
 return
 def on_press(self, key):
 key = self.key_test(key)
 if not key in self.keypress_list:
 self.keypress_list.append(key)
 def on_release(self, key):
 key = self.key_test(key)
 self.keypress_list.remove(key)
class SimObject:
 def __init__(self, x, y, speed, length):
 self.x = x
 self.y = y
 self.speed = speed
 self.length = length
 self.hLength = self.length/2
 def boundary_check(self, height, width):
 if self.x - self.hLength < 0:
 self.x = self.hLength
 if self.y - self.hLength < 0:
 self.y = self.hLength
 if self.x + self.hLength > width:
 self.x = width - self.hLength
 if self.y + self.hLength > height:
 self.y = height - self.hLength
 def update_x(self, offset):
 self.x+=offset
 def update_y(self, offset):
 self.y+=offset
class Player(SimObject):
 def __init__(self, x = 400, y = 400, speed = 10, length = 20):
 super().__init__(x, y, speed, length)
class Enemy(SimObject):
 def __init__(self, x = 10, y = 10, speed = 5, length = 20, safe_distance = 100):
 super().__init__(x, y, speed, length)
 self.safe_distance = safe_distance
 self.last_phase = -1
 def update_phase(self, n):
 phase_list=[f"{i} Phase" for i in ["Orbit", "Rush", "Run"]]
 if self.last_phase!=n:
 print(phase_list[n])
 self.last_phase = n
 def update_pos(self, player):
 PI=math.pi
 dx=player.x-self.x
 dy=player.y-self.y
 g_to_p_ang=math.atan2(dy,dx)
 p_to_g_ang=PI+g_to_p_ang
 dist=math.sqrt(dx*dx+dy*dy)
 ang_increase=self.speed/self.safe_distance
 t=p_to_g_ang
 if abs(dist-self.safe_distance)<=self.speed:#near the orbit
 self.update_phase(0)
 t+=ang_increase
 self.x=self.safe_distance*math.cos(t)+player.x
 self.y=self.safe_distance*math.sin(t)+player.y
 elif dist>self.safe_distance:#far from orbit
 self.update_phase(1)
 self.update_x(self.speed*math.cos(g_to_p_ang))
 self.update_y(self.speed*math.sin(g_to_p_ang))
 elif dist<self.safe_distance:#far inside of orbit
 self.update_phase(2)
 self.update_x(self.speed*math.cos(p_to_g_ang))
 self.update_y(self.speed*math.sin(p_to_g_ang))
root = tk.Tk()
root.resizable(0,0)
root.title("Enemy Movement Test")
application = Application(root)
tk.mainloop()

Answer 1

• Format your code according to PEP-8, there are automatic checker and even automatic formatters for that.

• This is often considered as code smell:

 def key_test(self, key):
 try:
 return key.name
 except:
 return

See: https://stackoverflow.com/questions/10594113/bad-idea-to-catch-all-exceptions-in-python

• Some methods are longer than I would consider readable and some of repetitive code. Try to extract some repeating code block as a methods and some repetitive expressions as well-named local variables to explain the process.

Answer 2

In addition to what Roman Pavelka suggests:

• Represent your keypress_list as a keypresses set (note that it's not helpful to embed the type of a variable in its name; this is what type hints are for)
• Factor out rectangle-with-margin calculation routines into your SimObject class
• Do not 'start' anything in your constructor. 'start' in a separate routine or perhaps the entry method of a context manager, stopping in the corresponding exit routine.
• Do not assign a boolean to self.safeCircle; it should be an Optional (i.e. an object or None)
• Do not run separate timers for your enemy and player objects; just use one
• key_test should be using getattr with a None default which will achieve the same effect in a more explicit and safe way
• Add PEP484 type hints
• Do not add the key name to the keypress set if the key name is None
• Rephrase your boundary_check - which does not check at all (nothing is returned), so should be called something like enforce_bounds - as a series of min and max calls
• Represent last_phase as an enumeration for better maintainability and legibility
• No need to import pi if you just negate the coordinate deltas based on whether you are in the rush or run phase
• In update_pos, your last else needs no condition; that's redundant
• Move the logic at the bottom into a main guard

Suggested

import enum
import math
import tkinter as tk
from enum import Enum
from typing import Optional, Tuple
from pynput import keyboard
from pynput.keyboard import Key
class Application:
 def __init__(
 self, master: tk.Tk, height: int = 800, width: int = 800,
 updates_per_second: int = 10, safe_circle: bool = True,
 ):
 self.height = height
 self.width = width
 self.root = master
 self.updates_per_second = updates_per_second
 self.player = Player()
 self.enemy = Enemy()
 self.canvas = tk.Canvas(self.root, height=self.height, width=self.width)
 self.canvas.pack()
 self.player_rectangle = self.canvas.create_rectangle(*self.player.rect)
 self.enemy_rectangle = self.canvas.create_rectangle(*self.enemy.rect)
 if safe_circle:
 self.safe_circle = self.canvas.create_oval(
 *self.player.margin_rect(self.enemy.safe_distance)
 )
 else:
 self.safe_circle = None
 self.keypresses = set()
 self.listener = keyboard.Listener(on_press=self.on_press, on_release=self.on_release)
 def __enter__(self) -> 'Application':
 self.listener.start()
 self.movement()
 return self
 def __exit__(self, exc_type, exc_val, exc_tb) -> None:
 self.listener.stop()
 def movement(self) -> None:
 self.player_movement()
 self.enemy_movement()
 self.root.after(1000 // self.updates_per_second, self.movement)
 def player_movement(self) -> None:
 if "down" in self.keypresses:
 self.player.update_y(self.player.speed)
 if "up" in self.keypresses:
 self.player.update_y(-self.player.speed)
 if "left" in self.keypresses:
 self.player.update_x(-self.player.speed)
 if "right" in self.keypresses:
 self.player.update_x(self.player.speed)
 self.player.enforce_bounds(self.height, self.width)
 self.canvas.coords(self.player_rectangle, *self.player.rect)
 if self.safe_circle:
 self.canvas.coords(
 self.safe_circle,
 *self.player.margin_rect(self.enemy.safe_distance)
 )
 def enemy_movement(self) -> None:
 self.enemy.update_pos(self.player.x, self.player.y)
 self.enemy.enforce_bounds(self.height, self.width)
 self.canvas.coords(
 self.enemy_rectangle,
 *self.enemy.margin_rect(self.enemy.length / 2),
 )
 @staticmethod
 def key_test(key: Key) -> Optional[str]:
 return getattr(key, 'name', None)
 def on_press(self, key: Key) -> None:
 key = self.key_test(key)
 if key is not None:
 self.keypresses.add(key)
 def on_release(self, key: Key) -> None:
 key = self.key_test(key)
 self.keypresses.discard(key)
@enum.unique
class EnemyPhase(Enum):
 ORBIT = 'Orbit'
 RUSH = 'Rush'
 RUN = 'Run'
class SimObject:
 def __init__(self, x: int, y: int, speed: int, length: int):
 self.x = x
 self.y = y
 self.speed = speed
 self.length = length
 self.h_length = self.length / 2
 def enforce_bounds(self, height: int, width: int) -> None:
 self.x = max(self.h_length, min(width - self.h_length, self.x))
 self.y = max(self.h_length, min(height - self.h_length, self.y))
 def update_x(self, offset: float) -> None:
 self.x += offset
 def update_y(self, offset: float) -> None:
 self.y += offset
 def margin_rect(self, margin: float) -> Tuple[float, float, float, float]:
 return (
 self.x - margin, self.y - margin,
 self.x + margin, self.y + margin,
 )
 @property
 def rect(self) -> Tuple[float, float, float, float]:
 return self.margin_rect(self.h_length)
class Player(SimObject):
 def __init__(self, x: int = 400, y: int = 400, speed: int = 10, length: int = 20):
 super().__init__(x, y, speed, length)
class Enemy(SimObject):
 def __init__(
 self, x: int = 10, y: int = 10, speed: int = 5, length: int = 20,
 safe_distance: int = 100,
 ):
 super().__init__(x, y, speed, length)
 self.safe_distance = safe_distance
 self.last_phase = EnemyPhase.RUSH
 def update_phase(self, phase: EnemyPhase) -> None:
 if self.last_phase != phase:
 self.last_phase = phase
 print(f'{phase.value} Phase')
 def update_pos(self, player_x: float, player_y: float) -> None:
 dx = self.x - player_x
 dy = self.y - player_y
 p_to_g_ang = math.atan2(dy, dx)
 dist = math.sqrt(dx*dx + dy*dy)
 if abs(dist - self.safe_distance) <= self.speed: # near the orbit
 ang_increase = self.speed / self.safe_distance
 t = p_to_g_ang + ang_increase
 self.update_phase(EnemyPhase.ORBIT)
 self.x = self.safe_distance * math.cos(t) + player_x
 self.y = self.safe_distance * math.sin(t) + player_y
 else:
 sx = self.speed * math.cos(p_to_g_ang)
 sy = self.speed * math.sin(p_to_g_ang)
 if dist > self.safe_distance: # far from orbit
 self.update_phase(EnemyPhase.RUSH)
 self.update_x(-sx)
 self.update_y(-sy)
 else: # far inside of orbit
 self.update_phase(EnemyPhase.RUN)
 self.update_x(sx)
 self.update_y(sy)
def main():
 root = tk.Tk()
 root.resizable(0, 0)
 root.title("Enemy Movement Test")
 with Application(root):
 tk.mainloop()
if __name__ == '__main__':
 main()

• Aiden Chow: Thanks for the review! As I did just start Python not long ago, I still have some trouble comprehending many parts of your suggested code, so I will need some time to be able to understand it. For now I just want to ask, do I need to install the enum and typing modules, and could you explain in more detail what those modules are doing?
• Reinderien: Both modules are built-in; you have them already. typing adds non-runtime hints for programmers and static analysers to better understand your code. Enumerations are used to represent a variable that can take a constrained set of values.