Communities

Writing
Writing
Codidact Meta
Codidact Meta
The Great Outdoors
The Great Outdoors
Photography & Video
Photography & Video
Scientific Speculation
Scientific Speculation
Cooking
Cooking
Electrical Engineering
Electrical Engineering
Judaism
Judaism
Languages & Linguistics
Languages & Linguistics
Software Development
Software Development
Mathematics
Mathematics
Christianity
Christianity
Code Golf
Code Golf
Music
Music
Physics
Physics
Linux Systems
Linux Systems
Power Users
Power Users
Tabletop RPGs
Tabletop RPGs
Community Proposals
Community Proposals
tag:snake search within a tag
answers:0 unanswered questions
user:xxxx search by author id
score:0.5 posts with 0.5+ score
"snake oil" exact phrase
votes:4 posts with 4+ votes
created:<1w created < 1 week ago
post_type:xxxx type of post
Search help
Notifications
Mark all as read See all your notifications »
Code Reviews

Welcome to Software Development on Codidact!

Will you help us build our independent community of developers helping developers? We're small and trying to grow. We welcome questions about all aspects of software development, from design to code to QA and more. Got questions? Got answers? Got code you'd like someone to review? Please join us.

Comments on A simple game with pygame

Parent

A simple game with pygame

+7
−0

I've just started playing around with pygame and have written a small game in it, of which I'd like a review. Note that I'm not only a complete beginner in pygame, but I also have very little experience in Python in general, therefore I also would welcome comments on my Python code that are not related to pygame specifically.

It is a simple game where a ship moves on the surface of water while submarines move in the opposite direction below it. Your task is to destroy the submarines with water bombs released from the ship. While you can have several bombs (up to 15) moving at the same time, each but the first one costs you score to drop (and you cannot throw another one if your score is too low). Indeed, your only game control is dropping bombs, which you do with the down arrow key.

Destroying submarines give you score, the more the deeper down the ship is. Submarines are spawned randomly, with varying speeds.

Other than the down key, the program also recognises the keys F, to toggle fullscreen mode, and Q to quit the game.

The main file, called uboot.py (U-Boot is the German term for submarine), is as follows:

import pygame
import random

# python files from this game
import settings
from colours import get_colour
from objects import MovingObject

class Game:
    "The game"

    # maximal number of simultaneous submarines
    max_subs = settings.limits["submarine"]

    #maximal number of simultaneous bombs
    max_bombs = settings.limits["bomb"]

    # background (sky) colour
    c_background = get_colour("sky")

    # water colour
    c_water = get_colour("water")

    # colour of the score display
    c_text = get_colour("text")

    # the game's frames per second
    fps = settings.fps

    def __init__(self, width, height):
        """
        Initialize the game with screen dimensions width x height
        """
        self.width = width
        self.height = height
        self.waterline = int(settings.sky_fraction * height)

        self.running = False

        pygame.init()
        self.screen = pygame.display.set_mode((width,height))
        pygame.display.set_caption(settings.game_name)
        pygame.mouse.set_visible(False)
        pygame.key.set_repeat(0)

        self.clock = pygame.time.Clock()

        # create the ship
        self.ship = MovingObject(settings.image_files["ship"],
                                 start = (0, self.waterline),
                                 adjust_start = (-0.5,0),
                                 end = (self.width, self.waterline),
                                 adjust_end = (0.5,0),
                                 speed = settings.speeds["ship"],
                                 origin = (0.5,1),
                                 repeat = True)

        # initially there are no submarines nor bombs
        self.submarines = []
        self.bombs = []

        # spawn a submarine on average every 3 seconds
        self.p_spawn = settings.spawn_rates["submarine"]/self.fps

        self.score = 0

        self.font = pygame.font.SysFont(settings.font["name"],
                                        settings.font["size"])

    def write_string(self, string, position):
        """
        Write a string at a given position on screen
        """
        text = self.font.render(string, True, self.c_text)
        self.screen.blit(text, position)

    def moving_objects(self):
        yield self.ship

        for sub in self.submarines:
            yield sub

        for bomb in self.bombs:
            yield bomb

    def draw(self):
        """
        Draw the game graphics
        """
        self.screen.fill(Game.c_background)
        pygame.draw.rect(self.screen, Game.c_water,
                         (0,
                          self.waterline,
                          self.width,
                          self.height - self.waterline))

        self.ship.draw_on(self.screen)

        for obj in self.moving_objects():
            obj.draw_on(self.screen)

        self.write_string("Bombs available: "
                          + str(self.get_available_bombs()),
                          (20, 20))

        self.write_string("Bomb cost: " + str(self.get_bomb_cost()),
                          (20,50))

        self.write_string("Score: " + str(self.score),
                          (20+self.width//2, 20))
        
        pygame.display.flip()

    def get_bomb_cost(self, count=1):
        "Returns the score cost of dropping another count bombs"
        l = len(self.bombs)
        return sum((l+k)**2 for k in range(count))

    def get_available_bombs(self):
        "Returns the maximum number of extra bombs that can be thrown"
        available_bombs = self.max_bombs - len(self.bombs)
        while self.get_bomb_cost(available_bombs) > self.score:
               available_bombs -= 1
        return available_bombs

    def drop_bomb(self):
        "Drop a bomb, if possible"

        # don't drop a new bomb if there already exist a naximal
        # number of them, or the score would go negative
        if self.get_available_bombs() > 0:
            ship_pos = self.ship.get_position();

            # don't drop a bomb off-screen
            if ship_pos[0] > 0 and ship_pos[0] < self.width:
                # the score must be updated before adding the new bomb
                # because adding the bomb changes the cost
                self.score -= self.get_bomb_cost();

                newbomb = MovingObject(settings.image_files["bomb"],
                                       start = ship_pos,
                                       end = (ship_pos[0], self.height),
                                       speed = settings.speeds["bomb"],
                                       origin = (0.5, 0))
                self.bombs.append(newbomb)

    def spawn_submarine(self):
        "Possibly spawn a new submarine"
        if random.uniform(0,1) < self.p_spawn:
            ship_speed = self.ship.speed

            total_depth = self.height - self.waterline
            min_depth = 0.1*total_depth + self.waterline
            max_depth = self.height - 20
            sub_depth = random.uniform(min_depth, max_depth)
            sub_speed = random.uniform(settings.speeds["submarine_min"],
                                       settings.speeds["submarine_max"])

            newsub = MovingObject(settings.image_files["submarine"],
                                  start = (self.width, sub_depth),
                                  end = (0, sub_depth),
                                  adjust_end = (-1,0),
                                  speed = sub_speed)
            
            self.submarines.append(newsub)                  

    def handle_hits(self):
        """
        Check if any bomb hit any submarine, and if so, remove both
        and update score
        """
        for sub in self.submarines:
            for bomb in self.bombs:
                bb_sub = sub.get_bounding_box()
                bb_bomb = bomb.get_bounding_box()
                if bb_sub.colliderect(bb_bomb):
                    subpos = sub.get_position()
                    self.score += int((subpos[1] - self.waterline) /
                                      self.height * 20 + 0.5)
                    sub.deactivate()
                    bomb.deactivate()

    def handle_events(self):
        """
        Handle all events
        """
        for event in pygame.event.get():
            if event.type == pygame.QUIT:
                self.running = False

            if event.type == pygame.KEYDOWN:
                # Down arrow drops a bomb
                if event.key == pygame.K_DOWN:
                    self.drop_bomb()

                # F toggles fullscreen display
                elif (event.key == pygame.K_f):
                    size = (self.width, self.height)
                    if self.screen.get_flags() & pygame.FULLSCREEN:
                        pygame.display.set_mode(size)
                    else:
                        pygame.display.set_mode(size, pygame.FULLSCREEN)

                # Q quits the game
                elif event.key == pygame.K_q:
                    pygame.event.post(pygame.event.Event(pygame.QUIT))
        
    def update_state(self):
        """
        Update the state of the game
        """
        # move all objects
        for sub in self.moving_objects():
            sub.move(1/self.fps)

        # handle bombs hitting submarines
        self.handle_hits()

        # remove inactive objects
        self.submarines = [sub for sub in self.submarines if sub.is_active()]
        self.bombs = [bomb for bomb in self.bombs if bomb.is_active()]

        # spawn new submarines at random
        if len(self.submarines) < Game.max_subs:
            self.spawn_submarine()

    def run(self):
        """
        Run the game
        """
        self.running = True
        while self.running:
            self.draw()
            self.clock.tick(60)
            self.handle_events()
            self.update_state()

if __name__=='__main__':
    Game(settings.width, settings.height).run()

There are three further code files.

The first one, objects.py, contains a class representing any moving object in the game. Objects are moving along a straight line, either repeatedly (this is used for the ship) or just once (after which they get deactivated, which basically means they are no longer drawn and can get discarded). It looks like this:

import pygame

class MovingObject:
    "This class represents any moving object in the game."

    # A storage for images, so that they aren't loaded each time
    # another object uses the same image is
    imagestore = {}

    def __init__(self, path, start, end, speed,
                 origin = (0,0), repeat=False,
                 adjust_start = (0,0), adjust_end = (0,0)):
        """
        Create a new moving object.

        Mandatory Arguments:
          path:          the file path to the image to display
          start:         the pixel at which the movement starts
          end:           the pixel at which the movement ends
          speed:         the fraction of the distance to move per second

        Optional arguments:
          origin:        which point of the image to use for placement
          repeat:        whether to repeat the movement
          adjust_start:  adjustment of the start position
          adjust_end:    adjustment of the end position

        All coordinates in the optional arguments are in units of
        image width or image height, as opposed to pixel coordinates
        as used in the mandatory arguments. For example, if the image
        has width of 5 pixels and height of 4 pixels, the arguments

           start = (5,5), adjust_start = (-1,0.5)

        will result in an actual starting point of (0,7).
        """
        if not path in MovingObject.imagestore:
            MovingObject.imagestore[path] =\
                 pygame.image.load(path).convert_alpha()
        self.image = MovingObject.imagestore[path]

        width = self.image.get_width()

        self.start = tuple(s + width * a for s,a in zip(start,adjust_start))
        self.end = tuple(e + width * a for e,a in zip(end,adjust_end))

        self.repeat = repeat

        self.pos = self.start

        self.speed = speed
        self.dist = 0

        self.disp = (-self.image.get_width()*origin[0],
                     -self.image.get_height()*origin[1])

        self.active = True

    def move(self, seconds):
        """
        Move the object.

        Arguments:
          seconds: The number of seconds passed.
        """
        if self.active:
            self.dist += self.speed * seconds
            if self.dist > 1:
                if self.repeat:
                    self.dist = 0
                else:
                    self.active = False

    def get_position(self, displace = False):
        """
        Return the current position of the object.

        Arguments:
          displace: If True, give the position of the upper left corner.
                    If False (default), give the position of the origin.

        Return value: The pixel coordinates of the object.
        """
        return tuple(int(s + self.dist*(e-s) + (d if displace else 0))
                     for e, s, d in zip(self.end, self.start, self.disp))

    def draw_on(self, surface):
        """
        Draw the object on a pygame surface, if active
        """
        if self.is_active():
            surface.blit(self.image, self.get_position(True))

    def is_active(self):
        """
        Returns true if the object is active, False otherwise
        """
        return self.active

    def deactivate(self):
        """
        Set the object's state to not active.
        """
        self.active = False

    def get_bounding_box(self):
        """
        Get the bounding box of the objects representation on screen
        """
        pos = self.get_position()
        return pygame.Rect(pos,
                           (self.image.get_width(),
                            self.image.get_height()))

Next, there is the file colours.py which provide a single function to translate colour names into RGB values. For this it uses both names defined in the settings file (posted below) and names from X11's rgb.txt, which I also copied to the game directory. Strictly speaking it currently only needs three lines of that file, which I'll reproduce below, but if you'd like to use the complete file, you can find it here (or locally in /etc/X11/rgb.txt if you are on a Linux system with X11 installed). The three lines actually needed are

  0   0   0		black
  0   0 255		blue
135 206 235		sky blue

so if you save these three lines in a text file named rgb.txt (be careful to keep the tab characters intact!) it should work, too.

Here's the file colours.py:

import settings

# get the colour names from X11's rgb.txt
rgbvalues = {}
with open("rgb.txt", "r") as rgbfile:
    for line in rgbfile:
        if line == "" or line[0] == '!':
            continue
        rgb, name = line.split('\t\t')
        rgbvalues[name.strip()] = tuple(int(value) for value in rgb.split())

def get_colour(name):
    """
    Get the rgb values of a named colour.

    The name is looked up in the settings, or failing that, in the rgb
    database. If the settings give a string as colour, that string is
    again looked up, otherwise the result is assumed to be a valid
    colour representation and returned.
    """
    while name in settings.colours:
        colour = settings.colours[name]
        if type(colour) is str:
            name = colour
        else:
            return colour
    return rgbvalues[name]

Finally, there is a file settings.py which, as the name says, contains various game settings, such as the screen resolution. Here it is:

# Settings for the uboot game

# Name of the game
game_name = "U-Boot"

# screen resolution/window size
width = 1024
height = 768

# fraction of the screen that's sky
sky_fraction = 0.2

# file names of the game graphics
image_files = {
    "ship": "schiff.png",
    "submarine": "Uboot.png",
    "bomb": "bomb.png"
    }

# frame rate
fps = 60

# speeds of objects
speeds = {
    "ship": 0.1,
    "submarine_min": 0.05,
    "submarine_max": 0.2,
    "bomb": 0.1
    }

# maximum number of objects
limits = {
    "submarine": 10,
    "bomb": 15
    }

# spawn rate (in average spawns per second) of randomly spawned obcets
# (currently only submarines)
spawn_rates = {
    "submarine": 1/3
    }

# colours used in the game
colours = {
    "sky": "sky blue",
    "water": "blue",
    "text": "black"
    }

# font used in the game
font = {
    "name": "Courier New",
    "size": 30
    }

Also, the game loads three graphics files with images of a ship silhouette, a submarine silhouette, and a bomb. Here they are:

The ship, schiff.png: ship silhuette

The submarine, Uboot.png: submarine silhuette

The bomb, bomb.png: bomb image

Im running the game using the command

python3 ./uboot.py

In case it matters, the installed version of Python is 3.8.10, and the version of pygame is 1.9.6.

History
Why does this post require attention from curators or moderators?
You might want to add some details to your flag.
Why should this post be closed?

0 comment threads

Post
+4
−0

Overall the structure seems sensible.

Style

It's Python style to have two blank lines after a function or method, not just one.

You seem to be trying to line wrap before 80 characters. Maybe you have a reason for that, but I don't think that's a sensible width in general any more. In fact, I think that PEP8 is far too conservative with the suggestion that a team might reach consensus to stretch to 99 chars. I wouldn't even consider wrapping a line before about 120 chars.

Inelegant code

        self.ship.draw_on(self.screen)

        for obj in self.moving_objects():
            obj.draw_on(self.screen)

This draws the ship twice.


    def spawn_submarine(self):
        "Possibly spawn a new submarine"
        if random.uniform(0,1) < self.p_spawn:
            ship_speed = self.ship.speed

ship_speed isn't used here.


        while self.running:
            self.draw()
            self.clock.tick(60)

Should that be self.clock.tick(self.fps)?

State representation

There are two things which feel slightly clunky to me. The bigger one is the movement code in MovingObject. From a numerical analysis perspective it's preferable to use linear interpolation rather than increments of the base position, but physics engines tend to work with current position, velocity, and acceleration rather than current path and parameterised position along it. The principle of least surprise suggests that it would be worth adopting the same approach.

The other thing is the separate structures to hold the ship, the submarines, and the bombs. This means that you need a moving_objects method to concatenate, and have separate

        self.submarines = [sub for sub in self.submarines if sub.is_active()]
        self.bombs = [bomb for bomb in self.bombs if bomb.is_active()]

I wonder how inelegant it would be to have each MovingObject increment a counter (probably literally a Counter from collections) on creation and decrement it when deactivated. That would then handle the one thing which really benefits from separate lists, which is knowing how many submarines and bombs are currently active.

Robustness

I bodged the downloads of the assets, and the first time I tried to test the game it broke with an error when it tried to load Uboot.png (I'd saved it as UBoot.png on a case-sensitive filesystem). It might be worth adding a startup check which loads all of the assets and fails early if some of them are missing.

History
Why does this post require attention from curators or moderators?
You might want to add some details to your flag.

1 comment thread

Thank you, and some remarks (4 comments)
Thank you, and some remarks
celtschk‭ wrote about 3 years ago

Thank you very much for the analysis.

On the two line separation, I didn't now that convention, but it makes a lot of sense. I'll definitely do that.

On the 80 character line length, my Emacs opens up at that width, and I like to have space for a terminal (and occasionally other windows) besides of it. Yes, my screen is larger, but I also have more things on it.

On the inelegant code, good points on everything; in all three cases they were actually stuff I forgot when I improved the code.

On the state representation: You're right on the duplication (and indeed, in the mean time I already unified that), but you're wrong on that the counts are the only place the distinction matters. The hit code definitely cares about it as well (only bombs hitting submarines give score).

Thanks for the suggestion of keeping separate counts; my current post-unification solution of counting each time is far from elegant.

You've got a very good point about robustness, too.

celtschk‭ wrote about 3 years ago

I just noticed that I forgot replying to your remark on the movement code.

Actually when I started writing the code, I first went with the increments version, but given the various movement directions, I quickly realised that testing the end condition would be quite complex; therefore I decided to go the interpolation way instead, where the end condition was a simple comparison with 1.

But maybe there's a simple trick that I didn't see that would make that condition easy also in the increment model.

Peter Taylor‭ wrote about 3 years ago

Are collisions other than bomb with submarine even possible? Although, sure, relying on that constrains future expansion.

I think that for incremental movement, a simple test would be whether the bounding box intersects the screen area.

celtschk‭ wrote about 3 years ago

Yes, submarines can “collide” with each other (they just move through each other). If I were not distinguishing between bombs and submarines, then each such collision would cause a collision detection. While the submarines disappearing wouldn't be that bad (well, the submarines collided, after all), getting score for that surely isn't a good idea. You shouldn't get score just for sitting and waiting for random events happening.

I've actually gotten a new idea on this, though: By making it a dictionary of lists, I could avoid code duplication by looping through the dict, yet still restrict iterations to specific object types where required.

On the bounding box test, that's a good idea; actually I should have thought of that myself. It should also allow me to get rid of start_adjustment and end_adjustment which aren't too elegant anyway. Thank you for that.