Passing two player objects to a class

Question

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

Answer 1

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.