6
\$\begingroup\$

I am a PyGame newbie and am learning as I go. I would like to become familiar with normal PyGame conventions. I would like you PyGame experts go over my code and let me know if there are any places I should modify my code to follow best practice.

The point of the game is to collect as many pills as possible. Yellows are worth 10, reds 20, blue 30 and black 40. First one to reach 15,000 wins. The ships are controlled using WASD and .

Some areas of concern I am looking at are the following:

Where I created two separate classes to store the score information. I feel that I could do the same job with one class, but am a bit confused on how that would look since I am using a TextGroup and it needs to be passed both Ship objects on the call to TextGroup.update().

class Text(Entity):
    def __init__(self, text, size, color, position, font=None):
        Entity.__init__(self)
        self.color = color
        self.position = position
        self.font = pygame.font.Font(font, size)
        self.text = text
        self.image = self.font.render(str(text), 1, self.color)
        self.rect = self.image.get_rect()
        self.rect.move_ip(position[0]-self.rect.width/2, position[1])


class Mass_Left(Text):
    def __init__(self, text, size, color, position, font=None):
        Text.__init__(self, text, size, color, position, font=None)

    def update(self, ship_left, ship_right):
        self.text = "mass: " + str(ship_left.density-169)
        self.image = self.font.render(str(self.text), 1, self.color)
        self.rect = self.image.get_rect()
        self.rect.move_ip(self.position[0]-self.rect.width/2, self.position[1])


class Mass_Right(Text):
    def __init__(self, text, size, color, position, font=None):
        Text.__init__(self, text, size, color, position, font=None)

    def update(self, ship_left, ship_right):
        self.text = "mass: " + str(ship_right.density-169)
        self.image = self.font.render(str(self.text), 1, self.color)
        self.rect = self.image.get_rect()
        self.rect.move_ip(self.position[0]-self.rect.width/2, self.position[1])

Also, here in the method moveShip() where I am checking if self.player is left or right. I feel that there should be a way to do this by passing the class a function when the Ship object is created that will take appropriate action and different action depending on whether it's the right or left ship.

def moveShip(self):
        key = pygame.key.get_pressed()

        if self.player == 'left' and (key[pygame.K_w] or key[pygame.K_s] or key[pygame.K_a] or key[pygame.K_d]):
            if key[pygame.K_w]:
                self.rect.y -= self.speed
            if key[pygame.K_s]:
                self.rect.y += self.speed
            if key[pygame.K_a]:
                self.rect.x -= self.speed
            if key[pygame.K_d]:
                self.rect.x += self.speed

        # Adjust Player 2 Speed
        if self.player == 'right' and (key[pygame.K_UP] or key[pygame.K_DOWN] or key[pygame.K_LEFT] or key[pygame.K_RIGHT]):
            if key[pygame.K_UP]:
                self.rect.y -= self.speed
            if key[pygame.K_DOWN]:
                self.rect.y += self.speed
            if key[pygame.K_LEFT]:
                self.rect.x -= self.speed
            if key[pygame.K_RIGHT]:
                self.rect.x += self.speed

Same issue here with the methods moveInbounds() and winGame().

The function genRandom generates a tuple that contains a random x value for the Pills and a random density value between 1-4. I am using string concatenation, then doing a type conversion, but I'm sure there's a more straightforward way to generate a random tuple.

def genRandom(size):
    xval_density = []

    for j in range(size):
        length = str(random.randrange(0, (WIN_W/2) - PILL_WIDTH))
        stup = '('
        stup = stup + str(length)
        stup = stup +  ", "
        stup = stup + random.choice('1111111111111111111122222334')
        stup = stup + ')'
        tup = literal_eval(stup)
        xval_density.append(tup)

    return xval_density

I’m also uncomfortable using so many global variables such as PILL_COUNT and TIMER. So if there is a best practice in those situations, I’d be happy to know about it.

Here's the complete code:

import sys, pygame, os, random, math, time

from ast import literal_eval

# Force static position of screen
os.environ['SDL_VIDEO_CENTERED'] = '1'

# Runs imported module
pygame.init()

# Constants
LEFT = 'left'
RIGHT = 'right'

YELLOW = (255, 255, 0)
RED = (255,0,0)
BLUE = (0,0,153)
BLACK = (0, 0, 0)
WHITE = (255, 255, 255)

SHIP_WIDTH = 13
SHIP_HEIGHT = 13

PILL_WIDTH = 7
PILL_HEIGHT = 25
PILL_MAX_SIZE = 3000
PILL_COUNT = 0
TIMER = 0

WIN_W = 1200
WIN_H = 670


class Entity(pygame.sprite.Sprite):
    def __init__(self):
        pygame.sprite.Sprite.__init__(self)


class Text(Entity):
    def __init__(self, text, size, color, position, font=None):
        Entity.__init__(self)
        self.color = color
        self.position = position
        self.font = pygame.font.Font(font, size)
        self.text = text
        self.image = self.font.render(str(text), 1, self.color)
        self.rect = self.image.get_rect()
        self.rect.move_ip(position[0]-self.rect.width/2, position[1])


class Mass_Left(Text):
    def __init__(self, text, size, color, position, font=None):
        Text.__init__(self, text, size, color, position, font=None)

    def update(self, ship_left, ship_right):
        self.text = "mass: " + str(ship_left.density-169)
        self.image = self.font.render(str(self.text), 1, self.color)
        self.rect = self.image.get_rect()
        self.rect.move_ip(self.position[0]-self.rect.width/2, self.position[1])


class Mass_Right(Text):
    def __init__(self, text, size, color, position, font=None):
        Text.__init__(self, text, size, color, position, font=None)

    def update(self, ship_left, ship_right):
        self.text = "mass: " + str(ship_right.density-169)
        self.image = self.font.render(str(self.text), 1, self.color)
        self.rect = self.image.get_rect()
        self.rect.move_ip(self.position[0]-self.rect.width/2, self.position[1])


class Ship(Entity):
    def __init__(self, x, y, player):
        Entity.__init__(self)
        self.win = False
        self.speed = 5
        self.player = player
        self.density = SHIP_WIDTH * SHIP_HEIGHT
        self.old_density = 144
        self.densityIncrease = False
        self.image = pygame.Surface((SHIP_WIDTH, SHIP_HEIGHT)).convert()
        self.rect = pygame.Rect(x, y, SHIP_WIDTH, SHIP_HEIGHT)

    def moveShip(self):
        key = pygame.key.get_pressed()

        if self.player == 'left' and (key[pygame.K_w] or key[pygame.K_s] or key[pygame.K_a] or key[pygame.K_d]):
            if key[pygame.K_w]:
                self.rect.y -= self.speed
            if key[pygame.K_s]:
                self.rect.y += self.speed
            if key[pygame.K_a]:
                self.rect.x -= self.speed
            if key[pygame.K_d]:
                self.rect.x += self.speed

        # Adjust Player 2 Speed
        if self.player == 'right' and (key[pygame.K_UP] or key[pygame.K_DOWN] or key[pygame.K_LEFT] or key[pygame.K_RIGHT]):
            if key[pygame.K_UP]:
                self.rect.y -= self.speed
            if key[pygame.K_DOWN]:
                self.rect.y += self.speed
            if key[pygame.K_LEFT]:
                self.rect.x -= self.speed
            if key[pygame.K_RIGHT]:
                self.rect.x += self.speed

    def moveInbounds(self):
        # Keep Ship Movement Inbounds
        if self.rect.y < WIN_H/15:
            self.rect.y = WIN_H/15
        if self.rect.y > WIN_H - self.rect.height:
            self.rect.y = WIN_H - self.rect.height

        if self.player == 'left':
            if self.rect.x < 0:
                self.rect.x = 0
            if self.rect.x > WIN_W/2 - self.rect.width:
                self.rect.x = WIN_W/2 - self.rect.width
        elif self.player == 'right':
            if self.rect.x < WIN_W/2:
                self.rect.x = WIN_W/2
            if self.rect.x > WIN_W - self.rect.width:
                self.rect.x = WIN_W - self.rect.width

    def checkCollisions(self, pillGroup):
        collisions = pygame.sprite.spritecollide(self, pillGroup, True)
        for key in collisions:
            self.density += key.density

    def grow(self):
        if self.old_density < self.density:
            self.old_density = self.density
            self.rect.width = self.rect.height = math.sqrt(self.density)
            self.image = pygame.transform.scale(self.image, (self.rect.width, self.rect.height))

    def update(self, pillGroup):
        # Ship Movement
        self.moveShip()
        self.moveInbounds()
        self.checkCollisions(pillGroup)
        self.grow()

    def winGame(self):
        if self.win:
            if TIMER % 5 == 0:
                self.rect.width += 20
                self.rect.height += 10
                if self.player == 'left':
                    self.rect.x -= 4
                elif self.player == 'right':
                    self.rect.x -= 10
                if self.player == 'left':
                    self.rect.y -= 5
                elif self.player == 'right':
                    self.rect.y -= 5
                self.image = pygame.transform.scale(self.image, (self.rect.width, self.rect.height))
                self.density += 378
        else:
            if TIMER % 5 == 0:
                if self.rect.width == 0:
                    pass
                elif self.rect.width > 10:
                    self.rect.width -= 5
                    self.rect.height -= 5
                    if self.density >= 0:
                        self.density -= self.density/3
                    self.image = pygame.transform.scale(self.image, (self.rect.width, self.rect.height))
                elif self.rect.width <= 10:
                    self.rect.width -= 1
                    self.rect.height -= 1
                    if self.density > 0:
                        self.density -= 2
                    self.image = pygame.transform.scale(self.image, (self.rect.width, self.rect.height))

                if self.density - 169 < 0:
                    self.density = 169

    def check_done(self):
        if self.rect.height > WIN_H*1.5 and self.rect.width > WIN_W * 1.5:
            return False
        else:
            return True


class Pill(Entity):
    def __init__(self, xval, density):
        Entity.__init__(self)
        self.speed = 3
        self.density = density
        self.image = pygame.Surface((PILL_WIDTH, PILL_HEIGHT)).convert()
        self.image.fill(self.setColor(density))
        self.rect = self.image.get_rect()
        self.rect = self.rect.move(xval, WIN_H/15)

    def setColor(self, density):
        if density == 50:
            return YELLOW
        elif density == 100:
            return RED
        elif density == 150:
            return BLUE
        elif density == 200:
            return BLACK

    def update(self):
        if self.rect.y > WIN_H:
            self.kill()
        else:
            self.rect = self.rect.move((0, self.speed))


def addPill(pillGroup, xvalue, density):
    global PILL_COUNT, PILL_MAX_SIZE, TIMER

    if PILL_COUNT + 1 < PILL_MAX_SIZE and TIMER % 10 == 0:
        pill = Pill(100, density)
        pill2 = Pill(100 + WIN_W/2, density)
        pillGroup.add(pill, pill2)
        PILL_COUNT += 1


def genRandom(size):
    xval_density = []

    for j in range(size):
        length = str(random.randrange(0, (WIN_W/2) - PILL_WIDTH))
        stup = '('
        stup = stup + str(length)
        stup = stup +  ", "
        stup = stup + random.choice('1111111111111111111122222334')
        stup = stup + ')'
        tup = literal_eval(stup)
        xval_density.append(tup)

    return xval_density


def loseGame(left, right):
    if left.density > 1500 or right.density > 1500:
        if left.density > 1500:
            left.win = True
        elif right.density > 1500:
            right.win = True
        return False
    else:
        return True


def main():
    # Initialize variables
    global TIMER, PILL_COUNT

    fps = 60
    pygame.display.set_caption('Pong')
    screen = pygame.display.set_mode((WIN_W, WIN_H), pygame.SRCALPHA)
    clock = pygame.time.Clock()
    play = game_done = True
    xval_density = genRandom(PILL_MAX_SIZE)

    # Create Game Objects
    ship_left = Ship((WIN_W/4) - (SHIP_WIDTH/2), WIN_H - (SHIP_HEIGHT * 4), 'left')
    ship_right = Ship((WIN_W/1.3) - (SHIP_WIDTH/2), WIN_H - (SHIP_HEIGHT * 4), 'right')
    score1 = Mass_Left("mass: " + str(ship_left.density-1), 40, BLACK, (WIN_W/5, 10))
    score2 = Mass_Right("mass: " + str(ship_right.density-1), 40, BLACK, (WIN_W/1.25, 10))
    vert_partition = pygame.Surface((1, WIN_H))
    hori_partition = pygame.Surface((WIN_W, 1))

    # Create Groups
    shipGroup = pygame.sprite.Group()
    shipGroup.add(ship_left, ship_right)
    pillGroup = pygame.sprite.Group()
    textGroup = pygame.sprite.Group()
    textGroup.add(score1, score2)

    # Gameplay
    while play:
        # Checks if window exit button pressed
        for event in pygame.event.get():
            if event.type == pygame.QUIT: sys.exit()

            # Keypresses
            elif event.type == pygame.KEYDOWN:
                if event.key == pygame.K_ESCAPE:
                    pygame.quit()
                    sys.exit()

        # Update Groups
        shipGroup.update(pillGroup)
        pillGroup.update()
        textGroup.update(ship_left, ship_right)

        # Adding Pills
        addPill(pillGroup, xval_density[PILL_COUNT][0], xval_density[PILL_COUNT][1]*50)

        # Print Groups
        screen.fill(WHITE)
        pillGroup.draw(screen)
        shipGroup.draw(screen)
        textGroup.draw(screen)
        screen.blit(vert_partition, (WIN_W/2, WIN_H/15))
        screen.blit(hori_partition, (0, WIN_H/15))

        play = loseGame(ship_left, ship_right)

        TIMER += 1
        # Limits frames per iteration of while loop
        clock.tick(fps)
        # Writes to main surface
        pygame.display.flip()

    # Gameplay
    while game_done:
        ship_left.winGame()
        ship_right.winGame()

        # Updating
        pillGroup.update()
        textGroup.update(ship_left, ship_right)

        # Adding Pills
        addPill(pillGroup, xval_density[PILL_COUNT][0], xval_density[PILL_COUNT][1]*50)

        # Print Groups
        screen.fill(WHITE)
        pillGroup.draw(screen)
        shipGroup.draw(screen)
        textGroup.draw(screen)
        screen.blit(vert_partition, (WIN_W/2, WIN_H/15))
        screen.blit(hori_partition, (0, WIN_H/15))

        game_done = ship_left.check_done() and ship_right.check_done()

        TIMER += 1
        # Limits frames per iteration of while loop
        clock.tick(fps)
        # Writes to main surface
        pygame.display.flip()


if __name__ == "__main__":
    main()
\$\endgroup\$
2
\$\begingroup\$

Use OOP more appropriately

  • Your Entity class adds absolutely nothing to pygame.sprite.Sprite so you’d be better of using the latter as base class for your Text, Ship and Pill ones.
  • It is usually advised to use super to call the parent class constructor instead of explicitly naming the class. However, pygame documentation uses explicit pygame.sprite.Sprite.__init__(self) call so I’ll stick with it.
  • You’re incorrectly calling the Text.__init__ method in Mass_Left and Mass_Right: you force the font parameter to be None instead of using the value provided in __init__’s parameters. You should call super(Mass_Right, self).__init__(text, size, color, position, font=font). Or even remove the font= part. Renaming the parameter might help to understand: you’re doing

    def __init__(self, text, size, color, position, f=None):
        Text.__init__(self, text, size, color, position, font=None)
    

    instead of

    def __init__(self, text, size, color, position, f=None):
        Text.__init__(self, text, size, color, position, font=f)
    
  • Your update method is pretty much the same between Mass_Left and Mass_Right. In fact it is the same if you are able to discriminate between ship_left and ship_right. Two possibilities: either you store a boolean value in the constructor telling which ship to use

    class Text(pygame.sprite.Sprite):
        def __init__(self, text, size, color, position, left_side, font=None):
            pygame.sprite.Sprite.__init__(self)
            ...
            self.left_side = left_side
    
        def update(self, ship_left, ship_right):
            ship = ship_left if self.left_side else ship_right
            text = "mass: " + str(ship.density-169)
            self.image = self.font.render(str(self.text), 1, self.color)
            self.rect = self.image.get_rect()
            self.rect.move_ip(self.position[0]-self.rect.width/2, self.position[1])
    

    or you make better use of inheritance

    class Text(pygame.sprite.Sprite):
        def __init__(self, text, size, color, position, font=None):
            pygame.sprite.Sprite.__init__(self)
            ...
    
        def _update(self, ship):
            text = "mass: " + str(ship.density-169)
            self.image = self.font.render(str(self.text), 1, self.color)
            self.rect = self.image.get_rect()
            self.rect.move_ip(self.position[0]-self.rect.width/2, self.position[1])
    
    
    class Mass_Left(Text):
        def __init__(self, text, size, color, position, font=None):
            super(Mass_Left, self).__init__(text, size, color, position, font)
    
        def update(self, ship_left, ship_right):
            self._update(ship_left)
    
    class Mass_Right(Text):
        def __init__(self, text, size, color, position, font=None):
            super(Mass_Right, self).__init__(text, size, color, position, font)
    
        def update(self, ship_left, ship_right):
            self._update(ship_right)
    

Use only what you need

  • Since your Texts subclasses directly convert the text into a surface, you don't need to store it. Better, you even don't need to pass it to the constructor since it is computed at each update.
  • In genRandom you can pass values to the tuple constructor instead of building a string and parsing it as a Python data-structure:

    tup = (random.randrange(0, (WIN_W/2) - PILL_WIDTH), int(random.choice('1111111111111111111122222334')))
    

    is the same as your

    tup = literal_eval(stup)
    

    without the need to build a string first. In fact, since the function is now only

    def genRandom(size):
         xval_density = []
         for _ in range(size):
             xval_density.append((
                 random.randrange(0, (WIN_W/2) - PILL_WIDTH),
                 int(random.choice('1111111111111111111122222334'))))
         return xval_density
    

    you could simplify further using a list-comprehension:

    def genRandom(size):
        return [
            (random.randrange(0, (WIN_W/2) - PILL_WIDTH),
            int(random.choice('1111111111111111111122222334')))
            for _ in range(size)]
    
  • In loseGame you check for a bunch of conditions and then, detail which condition it actualy is. So you’re doing twice the work. Remove the outer if:

    def loseGame(left, right):
        left.win = left.density > 1500
        right.win = right.density > 1500
    
        return not (left.win or right.win)
    

    This one is also weird because its name and what it does are kind of opposite. I'd rename it is_game_ended, remove the not in the return statement and call it usinig while not game_ended: ...; game_ended = is_game_ended(ship_left, ship_right); ....

  • In moveShip you check for a bunch of conditions and then, detail which condition it actualy is. So you’re doing twice the work.

    if self.player == 'right':
        if key[pygame.K_UP]:
            self.rect.y -= self.speed
        ...
    

Improve reusability

Your whole code has been built upon the assumption that there will be two players. But what if you want to add support for a third on a forth? Or add a single player mode agains an AI? Will you continue to add checks for various players and duplicate the logic over and over? Try to make you code more generic instead:

  • The problem you tried to solve using your Texts subclasses can be avoided by assigning an instance of Text to each Ship. That way, it is only one ship that can update its related text:

    class Ship(...):
        ...
        def update(self, pillGroup):
            # Ship Movement
            self.moveShip()
            self.moveInbounds()
            self.checkCollisions(pillGroup)
            self.grow()
            self.text.update(self.density)
    
    class Text(...):
        ...
        def update(self, density):
            text = "mass: {}".format(density-169)
            self.image = self.font.render(str(self.text), 1, self.color)
            self.rect = self.image.get_rect()
            self.rect.move_ip(self.position[0]-self.rect.width/2, self.position[1])
    
  • You can remove the need to check for the player type in moveShip by passing a list of keys to the constructor of Ship instead of the player type:

    class Ship(pygame.sprite.Sprite):
        def __init__(self, x, y, text, controls):
            pygame.sprite.Sprite.__init__(self)
            self.win = False
            self.speed = 5
            self.text = text
            self.controls = controls
            self.density = SHIP_WIDTH * SHIP_HEIGHT
            self.old_density = 144
            self.densityIncrease = False
            self.image = pygame.Surface((SHIP_WIDTH, SHIP_HEIGHT)).convert()
            self.rect = pygame.Rect(x, y, SHIP_WIDTH, SHIP_HEIGHT)
        ...
        def moveShip(self):
            key = pygame.key.get_pressed()
            up, down, left, right = self.controls
    
            if key[up]:
                self.rect.y -= self.speed
            if key[down]:
                self.rect.y += self.speed
            if key[left]:
                self.rect.x -= self.speed
            if key[right]:
                self.rect.x += self.speed
    

    You'll have to build your ships using ship_left = Ship((WIN_W/4) - (SHIP_WIDTH/2), WIN_H - (SHIP_HEIGHT * 4), score1, [pygame.K_w, pygame.K_s, pygame.K_a, pygame.K_d])

  • The end of the game could be checked for a variable amount of ships:

    def is_game_ended(*ships):
        ended = False
        for ship in ships:
            ship.win = value = ship.density > 1500
            ended = ended or value
        return ended
    

Various

  • min and max can help make your code more readable

    if self.rect.y < WIN_H/15:
        self.rect.y = WIN_H/15
    if self.rect.y > WIN_H - self.rect.height:
        self.rect.y = WIN_H - self.rect.height
    

    becomes self.rect.y = min(max(self.rect.y, WIN_H/15), WIN_H - sel.rect.height). Similar constructs are found elsewhere in the code.

  • names of your functions should be snake_case instead of camelCase
  • Global variable and constants of your program are messed up. fps should be a constant at the top of the file. PILL_COUNT should be computed from len(xval_density), PILL_MAX_SIZE is a constant passed as a parameter of genRandom and should not be under global in addPill since it is not modified, and TIMER well... global is not recommended but I have yet to think of a better alternative; maybe that it needs its own redesign as a whole.
  • Each module import should be on its own line.
  • You may want to put os.environ['SDL_VIDEO_CENTERED'] = '1' and pygame.init() under if __name__ == "__main__". (or in main.)
  • I am not able to play a player1 on my dvorak keyboard, maybe you could add a way to specify a set of keys to overwrite the default ones.
\$\endgroup\$

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.