6
\$\begingroup\$

I wrote this to teach myself how to make a menu with buttons and I want to know if I've done well and if there's any way to make it shorter and/or more efficient.

import pygame, sys
from pygame import *
run = True
global newBG
newBG = False

#-------------------------------------------------------------------------------

def mainLoop():
    global win, newW, newH, newBG
    newW, newH = (960 , 508)
    win = pygame.display.set_mode ((newW, newH) , pygame.RESIZABLE)
    pygame.display.set_caption("SMC")

    backGround = 0


    while run:


        global mouse, currentScreen
        mouse = pygame.mouse.get_pos()

        currentScreen()

        while newBG:
            drawBackground()
            newBG = False

        for event in pygame.event.get():
            if event.type == pygame.QUIT:
                pygame.quit()
                sys.exit()
            if event.type == pygame.VIDEORESIZE:
                win = pygame.display.set_mode ( (event.w, event.h) , pygame.RESIZABLE )       
                newW, newH = event.w, event.h
                drawBackground()
                uiPlate()
            if event.type == pygame.MOUSEBUTTONDOWN:
                for b in buttonList:
                    b.click()

        pygame.display.update()

#------------------------------------------------------------------------------

def mainMenu():
    global backGroundImage, buttonList, currentScreen

    backGroundImage = pygame.image.load('Background.jpg')
    ButtonPic = pygame.image.load('Button.jpg')
    ButtonHoverPic = pygame.image.load('ButtonHover.jpg')
    ButtonPressPic = pygame.image.load('ButtonPress.jpg')

    buttonList = [
    button(screenSwap, secondMenu, ButtonPic, ButtonHoverPic, ButtonPressPic, .1, .1, .1, .1),
    button('Hello', None, ButtonPic, ButtonHoverPic, ButtonPressPic, .1, .25, .1, .1),
    button('Hello to you!', None, ButtonPic, ButtonHoverPic, ButtonPressPic, .1, .50, .1, .1)
]

    for b in buttonList:
        b.drawButton()

#------------------------------------------------------------------------------

def secondMenu():
    global backGroundImage, buttonList

    backGroundImage = pygame.image.load('Button.jpg')
    ButtonPic = pygame.image.load('Background.jpg')
    ButtonHoverPic = pygame.image.load('ButtonHover.jpg')
    ButtonPressPic = pygame.image.load('ButtonPress.jpg')

    buttonList = [
    button(screenSwap, mainMenu, ButtonPic, ButtonHoverPic, ButtonPressPic, .75, .1, .1, .1),
    button('Goodbye', None, ButtonPic, ButtonHoverPic, ButtonPressPic, .75, .25, .1, .1),
    button('Goodbye to you friend!', None, ButtonPic, ButtonHoverPic, ButtonPressPic, .75, .50, .1, .1)
]

    for b in buttonList:
        b.drawButton()

#------------------------------------------------------------------------------

uiX = 0
uiY = 0
uiW = 960
uiH = 508
def uiPlate():
    global uiW, uiH, uiX, uiY
    uiW = (int(newW))
    uiH = (int(uiW*0.5296875))
    if uiH > newH:
        uiH = int(newH)
        uiW = (int(uiH/0.5296875))
    uiX = (int((newW - uiW)/2))
    uiY = (int((newH - uiH)/2))
##    pygame.draw.rect(win, (100, 255, 100,), (uiX, uiY, uiW, uiH))
##    print(uiW, uiH, uiX, uiY)

#------------------------------------------------------------------------------

def drawBackground():
    global backGround
    backGround = pygame.transform.scale(backGroundImage, (newW, newH))
    win.blit((backGround), (0,0))

#------------------------------------------------------------------------------

class button:
    global uiX, uiY, uiW, uiH
    def __init__(self, action, actionProps, ButtonPic, ButtonHoverPic, ButtonPressPic,  x, y, w, h):
        self.ButtonPic = ButtonPic
        self.ButtonHoverPic = ButtonHoverPic
        self.ButtonPressPic = ButtonPressPic

        self.bx = int(uiX + (x * uiW))
        self.by = int(uiY + (y * uiH))
        self.bw = int(w * uiW)
        self.bh = int(h * uiH)

        self.action = action
        self.actionProps = actionProps

    def drawButton(self):
        click = pygame.mouse.get_pressed()

        ButtonPic = pygame.transform.scale(self.ButtonPic, (self.bw, self.bh))
        ButtonHoverPic = pygame.transform.scale(self.ButtonHoverPic, (self.bw, self.bh))
        ButtonPressPic = pygame.transform.scale(self.ButtonPressPic, (self.bw, self.bh))

        win.blit(ButtonPic, (self.bx, self.by))
        if ((self.bx < mouse[0] < (self.bx + self.bw)) and (self.by < mouse[1] < (self.by + self.bh))):
            win.blit(ButtonHoverPic, (self.bx, self.by))
            if click[0]:
                win.blit(ButtonPressPic, (self.bx, self.by))

    def click(self):
        if ((self.bx < mouse[0] < (self.bx + self.bw)) and (self.by < mouse[1] < (self.by + self.bh))):
            if self.actionProps != None:
                self.action(self.actionProps)
            else:
                print(self.action)

#------------------------------------------------------------------------------

def screenSwap(screen):
    global currentScreen, newBG
    currentScreen = screen
    newBG = True

#------------------------------------------------------------------------------
currentScreen = mainMenu
mainLoop()

Sorry about the length, if it's too much to bother looking through, I'd like to be shown some code for a well-made menu system that I can compare to.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ mainMenu and secondMenu are incorrectly indented. Please fix this before review. \$\endgroup\$ Commented Oct 21, 2019 at 3:13
  • \$\begingroup\$ Not sure what you mean. \$\endgroup\$ Commented Oct 21, 2019 at 7:20
  • \$\begingroup\$ Try copying your code from here and running it. It'll show you where you have syntax failures due to indentation. \$\endgroup\$ Commented Oct 21, 2019 at 12:52

1 Answer 1

Reset to default
5
\$\begingroup\$

PEP-0008

Follow PEP-0008 guidelines. Use an automated checker (pylint, pyflakes, ...) to check your code for violations.

Issues include:

  • Variable, function, and method names should be snake_case. For example newBG should be new_background, mainLoop should be main_loop, and drawButton should be draw_button.
  • No space between a function/method name and the opening parentheses. set_mode ( should be set_mode(.
  • No spaces before commas, and no spaces between adjacent parenthesis, or after opening parenthesis or before closing parenthesis. set_mode ( (event.w, event.h) , pygame.RESIZABLE ) should be set_mode((event.w, event.h), pygame.RESIZABLE).
  • Class names should begin with uppercase letters. So Button instead of button.

to name a few. Run a checker to see all of them.

Import *

import pygame, sys
from pygame import *

Are you importing pygame, or are you importing everything from inside pygame? Do one or the other, preferably the former, but definitely not both!

Global variables are Global

global newBG
newBG = False

newBG is being defined in the global scope. global newBG is not needed, and is useless.

Type Consistency

What is the type of backGround? Is it an image? If so, then explain:

    backGround = 0

It looks like you are trying to assign an integer to something which normally holds an image. But wait! It is a local variable, and is not used anywhere.

While is not If

Is this a loop or an if statement?

    while newBG:
        drawBackground()
        newBG = False

It can never execute more than once, because it sets the loop condition to False. The following is clearer:

    if newBG:
        drawBackground()
        newBG = False

Integer Arithmetic

If you don't want floating point values, don't divided by two. Instead integer-divide by two!

Instead of this:

uiX = (int((newW - uiW)/2))
uiY = (int((newH - uiH)/2))

write this:

uiX = (newW - uiW) // 2
uiY = (newH - uiH) // 2

Persistance

Your main loop reads roughly:

while run:

    currentScreen()

    # Handle events

    pygame.display.update()

currentScreen() either calls mainMenu() or secondMenu():

def mainMenu():
    # load 4 images
    # create 3 button objects in a list
    # draw button objects

def secondMenu():
    # load 4 images
    # create 3 button objects in a list
    # draw button objects

So ... every iteration of your main event loop (say, 30 to 60 times a second), you are:

  • LOADING MULTIPLE IMAGES
  • CREATING UI ELEMENTS
  • drawing some stuff
  • DISCARDING THE IMAGES AND UI ELEMENTS

Forgive the above screaming. You don't want to waste time while painting your UI doing repetitive, redundant file I/O. Load the images once, at startup or the first time they are used. Create your buttons once, or at the very most, create them only when you swap to the new screen. Populate the button list when switching to this new screen. As this button list must maintain its value across multiple iterations of the event loop, you'll want this as a global variable, or ...

Stop Using Global Variables

Create a main class for your application, and store your "global" information as main class self members.

Create a screen class, and create two instances of it: one for your main menu screen, the other for your second menu. The screen class would hold:

  • the button list for that screen.
  • the background image for that screen.
  • a scaled copy of the background image (so you don't have to rescale it each iteration of the event loop!)

If the application detects VIDEORESIZE, and when changing screens, it would tell the screen object, so the screen object can re-layout its objects and rescale the background image.

Single-Coat Paint

What does this do?

    win.blit(ButtonPic, (self.bx, self.by))
    if ...:
        win.blit(ButtonHoverPic, (self.bx, self.by))
        if ...:
            win.blit(ButtonPressPic, (self.bx, self.by))

Unless ButtonPic, ButtonHoverPic and ButtonPressPic are not all the same size, or if ButtonHoverPic or ButtonPressPic have transparent areas, the above code can paint the button only to immediately repaint the button, and perhaps immediately repaint it a third time. Given that Premium One-Coat Guaranteed Paint™ is likely being used, this is doubling or tripling the cost of painting for no gain.

Instead, I'd recommend something more like:

    image = self._image
    if ...:
        if ...:
            image = self._press_image
        else:
            image = self._hover_image
    win.blit(image, (self.bx, self.by)
\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.