1
\$\begingroup\$

I'm a newbie to Python, and I was wondering whether anyone could give me any pointers on improving the following code:

import pygame, sys, time
from pygame.locals import *

# Define defaults
bg = (0,0,0) # background colour
spl = (255,255,255) # splash colour
ver = "INDEV v1.0" # current version
img = "icn.png" # corner icon (UNUSED)
cur = "cur.png" # cursor icon
player = "player.png" # player
logo = "pixl.png" # splash logo
currtile_x = 0
currtile_y = 0

# Player details
px = 20
py = 20
speedy = 25
up = False
down = False
left = False
right = False
singlerun = 1


# Load Map
with open('townhall.map', 'r') as f:
    for line in f:
            for character in line:
                if character == "\n":
                    print "Newline"
                else:
                    if character == "x":
                        print "WALL"
                    else: 
                        if character == "a":
                            print "LAND"



# Other
completed = 0
clock = pygame.time.Clock()
splashboot = 0

# Initialise screen
pygame.init()
pygame.mouse.set_visible(False)
screen = pygame.display.set_mode((640, 420))
pygame.display.set_caption('The Missing Piece ' + ver)

# Unused
#icon = pygame.image.load(img).convert_alpha()
#pygame.display.set_icon(icon)

# Initialise sprites
try:
    cursor = pygame.image.load(cur).convert_alpha()
    logo = pygame.image.load(logo).convert_alpha()
    player = pygame.image.load(player).convert_alpha()
except:
    print "Unexpected error loading sprites!"
    raw_input("Press ENTER to exit")
    pygame.quit()
    raise

# Splash screen
while splashboot != 25:
    for event in pygame.event.get():
        if event.type == QUIT:
            pygame.quit()
    splash = pygame.Surface(screen.get_size())
    splash = splash.convert()
    splash.fill(spl)
    x, y = screen.get_size()
    gx = x - 600
    yx = (y / 2) - 25
    font = pygame.font.Font(None, 20)
    splashver = font.render(ver, 20, (0, 0, 0))
    screen.blit(splash, (0, 0))
    screen.blit(logo, (gx,yx))
    screen.blit(splashver, (520, yx + 50))
    pygame.display.update()
    splashboot += 1
    time.sleep(0.1)
    print splashboot


# Fill background
background = pygame.Surface(screen.get_size())
background = background.convert()
background.fill(bg)

# Define instructions
font = pygame.font.Font(None, 20)
ver = font.render("The Missing Piece " + ver, 20, (0, 0, 0))
conl1 = font.render("W = UP", 20, (0, 0, 0))
conl2 = font.render("A = LEFT", 20, (0, 0, 0))
conl3 = font.render("S = DOWN", 20, (0, 0, 0))
conl4 = font.render("D = RIGHT", 20, (0, 0, 0))


# Event loop
while completed != 1:
    dt = clock.tick(60)
    speed = speedy / float(dt)
    for event in pygame.event.get():
        if event.type == QUIT:
            completed = 1
            pygame.quit()
        if event.type == KEYDOWN:
            if event.key == K_LEFT:
                left = True
            if event.key == K_RIGHT:
                right = True   
            if event.key == K_UP:
                up = True
            if event.key == K_DOWN:
                down = True
            if event.key == K_a:
                left = True
            if event.key == K_d:
                right = True   
            if event.key == K_w:
                up = True
            if event.key == K_s:
                down = True
        if event.type == KEYUP:
            if event.key == K_LEFT:
                left = False
            if event.key == K_RIGHT:
                right = False   
            if event.key == K_UP:
                up = False
            if event.key == K_DOWN:
                down = False
            if event.key == K_a:
                left = False
            if event.key == K_d:
                right = False   
            if event.key == K_w:
                up = False
            if event.key == K_s:
                down = False

    # direction scripts

    if up:
        py -= speed
    if down:
        py += speed
    if left:
        px -= speed
    if right:
        px += speed

    # cursor settings
    mousex,mousey = pygame.mouse.get_pos()
    mousex -= cursor.get_width()/2
    mousey -= cursor.get_height()/2
    x, y = screen.get_size()
    gx = x - 200

    # draw background
    screen.blit(background, (0, 0))

    #pygame.draw.rect(screen, (255,255,255), (10,10,200,200), 0) # testbox
    pygame.draw.rect(screen, (255,255,255), (gx,0,200,y), 0) # sidebar

    # draw player
    screen.blit(player, (px, py))

    # draw version
    screen.blit(ver, (gx,1))

    # draw instructions
    screen.blit(conl1, (gx+2,20))
    screen.blit(conl2, (gx+2,32))
    screen.blit(conl3, (gx+2,44))
    screen.blit(conl4, (gx+2,56))

    # draw cursor
    screen.blit(cursor, (mousex, mousey))

    pygame.display.update()

I have been given one or two suggestions about snippets of the code, but I would like to know how I can make my code more efficient and simpler. I seem to have gone the long / difficult way around.

\$\endgroup\$
3
\$\begingroup\$
import pygame, sys, time
from pygame.locals import *

# Define defaults
bg = (0,0,0) # background colour
spl = (255,255,255) # splash colour
ver = "INDEV v1.0" # current version
img = "icn.png" # corner icon (UNUSED)
cur = "cur.png" # cursor icon
player = "player.png" # player
logo = "pixl.png" # splash logo

According to python convention, global constants should be in ALL_CAPS. I would also suggest not using these really short abbreviations. Spell out your words.

currtile_x = 0
currtile_y = 0

# Player details
px = 20
py = 20
speedy = 25
up = False
down = False
left = False
right = False
singlerun = 1

You shouldn't store all of this in global variables. You should really put them all in classses. This'll work for now but it gets more and more problematic as the complexity grows.

# Load Map
with open('townhall.map', 'r') as f:
    for line in f:
            for character in line:
                if character == "\n":
                    print "Newline"
                else:
                    if character == "x":
                        print "WALL"
                    else: 
                        if character == "a":
                            print "LAND"

I'm guessing this is temporary until you actually implementing loading the map. But there are some improvements that can be made.

Firstly, we can make use of elif which will make the code simpler:

 if character == "\n":
     print "Newline"
 elif character == "x":
     print "WALL"
 elif character == "a":
     print "LAND"

We can also use a dictionary:

MAP_TYPES = {
   '\n' = "NEWLINE",
   'x' = "WALL",
   'a' = "LAND"
}

for line in f:
    for character in line:
        print MAP_TYPES[character]

Which I think further simplifies the situation.

# Other
completed = 0
clock = pygame.time.Clock()
splashboot = 0

# Initialise screen
pygame.init()
pygame.mouse.set_visible(False)
screen = pygame.display.set_mode((640, 420))
pygame.display.set_caption('The Missing Piece ' + ver)

# Unused
#icon = pygame.image.load(img).convert_alpha()
#pygame.display.set_icon(icon)

Learn to use version control, and then delete unused code.

# Initialise sprites
try:
    cursor = pygame.image.load(cur).convert_alpha()
    logo = pygame.image.load(logo).convert_alpha()
    player = pygame.image.load(player).convert_alpha()
except:
    print "Unexpected error loading sprites!"
    raw_input("Press ENTER to exit")
    pygame.quit()
    raise

Hmm... I'm not sure you bothered catching this error. You end-up-reraising the exception anyway, so what did you gain by asking for the user to push enter?

# Splash screen
while splashboot != 25:
    for event in pygame.event.get():
        if event.type == QUIT:
            pygame.quit()
    splash = pygame.Surface(screen.get_size())
    splash = splash.convert()
    splash.fill(spl)

Typically, this would be done outside of the loop. That way you don't have to recreate it all the time. You probably don't even need this splash, just call fill on the screen.

    x, y = screen.get_size()

x and y usually imply coordinates. Using them to mean sizes is odd.

    gx = x - 600
    yx = (y / 2) - 25

Very crypitic names, pick something more meaningful.

    font = pygame.font.Font(None, 20)
    splashver = font.render(ver, 20, (0, 0, 0))

The 20 you are passing is supposed to be a true or false value. Are you trying to tell it the size again?

    screen.blit(splash, (0, 0))
    screen.blit(logo, (gx,yx))
    screen.blit(splashver, (520, yx + 50))
    pygame.display.update()

Your display doesn't change during this loop. You should draw once before you start the loop. splashboot += 1

Use a for loop, for splashboot in xrange(25): rather then the while loop. Then you don't need to increment the loop here.

    time.sleep(0.1)
    print splashboot


# Fill background
background = pygame.Surface(screen.get_size())
background = background.convert()
background.fill(bg)

I don't this helps or is neccessary. Just fill the screen.

# Define instructions
font = pygame.font.Font(None, 20)
ver = font.render("The Missing Piece " + ver, 20, (0, 0, 0))
conl1 = font.render("W = UP", 20, (0, 0, 0))
conl2 = font.render("A = LEFT", 20, (0, 0, 0))
conl3 = font.render("S = DOWN", 20, (0, 0, 0))
conl4 = font.render("D = RIGHT", 20, (0, 0, 0))


# Event loop
while completed != 1:

Why isn't completed True/Falses instead of 1/0?

    dt = clock.tick(60)
    speed = speedy / float(dt)
    for event in pygame.event.get():
        if event.type == QUIT:
            completed = 1
            pygame.quit()
        if event.type == KEYDOWN:
            if event.key == K_LEFT:
                left = True
            if event.key == K_RIGHT:
                right = True   
            if event.key == K_UP:
                up = True
            if event.key == K_DOWN:
                down = True
            if event.key == K_a:
                left = True
            if event.key == K_d:
                right = True   
            if event.key == K_w:
                up = True
            if event.key == K_s:
                down = True

Use if event.key == K_s or event.key == K_DOWN or if event.key in (K_s, K_DOWN) to avoid have cases that do the same thing.

        if event.type == KEYUP:
            if event.key == K_LEFT:
                left = False
            if event.key == K_RIGHT:
                right = False   
            if event.key == K_UP:
                up = False
            if event.key == K_DOWN:
                down = False
            if event.key == K_a:
                left = False
            if event.key == K_d:
                right = False   
            if event.key == K_w:
                up = False
            if event.key == K_s:
                down = False

You spend a lot of code worrying about flipping up/down/left/right variable. Instead you could do something like

pressed_keys = set()

if event.type == KEY_DOWN: pressed_keys.add(event.key) elif event.type == KEY_UP: pressed_keys.remove(event.key)

Then you use if KEY_s in pressed_keys: to determine whether a key is currently pressed.

    # direction scripts

    if up:
        py -= speed
    if down:
        py += speed
    if left:
        px -= speed
    if right:
        px += speed

    # cursor settings
    mousex,mousey = pygame.mouse.get_pos()
    mousex -= cursor.get_width()/2
    mousey -= cursor.get_height()/2
    x, y = screen.get_size()
    gx = x - 200

    # draw background
    screen.blit(background, (0, 0))

    #pygame.draw.rect(screen, (255,255,255), (10,10,200,200), 0) # testbox
    pygame.draw.rect(screen, (255,255,255), (gx,0,200,y), 0) # sidebar

    # draw player
    screen.blit(player, (px, py))

    # draw version
    screen.blit(ver, (gx,1))

    # draw instructions
    screen.blit(conl1, (gx+2,20))
    screen.blit(conl2, (gx+2,32))
    screen.blit(conl3, (gx+2,44))
    screen.blit(conl4, (gx+2,56))

    # draw cursor
    screen.blit(cursor, (mousex, mousey))

    pygame.display.update()

Here's my reworking of your game:

import pygame, sys, time
from pygame.locals import *
VERSION = "INDEV v1.0" # current version

BLACK = (0, 0, 0)
WHITE = (255, 255, 255)

BACKGROUND_COLOR = BLACK
SPLASH_COLOR = WHITE

def setup_graphics():
    pygame.init()
    pygame.mouse.set_visible(False)
    screen = pygame.display.set_mode((640, 420))
    pygame.display.set_caption('The Missing Piece ' + VERSION)
    return screen

def load_image(name):
    return pygame.image.load(name).convert_alpha()


def draw_text(surface, text, coordinates):
    text_surface = FONT.render(text, True, BLACK)
    surface.blit(text_surface, coordinates)

class SplashScreen:
    def __init__(self):
        self.time = 25.0 # show for 25 seconds

    def draw(self, screen):
        screen.fill(SPLASH_COLOR)
        screen.blit(LOGO, (40, 185))
        draw_text(screen, VERSION, (520, 235))

    def update(self, game, time):
        self.time -= time
        if self.time < 0:
            game.current_screen = GameScreen()

class GameScreen:
    PLAYER_SPEED = 25

    def __init__(self):
        self.player_x = 0
        self.player_y = 0

    def draw(self, screen):
        screen.fill(BACKGROUND_COLOR)

        pygame.draw.rect(screen, (255,255,255), (440,0,200,420), 0) # sidebar


        # draw player
        screen.blit(PLAYER_IMAGE, (self.player_x, self.player_y))

        # draw instructions
        draw_text(screen, "The Missing Piece " + VERSION, (441, 1))
        draw_text(screen, "W = UP", (442, 20))
        draw_text(screen, "A = LEFT", (442, 32))

        draw_text(screen, "S = DOWN", (442, 44))
        draw_text(screen, "D = RIGHT", (442, 56))

        mouse_x,mouse_y = pygame.mouse.get_pos()
        mouse_x -= CURSOR.get_width()/2
        mouse_y -= CURSOR.get_height()/2

        screen.blit(CURSOR, (mouse_x, mouse_y) )

    def update(self, game, time):
        speed = self.PLAYER_SPEED / time
        if K_s in game.keys_pressed or K_DOWN in game.keys_pressed:
            self.player_y += speed
        elif K_w in game.keys_pressed or K_UP in game.keys_pressed:
            self.player_y -= speed
        elif K_a in game.keys_pressed or K_LEFT in game.keys_pressed:
            self.player_x -= speed
        elif K_d in game.keys_pressed or K_RIGHT in game.keys_pressed:
            self.player_x += speed


class Game:
    def __init__(self, screen, current_screen):
        self.keys_pressed = set()
        self.clock = pygame.time.Clock()
        self.screen = screen
        self.current_screen = current_screen

    def run(self):
        while True:
            time = self.clock.tick(60)
            for event in pygame.event.get():
                if event.type == QUIT:
                    return
                elif event.type == KEYDOWN:
                    self.keys_pressed.add(event.key)
                elif event.type == KEYUP:
                    self.keys_pressed.remove(event.key)

            self.current_screen.update(self, time)
            self.current_screen.draw(self.screen)
            pygame.display.update()


screen = setup_graphics()
CURSOR = load_image("cur.png")
PLAYER_IMAGE = load_image("player.png")
LOGO = load_image("pixl.png")
FONT = pygame.font.Font(None, 20)

game = Game(screen, SplashScreen())
game.run()

If you don't know classes and functions, you are really going to want to learn them. It'll be difficult doing anything beyond a very simple project without them.

\$\endgroup\$
  • \$\begingroup\$ Thankyou so much for spending your time writing that, I understand my problems and will fix them. I will add your name somewhere in the games's credits :) TY \$\endgroup\$ – user11064 Feb 15 '12 at 19:20
  • \$\begingroup\$ +1. Yet there are a lot of cryptic numbers in the code like 442 vs. constants like SIDEBAR_INDENTED_TEXT_X. --- Do not recalculate fixed background details in the loop. Draw them once onto a pygame surface. In the loop, blit the surface to the screen before the player and cursor. More efficient and clearer on what changes and what is fixed. --- The speed variable seems wrong. It's a step or distance, not a speed. PLAYER_SPEED is a speed, but to get constant player speed, multiply PLAYER_SPEED by the elapsed time (vs. dividing) to get the distance (change in x and/or y). \$\endgroup\$ – Paul Martel Feb 15 '12 at 22:21
  • \$\begingroup\$ @PaulMartel, all true. I did notice most of those problems, but compared to the bigger issues I though it would better to let them slide. \$\endgroup\$ – Winston Ewert Feb 15 '12 at 22:46
  • \$\begingroup\$ The player cannot go diagonal, how would I make it work? Nevermind, I have fixed it by changing the elif's to ifs \$\endgroup\$ – Jack Wilsdon Feb 16 '12 at 11:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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