SimCity clone with PyGame

Question

I am doing my best to keep this code clean and fast, but as I add more to the game, it seems to get harder to maintain a frame-rate higher than 200! I believe that the culprit is a somewhat large for loop inside of the main loop. It performs all of the actions for the tiles inside of the world, and as more tiles are added, the game gets slower.

import pygame
from pygame.locals import *
from random import choice
from math import sqrt

#inits
pygame.init()
font=pygame.font.Font(None, 18)
screen=pygame.display.set_mode((640,480))
pygame.display.set_caption('City Game | Pre-Alpha')
clock=pygame.time.Clock()

#sprites
curspr=pygame.image.load('curs.png').convert()
curspr.set_alpha(100)
grassspr=pygame.image.load('grass.png').convert()
roadspr=pygame.image.load('road.png').convert()
forestspr=pygame.image.load('forest.png').convert()
water1=pygame.image.load('water1.png').convert()
water2=pygame.image.load('water2.png').convert()
power1=pygame.image.load('power1.png').convert()
power2=pygame.image.load('power2.png').convert()
res=pygame.image.load('res.png').convert()
house1_0=pygame.image.load('house1_0.png').convert()
house1_1=pygame.image.load('house1_1.png').convert()
res.set_alpha(215)

#vars and lists
taxrateR=8
tilelist=[grassspr,roadspr,forestspr,water1,res,power1]
namelist=['Grass','Road','Forest','Water','Residental','Power Plant']
costlist=[5,10,20,50,100,250]
tiles=[]
sel=0
money=10000
mse=(0,0)
tileframe=2000
pop=0
month=1
year=1
monthtime=0
R=1
C=1
I=1

def Dist(set1,set2):
    vec=(set2[0]-set1[0],set2[1]-set1[1])
    dist=(vec[0]**2+vec[1]**2)
    return sqrt(dist)

class Tile(object):
    def __init__(self,pos,spr,typ):
        self.typ=typ
        self.spr=spr
        self.pos=pos
        self.rect=pygame.rect.Rect(pos[0],pos[1],32,32)
        self.adv=0
        self.haspower=0

while True:
    pygame.display.set_caption(str(clock.get_fps()))
    screen.fill((2,110,200))
    key=pygame.key.get_pressed()
    othertiles=[x for x in tiles if x.spr==power1 or x.spr==power2]
    for e in pygame.event.get():
        if e.type==QUIT:
            exit()

        if e.type==KEYUP:
            if key[K_s]:
                if e.key==K_e:
                    sel+=1
                    if sel==len(tilelist):
                        sel=0
                if e.key==K_q:
                    sel-=1
                    if sel==-1:
                        sel=len(tilelist)-1

        if e.type==MOUSEMOTION:
            mse=pygame.mouse.get_pos()
            if key[K_LSHIFT] or key[K_RSHIFT]:
                if pygame.mouse.get_pressed()==(1,0,0):
                    tilesatmouse=[t for t in tiles if t.rect.collidepoint(mse)]
                    if not tilesatmouse:
                        if sel==4:
                            money-=costlist[sel]
                            tiles.append(Tile((mse[0] & 0x7fffffe0,mse[1] & 0x7fffffe0),tilelist[sel],'res'))
                        else:
                            money-=costlist[sel]
                            tiles.append(Tile((mse[0] & 0x7fffffe0,mse[1] & 0x7fffffe0),tilelist[sel],'tile'))
                elif pygame.mouse.get_pressed()==(0,0,1):
                    for t in tiles:
                        if t.rect.collidepoint(mse):
                            money-=5
                            tiles.remove(t)       

        if e.type==MOUSEBUTTONUP:
            if e.button==1:
                tilesatmouse=[t for t in tiles if t.rect.collidepoint(mse)]
                if not tilesatmouse:
                    if sel==4:
                        money-=costlist[sel]
                        tiles.append(Tile((mse[0] & 0x7fffffe0,mse[1] & 0x7fffffe0),tilelist[sel],'res'))
                    else:
                        money-=costlist[sel]
                        tiles.append(Tile((mse[0] & 0x7fffffe0,mse[1] & 0x7fffffe0),tilelist[sel],'tile'))

            if e.button==3:
                for t in tiles:
                    if t.rect.collidepoint(mse):
                        money-=5
                        tiles.remove(t)

    for t in tiles:
        if t.spr==water1 or t.spr==water2:
            if tileframe>999:
                t.spr=water1
            else:
                t.spr=water2
        if t.spr==power1 or t.spr==power2:
            if tileframe>999:
                t.spr=power1
            else:
                t.spr=power2

        if t.typ=='res':
            for x in othertiles:
                distance=Dist((t.pos[0],t.pos[1]),(x.pos[0],x.pos[1]))
                if distance<2500:
                    t.haspower=1
                else:
                    t.haspower=0
                if othertiles==[]:
                    t.haspower=0
            if t.haspower==0:
                t.spr.set_alpha(100)
            else:
                t.spr.set_alpha(255)
            if t.adv>=0:
                t.adv+=R
                if t.adv==2000:
                    t.spr=house1_0
                if t.adv==4000:
                    t.spr=house1_1
                    pop+=choice([4,6,8,10])
                    t.adv=-1
        screen.blit(t.spr,t.pos)

    if monthtime<10000:
        monthtime+=1
    else:
        month+=1
        money+=20
        monthtime=0
        if month==13:
            month=0
            money+=pop/2*taxrateR
            year+=1
    screen.blit(curspr, (mse[0] & 0x7fffffe0,mse[1] & 0x7fffffe0))
    if key[K_s]:
        pygame.draw.rect(screen, (0,0,0), (0,0,640,80), 0)
        moneydraw=font.render('Funds: '+str(money), 1, (255,255,255))
        yeardraw=font.render('Year: '+str(year), 1, (255,255,255))
        monthdraw=font.render('Month: '+str(month), 1, (255,255,255))
        namedraw=font.render(namelist[sel],1,(255,255,255))
        screen.blit(moneydraw, (2,2))
        screen.blit(namedraw, (2,18))
        screen.blit(yeardraw, (2,34))
        screen.blit(monthdraw, (2,50))
    clock.tick(999)
    tileframe-=1
    if tileframe==0:tileframe=2000
    pygame.display.flip()

Answer 1

Humans are bad at figuring out where the slow spots are. You will need to ask the computer by profiling your code. As rolfl said, you're likely better off saving this for when you have more of your code in place and it does enough work that it's actually slowing down.

It's quite possible most of your slowdown related to extra tiles comes from othertiles=[x for x in tiles if x.spr==power1 or x.spr==power2]; perhaps you should update othertiles incrementally as the board changes instead of recalculating it every pygame.event. Other than that, I'm going to concentrate on the inner for t in tiles loop. There are a couple things in there that stand out to me. They may or may not make a real difference to your code's performance.

• The overall body reads like an if tree, yet all scenarios are mutually exclusive. You can probably squeak a little better performance out by changing the second and later ifs to elifs. Make sure they're ordered in terms of descending frequency. If the number of possibilities will grow larger, consider other approaches that don't require a series of if tests. (At some point the overhead of testing each alternative will likely be more expensive than that of using alternate lookups such as draw method on various tile classes.)

• The distance check loop in a res tile seems flawed. It executes a for loop, but only saves the result from the last element of othertiles. And for each element, it checks if othertiles is empty (aside: you should spell that if othertiles: rather than creating and comparing against an empty list every time). If you want the first result, you can fix both of these by using break and a for/else loop:

  for x in othertiles:
      # distance calculation
      break
  else:
      t.haspower = 0
  

  (Alternately you could just set t.haspower = 0—or False—before the for loop, and let the for loop set it to 1—or True—if applicable.)

• The distance calculation is computationally heavy. Like my comment on your previous post, this is typically more relevant to C level code, but it's still good to remember that sqrt takes a while to calculate, so you can look for opportunities to avoid calling it. Consider that you compare Dist(...) to a constant 2500. If instead of calling sqrt you just returned the square of the distance, you could compare to 6250000 and have the same effect.
• Calling the distance function, you tear t.pos and x.pos apart and piece it back together for a function that just uses their first two elements. You might save some effort, readability, and possibly gain a hint of performance by calling your distance function with just Dist(t.pos, x.pos).

Two more notes:

• You probably don't want just the first or last distance to a power tile. Instead you likely want the closest. This might be easier to handle with an approach like distance = min(Dist(t.pos, x.pos) for x in othertiles)) if othertiles else 2501 or even better:

  t.haspower = False
  if othertiles:
      t.haspower = min(Dist(t.pos, x.pos) for x in othertiles)) < 2500
      # or distance_squared and compare to 6250000 per third bullet
  

• It's unusual in python to name your functions with initial capital letters. When I read the line distance=Dist((t.pos[0],t.pos[1]),(x.pos[0],x.pos[1])) I assumed it was instantiating a class (which would probably be even heavier than calculating a square root). I would suggest renaming this use all lowercase letters.

Answer 2

I believe comparing images isn't of the greatest efficiency, replace the spr with a enum type value, then you don't need to swap over the image, or at least not compare them.

also you keep changing the alpha on the images, duplicate the image and keep both versions, one with alpha 100 and one with alpha 255.

I also found a bug in the for x in othertiles: loop body; the if othertiles == [] is inside it so will never be reached; just initialize it before the loop. And only the last othertile actually matters in the loop as you overwrite haspower each time. you only need to find if there is any tile in otherTiles where Dist to it if smaller than 2500, I'm pretty sure there is a list comprehension for that, but I'm no python wiz.

Answer 3

I'd see what happens if you refactor drawing code out of the cel update loop. Let each sprite image maintain a list of bools corresponding to all of the available tile position (as the other commenters have noticed, make separate images for the different alpha versions of the same sprite instead of tweaking it on the fly.)

As each tile updates, see if it's state has changed enough to warrant a sprite change. If it has, toggle the corresponding entry for the old sprite list off and the same entry in the new sprite list on. In general you won't be changing the lists that much (and by keeping them as fixed size lists instead of adding and removing them you won't have to worry about moving a lot of memory around).

Once all the sprite image lists are set, just loop through each and draw a tile for every true value (you'll need a formula to get the 2-d position from the 1-d index of the bool, but that is basically just (tile_height * floor(index / columns), tile_width * int(index % columns))

Hopefully this means (a) you do a lot less moving of memory around - the biggest category of changes is bools toggling on and off in fixed sized arrays - and (b) you can draw the same sprite many times, which should be more cache friendly for the graphics hardware, instead of constantly cycling different art in and out of memory.

in this context you might want to google the phrase 'Data Oriented Design' or 'Mike Acton'