Add window example #2626
Add window example #2626
Conversation
Starbuck5
force-pushed
the
add-window-example
branch
from
d5e6095 to
a02870a
Compare
There was a problem hiding this comment.
Nice work on the example! 😎
Seeing this example in action has gotten me more interested in Window 😄 as it demonstrates new cool stuff and also serves as a good way to test things in action
I added a couple of reviews, some reviews for the example, and also a few API reviews that I noticed while playing around with the example.
|
|
||
| if event.key == pygame.K_1: | ||
| for win, _ in windows_and_surfaces: | ||
| win.size -= pygame.Vector2(WIN_GROW_SPEED, WIN_GROW_SPEED) |
There was a problem hiding this comment.
API comment
Continuously pressing 1 leads to the error
ValueError: width or height should not be less than or equal to zero
Interestingly, this issue does not happen for the "main window" which has minimum_size set to some larger than (0, 0). In that case, the size is implicitly capped with no python errors raised.
This could be an API inconsistency? I'd like an error to be not raised here and it should just cap at (0, 0)
There was a problem hiding this comment.
It's unclear reading your comment whether you see the minimum_size of the window is explicitly set to contain the instruction text.
It would happen to the main window if it was ever told to do a negative size there. Which it can't because the minimum size dimensions are higher than WIN_GROW_SPEED.
I agree it shouldn't be possible to crash the example by messing around with it, but I'm not sure what the best solution would be.
| surface.blit(rendered_instructions, centered_rect) | ||
| win.flip() | ||
|
|
||
| clock.tick(144) |
There was a problem hiding this comment.
Would be a small and nice addition to set FPS in the title for the main window, also serves as a demonstration of the title property.
Also how about something like window.id in the titles for all windows?
| pygame.init() | ||
| pygame.key.set_repeat(500, 100) | ||
|
|
||
| main_window = pygame.Window("demo window", (500, 500), resizable=True) |
There was a problem hiding this comment.
could the main window also be always_on_top?
Another trivial note, how about setting an icon with set_icon?
There was a problem hiding this comment.
At least on Windows, the spawned windows spawn in the center of the main window (I'm not controlling the spawn point). So if the main window was always_on_top it would seem like the spawn window command does nothing, because the other windows would not be visible unless you move the main window with the mouse.
I was looking around the examples data folder for art to use for this, (icons, backgrounds), and none of it spoke to me. I don't feel the need to set an icon in this example.
|
|
||
| if event.key == pygame.K_2: | ||
| for win, _ in windows_and_surfaces: | ||
| win.size += pygame.Vector2(WIN_GROW_SPEED, WIN_GROW_SPEED) |
There was a problem hiding this comment.
might also be a good idea to cap it to some upper bound? Perhaps using the maximum_size property?
| try: | ||
| event.window.opacity = 1.0 | ||
| except pygame.error: | ||
| pass |
There was a problem hiding this comment.
How about adding a event.window.focus() here?
| instructions = """Welcome to the window demo! | ||
| Controls: | ||
| m.) maximize main window | ||
| n.) minimize main window |
There was a problem hiding this comment.
In addition to maximise/minimise, could also two keys for add/remove full screen?
| win.opacity = UNHOVERED_OPACITY | ||
| except pygame.error: | ||
| pass | ||
| windows_and_surfaces.append((win, win.get_surface())) |
There was a problem hiding this comment.
API comment
This has gotten me thinking about the API. As a user, I think it would be much neater if we could do something like win.surface and not have to explicitly track the output of win.get_surface. Even helps conceptually to show that "this surface instance is a special window-surface tied to the window"
There was a problem hiding this comment.
Well I think I could be calling get_surface() every time I need the surface. I'm just not sure how that works SDL-side.
There was a problem hiding this comment.
This is also why I thought of subclassing window, issue raised as #2625. In this demo it would be nice to store the color of the window with the window. (Store arbitrary things in instance of class, subclass)
There was a problem hiding this comment.
So, SDL stores the surface in the window struct (both in SDL2 and SDL3), and if the surface needs updating it updates the surface, so for most of the calls to this function it is just a cheap struct member access.
On the python side, we could basically the change this get_surface to a property.
There was a problem hiding this comment.
Here is a commit on a branch I made on top of yours, implementing my idea: 6b620f1
I also updated this example to see/test the property in action
|
|
||
| if event.key == pygame.K_RIGHT: | ||
| for win, _ in windows_and_surfaces: | ||
| win.position += pygame.Vector2(WIN_MOVE_SPEED, 0) |
There was a problem hiding this comment.
API comment
SMH my head, window.position is also broken under direct wayland
Nothing moves when I press the arrow keys. 😢
All these "direct wayland" issues are fine/ignorable for now because these issues don't happen under XWayland which SDL2 picks by default, but this could be a problem in the SDL3 era if SDL3 devs decide to do wayland-default.
There was a problem hiding this comment.
This is by design. Wayland does not want applications to know this for API design and security reasons. There is nothing SDL3 can do about it short of sending patches to KDE and GNOME and sway
There was a problem hiding this comment.
Do I need to add try/except pygame.error blocks around them then? I did it for opacity given your previous report.
I'm also unsure if we really want to raise an exception if the operation is not supported. We could return True/False about success?
There was a problem hiding this comment.
In this case the position is silently ignored by SDL, like always_on_top. Only opacity explicitly errors.
I think for this example, we should not have this position-moving thing as a highlight, and make it more low-key somehow?
There was a problem hiding this comment.
This example also reveals memory leaks 😓
Open task manager while this example is running and play around with this example. Every time a new window is spawned the memory usage goes up as expected, but closing the window does not reduce the memory consumption
Similarly everytime the window is resized memory usage increases, even when downsizing.
These memory leaks are not insignificant, I believe entire window surfaces are being leaked, so depending on the window size and display resolutions we may be leaking a lot.
Task for #2603