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