11
\$\begingroup\$

I am working on a SimCity clone, and I am noticing a drop in frame rate as I add more objects to my map, now this is expected, but, when I fill the whole screen, the game maintains about 300 FPS.

Will it hold this as I add a lot more objects, or will it continue to drop?

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

#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()
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
tilelist=[grassspr,roadspr,forestspr,water1,res]
namelist=['Grass','Road','Forest','Water','Residental']
tiles=[]
sel=0
money=10000
moneydraw=font.render('Funds: '+str(money), 1, (255,255,255))
namedraw=font.render(namelist[sel],1,(255,255,255))
mse=(0,0)
waterframe=2000
pop=0

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

while True:
    pygame.display.set_caption(str(clock.get_fps()))
    namedraw=font.render(namelist[sel],1,(255,255,255))
    screen.fill((2,110,200))
    for e in pygame.event.get():
        if e.type==QUIT:
            exit()
        if e.type==MOUSEMOTION:
            mse=pygame.mouse.get_pos()
            key=pygame.key.get_pressed()
            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:
                            tiles.append(Tile(((mse[0]/32)*32,(mse[1]/32)*32),tilelist[sel],'res'))
                        else:
                            tiles.append(Tile(((mse[0]/32)*32,(mse[1]/32)*32),tilelist[sel],'tile'))
                elif pygame.mouse.get_pressed()==(0,0,1):
                    for t in tiles:
                        if t.rect.collidepoint(mse):
                            tiles.remove(t)       
        if e.type==KEYUP:
            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==MOUSEBUTTONUP:
            if e.button==1:
                tilesatmouse=[t for t in tiles if t.rect.collidepoint(mse)]
                if not tilesatmouse:
                    if sel==4:
                        tiles.append(Tile(((mse[0]/32)*32,(mse[1]/32)*32),tilelist[sel],'res'))
                    else:
                        tiles.append(Tile(((mse[0]/32)*32,(mse[1]/32)*32),tilelist[sel],'tile'))

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

    for t in tiles:
        if t.spr==water1 or t.spr==water2:
            if waterframe>999:
                t.spr=water1
            else:
                t.spr=water2
        if t.typ=='res':
            if t.adv>=0:
                t.adv+=1
                if t.adv==2000:
                    t.spr=house1_0
                if t.adv==4000:
                    t.spr=house1_1
                    pop+=choice([1,2,3,4])
                    t.adv=-1
        screen.blit(t.spr,t.pos)
    waterframe-=1
    if waterframe==0:
        waterframe=2000
    clock.tick(999)
    screen.blit(curspr, ((mse[0]/32)*32,(mse[1]/32)*32))
    screen.blit(moneydraw, (2,2))
    screen.blit(namedraw, (2,18))
    pygame.display.flip()
#if any(t.rect.colliderect(x.rect) for x in tiles if x is not t):
\$\endgroup\$
  • 4
    \$\begingroup\$ You ask: Will it hold this as I add a lot more objects? Or will it continue to drop? ... well, have you tried that and observed what happens? \$\endgroup\$ – rolfl Nov 22 '13 at 18:11
  • 4
    \$\begingroup\$ Watch out for complicated expressions inside loops. Whereas in C it's useful to decrease strength of operation, in Python it's often worth decreasing the number of operations. As it happens, none of the instances of this are in tight loops, but you could reduce both by turning ((mse[x]/32)*32) into (mse[x] & 0x7fffffe0). \$\endgroup\$ – Michael Urman Nov 23 '13 at 4:20
  • \$\begingroup\$ @rolfl, I haven't tried, there is only so much room on the screen. But when I get around to adding scrolling, I will test this. @Michael Urman, What is &0xfffffe0?? \$\endgroup\$ – Sam Tubb Nov 23 '13 at 23:39
  • 1
    \$\begingroup\$ Clear as mud now? \$\endgroup\$ – rolfl Nov 24 '13 at 0:40
  • 2
    \$\begingroup\$ 32 is the 5th power of 2. Any multiplication or division by a power of 2 can be expressed as a bit-shift. In decimal, the same is true for 10. E.g. 12345 / 1000 is 12345 shifted right 3 times (1000 is the 3rd power of 10), which (in integer terms truncates the decimal) is 12. So, (12345/1000)*1000 is 12000. Which is the same as just setting the units/tens/hundreds digits to 0. Michael, in binary instead of decimal, has just pointed out that a binary number divided then multiplied by 32 is the same as setting the low-5 bits to 0, and that is what the & 0xffffffe0 does \$\endgroup\$ – rolfl Nov 24 '13 at 4:59
11
\$\begingroup\$

As you asked in your question, I'm going to try and address your performance issues here. Some of this will be a style review though, so bear with me.


Style

  • First off, no offense, but this code looks, not so great. You have many major style issues, so here's a list of some of the major ones I see.

    • You have no spaces between operators. You should have space between all operators. For example, if I wanted to assign x to 1, it should look like this, x = 1, and not this. x=1
    • Your variable naming is just horrendous. For example, you named a variable sel. I have no idea what this variable does, or what it's purpose is. Good variable names should not be too short, or too long, and should reflect clearly on that variable's purpose.
    • This code needs to be separated into various different functions/classes. Right now there's some top-level variables, one class, and a while loop doing most of the work. Try and see if there are ways to separate this into separate functions, e.g, a function for checking key actions, or another for rendering the tiles.

    You can find these and many other recommendations in PEP8, the Python coding style guide.


Performance

  • First off, from what I can see, you're doing a lot of looping through the list storing the tile data. This is where your problem is. The more you loop through the data, and the larger the data gets, the slower your program will run.
  • If you have any loops that are looping through the data and checking for an item with a specific value, it's better to use if..in instead.
  • As mentioned in the comments, you can also shorten some of your expressions. For example, the following expression, ((mse[x]/32)*32), can be shortened into (mse[x] & 0x7fffffe0). While shortening expressions is something that isn't easy to do, (even I can't do it very well), see where you can do things like this.

Anyways, I hope that this review helped! If you want me to add on anything else, just ask about it in the comments, and I'll see what I can do.

\$\endgroup\$
8
\$\begingroup\$

You have several if conditions that cannot be true at the same time:

if e.type==MOUSEMOTION:
    # ...

if e.type==KEYUP:
    # ...

if e.type==MOUSEBUTTONUP:
    # ...

Such conditions should be chained with elif in between. Otherwise, even after we already know that e.type == MOUSEMOTION, the other conditions will be evaluated too, needlessly.


You do this kind of thing at so many places, it would be better to put it into a function:

(x / 32) * 32

Perhaps something like this:

def drop_lowest_5_bits(x):
    return x & 0x7fffffe0

As you notice, the name of the function documents what this does.


You could benefit from some more imports, so that you can reduce all that code prefixed by pygame.. For example I would import these names:

from pygame.image import load
from pygame.rect import Rect
from pygame import mouse
from pygame import key
\$\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.