2
\$\begingroup\$

I developed two custom UI components with Pygame: a Button widget and a Frame widget. After placing the Button inside the Frame, I encountered an issue with the Button’s collision detection.

Requirements pygame-ce==2.5.6 python >=3.11

Problem When the Button was nested inside the Frame, its collision area was still being calculated relative to the main screen, not relative to the Frame’s coordinate space. This caused inaccurate interaction behavior when the cursor hovered or clicked on the Button.

My Solution To resolve the issue, I attempted to convert the cursor position by adjusting event.pos inside the BTNBase class. I defined a method named convert_pos to translate the global cursor coordinates into the Frame’s local coordinate system.

Request for Feedback I would like to know whether my current implementation of the Frame widget is structurally correct. Will the current design cause issues in future updates?

import pygame # pygame-ce==2.5.6 from abc import ABC, abstractmethod CLR_SPACE_GREY = (17, 0, 34) # main #110022 CLR_VOID = (5, 13, 37) CLR_NEON_GREEN = (80, 255, 120) # Active #50ff78 CLR_NEON_GREEN_LIGHT = (150, 255, 180) # Hover #96FFB4 CLR_NEON_GREEN_DARK = (20, 120, 50) # Press #147832 BTN_WIDTH = 200 BTN_HEIGHT = 50 BTN_SIZE = (BTN_WIDTH,BTN_HEIGHT) BTN_CLR_NORMAL = CLR_NEON_GREEN BTN_CLR_HOVER = CLR_NEON_GREEN_LIGHT BTN_CLR_PRESS = CLR_NEON_GREEN_DARK class Color: def __init__(self, r:int, g:int, b:int): self.r = r self.g = g self.b = b class Point: def __init__(self,x:int,y:int): self.x = x self.y = y class BTNBase(pygame.sprite.Sprite, ABC): def __init__(self,command:function=None): super().__init__() self.on_click = command self.state = "NORMAL" self.pressed = False @staticmethod def convert_pos(pos:tuple[int,int], offsets:tuple[int,int]): """ to fix frame positioning actually buttons collision is counted relative to main screen surface but to fix it i convert cursor position for when widget is inside frame :param pos: cursor position by `pygame.event` :param offsets: the difference between main pos and frame pos :return: exact position where cursor can find widget """ return (pos[0]-offsets[0] ,pos[1]-offsets[1]) def events(self, event:pygame.event.Event, frame_offsets:tuple[int,int] = (0,0)): if event.type == pygame.MOUSEBUTTONDOWN and event.button == 1: cursor_pos = self.convert_pos((event.pos[0],event.pos[1]), frame_offsets) if self.rect.collidepoint(cursor_pos): self.pressed = True self.state = "PRESS" elif event.type == pygame.MOUSEBUTTONUP and event.button == 1: cursor_pos = self.convert_pos((event.pos[0],event.pos[1]), frame_offsets) if self.rect.collidepoint(cursor_pos): self.curr_time = pygame.time.get_ticks() if self.on_click and self.pressed: self.on_click() self.pressed = False if self.rect.collidepoint(cursor_pos): self.state = "HOVER" else: self.state = "NORMAL" if event.type == pygame.MOUSEMOTION: cursor_pos = self.convert_pos((event.pos[0],event.pos[1]), frame_offsets) if self.rect.collidepoint(cursor_pos): if not self.pressed: self.state = "HOVER" else: self.state = "NORMAL" @abstractmethod def blits(self): pass class Button(BTNBase): def __init__(self,surface:pygame.Surface, width:int=10, height:int=10, x:int=10, y:int=10, text:str="" ,font_size:int=18, font_bold:bool=False, command:function=None): super().__init__(command) self.surface = surface self.background = pygame.Surface((width,height), pygame.SRCALPHA) self.image = self.background self.rect = self.image.get_rect(topleft = (x, y)) self.font = pygame.font.Font(None, font_size) if font_bold: self.font.set_bold(True) self.text = self.font.render(text,True,CLR_SPACE_GREY) self.text_rect = self.text.get_rect() # pos relates to self.image pos self.text_rect.topleft = ((width//2)-(self.text_rect.w//2),(height//2)-(self.text_rect.h//2)) def blits(self): self.image = self.background if self.state == "NORMAL": self.image.fill(BTN_CLR_NORMAL) elif self.state == "HOVER": self.image.fill(BTN_CLR_HOVER) elif self.state == "PRESS": self.image.fill(BTN_CLR_PRESS) self.image.blit(self.text, self.text_rect) self.surface.blit(self.image, self.rect) class Frame(pygame.Surface): def __init__(self, surface:pygame.Surface, size:Point=(10,10), x:int = 10, y:int = 10, bg:Color=(pygame.Color("Black"))): super().__init__(size,pygame.SRCALPHA) self.bg = bg self.fill(self.bg) self.surface = surface self.rect = pygame.rect.Rect((x,y),size) self.widgets = dict() self.widgets_count = 0 def add_widget(self,widget:pygame.sprite.Sprite): # TODO add feature to add multiple widgets at once self.widgets_count += 1 self.widgets[self.widgets_count] = widget def events(self,event): self.fill(self.bg) for key,widget in self.widgets.items(): widget.events(event, frame_offsets=(self.rect.x, self.rect.y)) widget.blits() def blits(self): self.surface.blit(self, self.rect) if __name__ == "__main__": pygame.init() demo_screen = pygame.display.set_mode((400,400)) frame = Frame(demo_screen,(200, 300), x=40, y=40) # Button(surface, width, height, x-pos, y-pos, text, ...) btn1 = Button(frame, 100 , 50 , 40 , 40, "BTN1",font_size=22, command=lambda : print("BTN1")) btn2 = Button(frame, 100 , 50 , 40 , 100, "BTN2",font_size=22, command=lambda : print("BTN2")) btn3 = Button(frame, 100 , 50 , 40 , 160, "BTN3",font_size=22, command=lambda : print("BTN3")) frame.add_widget(btn1) frame.add_widget(btn2) frame.add_widget(btn3) while True: for event in pygame.event.get(): if event.type == pygame.QUIT: break; demo_screen.fill(CLR_SPACE_GREY) frame.events(event) frame.blits() pygame.display.update() ```
\$\endgroup\$

2 Answers 2

Reset to default
1
\$\begingroup\$

skipped events

 while True: for event in pygame.event.get(): if event.type == pygame.QUIT: break; demo_screen.fill(CLR_SPACE_GREY) frame.events(event)

That for event loop makes me nervous; it might drop events.

I think the intent is we shall consume either 0 or 1 events, typically a non-quit event. But if there's more than one queued up, then frame.events() only sees the last one, which seems bad. You might want demo_screen... and following indented one level deeper, so that code sees fresh events.

Typically the main loop has a goal like "maintain a 30 FPS refresh rate", which I don't see here.

dataclass

class Color: ... class Point:

No need for all that boilerplate. Just make each one a @dataclass.

keyword-only argument

 class BTNBase(pygame.sprite.Sprite, ABC): def __init__(self, command:function=None): ... class Button(BTNBase): def __init__(self, surface:pygame.Surface, width:int=10, height:int=10, x:int=10, y:int=10, text:str="", font_size:int=18, font_bold:bool=False, command:function=None):

Those two signatures are not compatible.

Now, the calling code happens to use a , command= kwarg, so it works. But the signatures really need a , *, before that arg, in order to force a proper calling sequence every time. I would expect that some configurations of ruff or mypy would call out the discrepancy, so the OP code wouldn't lint clean.

number of args

 # Button(surface, width, height, x-pos, y-pos, text, ...)

Also, we might squish both (w, h) & (x, y) into size & position parameters. You wrote a good, helpful comment. But the need to write it suggests that the inconveniently large number of args makes this harder to call than desired.

Enum

class BTNBase( ... ): ... self.state = "NORMAL"

Consider turning that into an Enum class.

 if self.state == "NORMAL": self.image.fill(BTN_CLR_NORMAL) elif self.state == "HOVER": self.image.fill(BTN_CLR_HOVER) elif self.state == "PRESS": self.image.fill(BTN_CLR_PRESS)

There's an opportunity to hang a color value on each of the enumerated values, saving us all those elifs.

inheritance

 def convert_pos(pos:tuple[int,int], offsets:tuple[int,int]):

From this and from the docstring apology it seems like you feel this is a bit kludgey. I believe it could be cleaner. And maybe the right name for this is def pos_within_frame(

class Button(BTNBase):

I am broadly skeptical that decomposition into those two classes is helping this code. It may have been a good idea at one time, but then things happened during your edit - debug cycle, and we discovered the need for some additional variables. Consider merging into a single class, or if not, shuffle some existing responsibilities between the classes.

I feel that there is little point to modeling a disconnected button, and that constructors should always insist on knowing which frame the button is within. With that in place, we already have the frame's position, without caller needing to redundantly specify some offsets.

using a dict as a list

 def add_widget( ... ): self.widgets_count += 1 self.widgets[self.widgets_count] = widget

I can't imagine that storing widgets in a dict is helpful, here. Init to a [] list, do self.widgets.append(widget), and ditch that count attribute in favor of len(self.widgets).

\$\endgroup\$
1
\$\begingroup\$

I have just read the code and made no attempt to run it, so this is a high level overview but some things are visually obvious.

DRY (do not repeat yourself)

In function events:

cursor_pos = self.convert_pos((event.pos[0],event.pos[1]), frame_offsets)

is repeated for every if condition. So it would be sufficient to move this statement to the top of the function.

Inside the same function, a condition is repeated, I have put a comment. Just merge these blocks:

 elif event.type == pygame.MOUSEBUTTONUP and event.button == 1: cursor_pos = self.convert_pos((event.pos[0],event.pos[1]), frame_offsets) if self.rect.collidepoint(cursor_pos): # <-- merge self.curr_time = pygame.time.get_ticks() if self.on_click and self.pressed: self.on_click() self.pressed = False if self.rect.collidepoint(cursor_pos): # <-- merge self.state = "HOVER" else: self.state = "NORMAL"

So this function can be shortened at no cost before we even consider refactoring it. Unless you are stuck with an old version of Python I would consider replacing the if block with a match clause instead.

Tuple

And instead of doing:

self.convert_pos((event.pos[0],event.pos[1]), frame_offsets)

you could just send event.pos as-is, since it is already a tuple:

self.convert_pos(event.pos, frame_offsets)

Besides, your function convert_pos is going to extract the tuple components anyway. It's the same for parameter offsets, which as you can see works without that unneeded splitting.

Magic numbers

I am not much familiar with pygame but I believe event.button == 1 refers to the left mouse click, so I would add a constant for it, unless pygame already provides one. So you could write more expressive code like:

elif event.type == pygame.MOUSEBUTTONUP and event.button == LEFT:

Coordinates

In some functions like convert_pos, it looks like you are returning coordinates. You should really leverage that Point class that already exists in your code. In fact, it could just be a named tuple since it implements no methods. A named tuple is easier to handle than a regular tuple because you don't have to worry about parameter order.

You have created a class Color, but it's only used here:

def __init__(self, surface:pygame.Surface, size:Point=(10,10), x:int = 10, y:int = 10, bg:Color=(pygame.Color("Black"))):

It seems to be useless.

Argument size uses a Point instance. Your signature should rather be: size=Point(10, 10) or even more accurate: size=Point(y=10, x=10). I believe that makes intention even clearer.

In that __init__ method, we still have x and y but it would make sense to replace both with another Point instance named rectangle_position perhaps. Using Point makes perfect sense here, and all coordinates should use this. There is lots of room for harmonizing the code along these lines.

Since you are dealing with shapes, this article should interest you.

To address your concerns:

Will the current design cause issues in future updates?

I see no reason why, if you mean future updates of the pygame lib. In a programming exercise like this, I believe my main concern would be guardrails against negative coordinates, which are probably a flaw here. Unit tests should be written to prove that the calculation of coordinates is sound and test edge cases.

\$\endgroup\$
2
  • \$\begingroup\$ I see your code showing up with ".ini" and "C#" highlighting. My code used even more than that, including "Haskell". We have just a "pygame" tag. I'm going to try tagging the OP as "python", and see if that leads to less language guessing. \$\endgroup\$ Commented 7 hours ago
  • \$\begingroup\$ Yup, that fixed it. (Alas, "python-3.x" has zero effect on syntax highlighting.) \$\endgroup\$ Commented 7 hours ago

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.