Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'll soon begin teaching a beginners' programming class. It's voluntary, so I thought I'd make it interesting by teaching Python programming and then introduce the kids to Pygame, so that they can make their own games. To try out Pygame (I've never used it before) and to see exactly how easy it is to make a game, I made a clone of Flappy Bird.

What do you think? Could this be made simpler/shorter? Is there anything in there that I shouldn't teach my students?

Github repo, with images

#!/usr/bin/env python3

"""Flappy Bird, implemented using Pygame."""

import math
import os
from random import randint

import pygame
from pygame.locals import *


FPS = 60
EVENT_NEWPIPE = USEREVENT + 1  # custom event
PIPE_ADD_INTERVAL = 3000       # milliseconds
FRAME_ANIMATION_WIDTH = 3      # pixels per frame
FRAME_BIRD_DROP_HEIGHT = 3     # pixels per frame
FRAME_BIRD_JUMP_HEIGHT = 5     # pixels per frame
BIRD_JUMP_STEPS = 20           # see get_frame_jump_height docstring
WIN_WIDTH = 284 * 2            # BG image size: 284x512 px; tiled twice
WIN_HEIGHT = 512
PIPE_WIDTH = 80
PIPE_PIECE_HEIGHT = BIRD_WIDTH = BIRD_HEIGHT = 32


class PipePair:
    """Represents an obstacle.

    A PipePair has a top and a bottom pipe, and only between them can
    the bird pass -- if it collides with either part, the game is over.

    Attributes:
    x: The PipePair's X position.  Note that there is no y attribute,
        as it will only ever be 0.
    surface: A pygame.Surface which can be blitted to the main surface
        to display the PipePair.
    top_pieces: The number of pieces, including the end piece, in the
        top pipe.
    bottom_pieces: The number of pieces, including the end piece, in
        the bottom pipe.
    """

    def __init__(self, surface, top_pieces, bottom_pieces):
        """Initialises a new PipePair with the given arguments.

        The new PipePair will automatically be assigned an x attribute of
        WIN_WIDTH.

        Arguments:
        surface: A pygame.Surface which can be blitted to the main
            surface to display the PipePair.  You are responsible for
            converting it, if desired.
        top_pieces: The number of pieces, including the end piece, which
            make up the top pipe.
        bottom_pieces: The number of pieces, including the end piece,
            which make up the bottom pipe.
        """
        self.x = WIN_WIDTH
        self.surface = surface
        self.top_pieces = top_pieces
        self.bottom_pieces = bottom_pieces
        self.score_counted = False

    @property
    def top_height_px(self):
        """Get the top pipe's height, in pixels."""
        return self.top_pieces * PIPE_PIECE_HEIGHT

    @property
    def bottom_height_px(self):
        """Get the bottom pipe's height, in pixels."""
        return self.bottom_pieces * PIPE_PIECE_HEIGHT

    def is_bird_collision(self, bird_position):
        """Get whether the bird crashed into a pipe in this PipePair.

        Arguments:
        bird_position: The bird's position on screen, as a tuple in
            the form (X, Y).
        """
        bx, by = bird_position
        in_x_range = bx + BIRD_WIDTH > self.x and bx < self.x + PIPE_WIDTH
        in_y_range = (by < self.top_height_px or
                      by + BIRD_HEIGHT > WIN_HEIGHT - self.bottom_height_px)
        return in_x_range and in_y_range


def load_images():
    """Load all images required by the game and return a dict of them.

    The returned dict has the following keys:
    background: The game's background image.
    bird-wingup: An image of the bird with its wing pointing upward.
        Use this and bird-wingdown to create a flapping bird.
    bird-wingdown: An image of the bird with its wing pointing downward.
        Use this and bird-wingup to create a flapping bird.
    pipe-end: An image of a pipe's end piece (the slightly wider bit).
        Use this and pipe-body to make pipes.
    pipe-body: An image of a slice of a pipe's body.  Use this and
        pipe-body to make pipes.
    """

    def load_image(img_file_name):
        """Return the loaded pygame image with the specified file name.

        This function looks for images in the game's images folder
        (./images/).  All images are converted before being returned to
        speed up blitting.

        Arguments:
        img_file_name: The file name (including its extension, e.g.
            '.png') of the required image, without a file path.
        """
        file_name = os.path.join('.', 'images', img_file_name)
        img = pygame.image.load(file_name)
        # converting all images before use speeds up blitting
        img.convert()
        return img

    return {'background': load_image('background.png'),
            'pipe-end': load_image('pipe_end.png'),
            'pipe-body': load_image('pipe_body.png'),            
            # images for animating the flapping bird -- animated GIFs are
            # not supported in pygame
            'bird-wingup': load_image('bird_wing_up.png'),
            'bird-wingdown': load_image('bird_wing_down.png')}


def get_frame_jump_height(jump_step):
    """Calculate how high the bird should jump in a particular frame.

    This function uses the cosine function to achieve a smooth jump:
    In the first and last few frames, the bird jumps very little, in the
    middle of the jump, it jumps a lot.
    After a completed jump, the bird will have jumped
    FRAME_BIRD_JUMP_HEIGHT * BIRD_JUMP_STEPS pixels high, thus jumping,
    on average, FRAME_BIRD_JUMP_HEIGHT pixels every step.

    Arguments:
    jump_step: Which frame of the jump this is, where one complete jump
        consists of BIRD_JUMP_STEPS frames.
    """
    frac_jump_done = jump_step / float(BIRD_JUMP_STEPS)
    return (1 - math.cos(frac_jump_done * math.pi)) * FRAME_BIRD_JUMP_HEIGHT


def random_pipe_pair(pipe_end_img, pipe_body_img):
    """Return a PipePair with pipes of random height.

    The returned PipePair's surface will contain one bottom-up pipe
    and one top-down pipe.  The pipes will have a distance of
    BIRD_HEIGHT*3.
    Both passed images are assumed to have a size of (PIPE_WIDTH,
    PIPE_PIECE_HEIGHT).

    Arguments:
    pipe_end_img: The image to use to represent a pipe's endpiece.
    pipe_body_img: The image to use to represent one horizontal slice
        of a pipe's body.
    """
    surface = pygame.Surface((PIPE_WIDTH, WIN_HEIGHT), SRCALPHA)
    surface.convert()   # speeds up blitting
    surface.fill((0, 0, 0, 0))
    max_pipe_body_pieces = int(
        (WIN_HEIGHT -            # fill window from top to bottom
        3 * BIRD_HEIGHT -        # make room for bird to fit through
        3 * PIPE_PIECE_HEIGHT) / # 2 end pieces and 1 body piece for top pipe
        PIPE_PIECE_HEIGHT        # to get number of pipe pieces
    )
    bottom_pipe_pieces = randint(1, max_pipe_body_pieces)
    top_pipe_pieces = max_pipe_body_pieces - bottom_pipe_pieces
    # bottom pipe
    for i in range(1, bottom_pipe_pieces + 1):
        surface.blit(pipe_body_img, (0, WIN_HEIGHT - i*PIPE_PIECE_HEIGHT))
    bottom_pipe_end_y = WIN_HEIGHT - bottom_pipe_pieces*PIPE_PIECE_HEIGHT
    surface.blit(pipe_end_img, (0, bottom_pipe_end_y - PIPE_PIECE_HEIGHT))
    # top pipe
    for i in range(top_pipe_pieces):
        surface.blit(pipe_body_img, (0, i * PIPE_PIECE_HEIGHT))
    top_pipe_end_y = top_pipe_pieces * PIPE_PIECE_HEIGHT
    surface.blit(pipe_end_img, (0, top_pipe_end_y))
    # compensate for added end pieces
    top_pipe_pieces += 1
    bottom_pipe_pieces += 1
    return PipePair(surface, top_pipe_pieces, bottom_pipe_pieces)


def main():
    """The application's entry point.

    If someone executes this module (instead of importing it, for
    example), this function is called.
    """

    pygame.init()

    display_surface = pygame.display.set_mode((WIN_WIDTH, WIN_HEIGHT))
    pygame.display.set_caption('Pygame Flappy Bird')

    clock = pygame.time.Clock()

    score_font = pygame.font.SysFont(None, 32, bold=True)  # default font

    # the bird stays in the same x position, so BIRD_X is a constant
    BIRD_X = 50
    bird_y = int(WIN_HEIGHT/2 - BIRD_HEIGHT/2)  # center bird on screen

    images = load_images()

    # timer for adding new pipes
    pygame.time.set_timer(EVENT_NEWPIPE, PIPE_ADD_INTERVAL)
    pipes = []

    steps_to_jump = 2
    score = 0
    done = paused = False
    while not done:
        for e in pygame.event.get():
            if e.type == QUIT or (e.type == KEYUP and e.key == K_ESCAPE):
                done = True
                break
            elif e.type == KEYUP and e.key in (K_PAUSE, K_p):
                paused = not paused
            elif e.type == MOUSEBUTTONUP or (e.type == KEYUP and
                    e.key in (K_UP, K_RETURN, K_SPACE)):
                steps_to_jump = BIRD_JUMP_STEPS
            elif e.type == EVENT_NEWPIPE:
                pp = random_pipe_pair(images['pipe-end'], images['pipe-body'])
                pipes.append(pp)

        clock.tick(FPS)
        if paused:
            continue  # don't draw anything

        for x in (0, WIN_WIDTH / 2):
            display_surface.blit(images['background'], (x, 0))

        for p in pipes:
            p.x -= FRAME_ANIMATION_WIDTH
            if p.x <= -PIPE_WIDTH:  # PipePair is off screen
                pipes.remove(p)
            else:
                display_surface.blit(p.surface, (p.x, 0))

        # calculate position of jumping bird
        if steps_to_jump > 0:
            bird_y -= get_frame_jump_height(BIRD_JUMP_STEPS - steps_to_jump)
            steps_to_jump -= 1
        else:
            bird_y += FRAME_BIRD_DROP_HEIGHT

        # because pygame doesn't support animated GIFs, we have to
        # animate the flapping bird ourselves
        if pygame.time.get_ticks() % 500 >= 250:
            display_surface.blit(images['bird-wingup'], (BIRD_X, bird_y))
        else:
            display_surface.blit(images['bird-wingdown'], (BIRD_X, bird_y))

        # update and display score
        for p in pipes:
            if p.x + PIPE_WIDTH < BIRD_X and not p.score_counted:
                score += 1
                p.score_counted = True

        score_surface = score_font.render(str(score), True, (255, 255, 255))
        score_x = WIN_WIDTH/2 - score_surface.get_width()/2
        display_surface.blit(score_surface, (score_x, PIPE_PIECE_HEIGHT))

        pygame.display.update()

        # check for collisions
        pipe_collisions = [p.is_bird_collision((BIRD_X, bird_y)) for p in pipes]
        if (0 >= bird_y or bird_y >= WIN_HEIGHT - BIRD_HEIGHT or
                True in pipe_collisions):
            print('You crashed! Score: %i' % score)
            break
    pygame.quit()


if __name__ == '__main__':
    # If this module had been imported, __name__ would be 'flappybird'.
    # It was executed (e.g. by double-clicking the file), so call main.
    main()
share|improve this question
17  
Oh how I wish more teachers were like you! Welcome to Code Review! Teach your students to review code! –  Simon André Forsberg Aug 29 at 16:41
    
Hey, why not put this code with the images on GitHub? –  janos Aug 29 at 16:44
    
@janos: good idea. Here it is: github.com/TimoWilken/flappy-bird-pygame –  Timo Aug 29 at 17:03
    
@SimonAndréForsberg: thanks! And, that's good idea -- I'll find some good code they can learn from, or some bad code for error-spotting. –  Timo Aug 29 at 17:08
4  
The frozen GitHub tree at the time this question was asked –  200_success Aug 29 at 18:17

4 Answers 4

up vote 15 down vote accepted
  1. You have docstrings for your functions and classes, which makes your code better than 95% of code submitted to Code Review.

  2. The behaviour of the pipes is split into several pieces: (i) the PipePair class; (ii) the motion, drawing, and destruction logic in main; (iii) the scoring logic in main; (iv) the factory function random_pipe_pair. It would make the code easier to understand and maintain if you collected all the pipe logic into methods on the PipePair class.

  3. Similarly, the behaviour of the bird is distributed among several places: (i) the local variables bird_y and steps_to_jump in main; (ii) the "calculate position of jumping bird" logic; (iii) the flapping animation logic; (iv) the get_frame_jump_height function. It would make the code easier to understand if you collected all the bird logic into methods on a Bird class.

  4. The word "jumping" doesn't seem like a good description of the behaviour of the bird.

  5. Name is_bird_collision make not sense English.

  6. In the collision logic you're effectively testing for intersection of rectangular hit boxes. Pygame provides a Rect class with various collide methods that would make your code clearer, and would make it easier to do things like drawing the hit boxes to help with debugging.

  7. You store the pipes in a list, but this is inefficient when it comes to remove a pipe: list.remove takes time proportional to the length of the list. You should use a set, or, since you know that pipes get created on the right and destroyed on the left, a collections.deque.

  8. When you test for collisions, you store the collision results in a list and then test to see if True is an element of the list. Instead, you should use the built-in function any:

    if any(p.collides_with(bird) for p in pipes):
    

    (This has the additional advantage of short-circuiting: that is, stopping as soon as a collision has been detected, instead of going on to test the remaining pipes.)

  9. You measure time in frames (for example, pipes move leftwards at a particular number of pixels per frame). This has the consequence that you cannot change the framerate without having to change many other parameters. It is more general to measure time in seconds: this makes it possible to vary the framerate.

    (In this kind of simple game you can get away with measuring in frames, but in more sophisticated games you'll need to be able to vary the framerate and so it's worth practicing the necessary techniques.)

  10. In commit 583c3e49 you broke the game by (i) removing the function random_pipe_pair without changing the caller; and (ii) changing the local variable surface to an attribute self.surface in some places but not others.

    We all make commits in error from time to time, but there are four commits after this one, which suggests that you haven't been testing your code before committing it. This is a bad habit to get into!

share|improve this answer
1  
I agree, the bird doesn't really jump, but what verb would you suggest? Fly up? Climb (as used in aviation)? –  Timo Aug 30 at 18:17
3  
I suggest climb/sink, rise/fall, or ascend/descend. –  200_success Aug 30 at 18:36
4  
"flap" would be my choice. –  Gareth Rees Aug 30 at 18:46
1  
All the answers here are good, but I'm accepting this one because it has the most suggestions. –  Timo Aug 31 at 16:40
1  
Alright, I think I've got everything covered. The latest code is in the Github repo. –  Timo Aug 31 at 19:00

This is pretty nice code! I can still nitpitck a bit :)


You could use the cool .. < .. < .. operator for this:

in_x_range = bx + BIRD_WIDTH > self.x and bx < self.x + PIPE_WIDTH

like this:

in_x_range = bx - PIPE_WIDTH < self.x < bx + BIRD_WIDTH

Maybe random_pipe_pair will be slightly more readable if you added a few line breaks.


Moot point, but keeping it anyway, see my conclusion at the bottom.

You don't need the parentheses here:

in_y_range = (by < self.top_height_px or
              by + BIRD_HEIGHT > WIN_HEIGHT - self.bottom_height_px)

and here:

if e.type == QUIT or (e.type == KEYUP and e.key == K_ESCAPE):

and here the outer parens:

elif e.type == MOUSEBUTTONUP or (e.type == KEYUP and
        e.key in (K_UP, K_RETURN, K_SPACE)):

and here:

if (0 >= bird_y or bird_y >= WIN_HEIGHT - BIRD_HEIGHT or
        True in pipe_collisions):

BUT... As you commented, you use the parens mostly to allow breaking long lines (probably following PEP8) without the ugly \. I totally agree with that, so yeah, keep 'em! (In fact I didn't even know this was possible, so thanks for the lesson, teach'!)

share|improve this answer
1  
Good points! About the parentheses: either I'm using them for line continuation so I don't have to use that ugly backslash :), or I'm using them for clarity -- I know the precedence of and and or, but 'explicit is better than implicit', as the Zen of Python says :) –  Timo Aug 29 at 17:16
1  
I don't actually know Python but found the question interesting. About removing the parenthesis from if a or (b and c): I would leave those in, simply because it makes it explicit and you don't need to know/remember the operator precedence. I've worked with about 20 languages now and operator precedence rules is something I got sick of: just add parenthesis and everybody knows how it's supposed to work. –  DarkDust Aug 29 at 17:26
    
I agree that explicit is better than implicit, but this is a bit bordering paranoid coding for me. BUT, I agree with your other point about line breaking, and I updated my post –  janos Aug 29 at 17:29
1  
Changed the code to include 'the cool x < y < z operator' -- thanks! :) –  Timo Aug 29 at 18:48
  • I am not comfortable with pause handling. First, the busy-waiting loop. Second, the paused game just doesn't render anything, but it still serves events, e.g. pipes are added.

  • random_pipe_pair really wants to be a PipePair constructor. Similarly, images['pipe_body'] and images['pipe_end'] should be static members of PipePair.

share|improve this answer
    
Good points, thanks. Let's see how that works out.. I'll commit to Github when I'm finished. –  Timo Aug 29 at 17:10
    
Finally... after some trouble with git, I committed the changes. I'll update the question as well. –  Timo Aug 29 at 18:41
2  
It is against CR policy to edit the code, because it invalidates the review. Please roll back the edit. You are more than welcome to post the updated code as a follow-up question. –  vnp Aug 29 at 18:55

This is neat :)

I think that unless you're tracking the rects which have changed, display.update() is no better than display.flip(), though I suppose for a student it would be easier to read.

I'm curious as to why collisions are being checked after the view updates? I know that in an event loop situation the actual order of processes can be a little lax, especially if you're working at a high FPS, and I see that it presents the code in a way where the end of the loop contains the "exit or don't" code, but I guess I'm of the opinion that all the state checking work should happen before the view is updated. That's probably small potatoes, I've no idea the age or experience of the students you're working with.

I have to agree that the paused handling is a bit weird, just because it kind of starts a discussion about using states to control the flow of the (very short) script. Would rather see something more like "if not paused: do_stuff()" rather than "if paused: continue".

It's also sort of interesting which aspects of the PyGame API you've chosen to work with; for my part, it feels very strange to do an Intro To Games class without working with sprites. The Pygame sprite.Sprite object is a pretty helpful thing to have around! But as I look over the code it seems like it introduces a range of different concepts, so maybe there's no call for adding yet another concept to the mix.

share|improve this answer
    
Thanks! Because I'm breaking from the main loop directly if a collision is detected, I put it at the end when I first coded that part of the game so that the user isn't told that they crashed while the bird is still displayed a few pixels away from the pipe. I'll move all state checking to the beginning of the loop and just set done = True -- like it's done for the QUIT event, for example. –  Timo Aug 29 at 18:17
    
Good point about display.flip() vs display.update() -- I must have overlooked flip when I read through the documentation. –  Timo Aug 29 at 18:19
    
I'm not entirely sure what you mean by if not paused: do_stuff() -- are you suggesting that I extract the entire loop body into another function and just call it if the game isn't paused? –  Timo Aug 29 at 18:21
    
Regarding sprites: I could either create a new Bird class subclassing sprite.Sprite and make PipePair subclass sprite.Group, which would, IMHO, make the game more complex than it needs to be; or I could just replace every usage of images[...] with some_sprite.image or something similar, and replace every display_surface.blit(some_pipe_pair.surface, ...) with some_pipe_pair.draw(display_surface), which wouldn't really simplify the code either, IMHO. But I would definitely introduce sprites somehow, perhaps in a different example. –  Timo Aug 29 at 18:30
    
Well you don't necessarily have to pull all the view stuff out of the view, but I see what you're saying. As for subclassing sprite.Group, I may have misunderstood what you meant, but Group is a container for sprites with convenience methods. For example, MyGroup.draw(DispSurf) would blit all sprites in that group without the use of a for loop; and a call like pygame.sprite.spritecollide(Bird, PipesGroup) would return a list of all the sprites in PipesGroup the bird hit, so a simple "if spritecollide(Bird, PipesGroup):" would serve as hit detection (since an empty list is falsy) –  Stick Aug 29 at 19:14

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.