Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add get_surface() for Window class #2350

Merged
merged 19 commits into from
Nov 19, 2023

Conversation

yunline
Copy link
Contributor

@yunline yunline commented Jul 26, 2023

  • Window.get_surface()
  • Window.update_from_surface()
  • Auto resize
import pygame
from pygame._sdl2 import video

pygame.init()

win = video.Window()
sf = win.get_surface()

sf.fill((255,0,0))
win.update_from_surface()

while 1:
    pygame.event.get()
Screenshot

图片

@yunline yunline added New API This pull request may need extra debate as it adds a new class or function to pygame _sdl2 pygame._sdl2 video pygame.video labels Jul 26, 2023
@yunline yunline marked this pull request as ready for review July 27, 2023 16:36
@yunline yunline requested a review from a team as a code owner July 27, 2023 16:36
@Starbuck5 Starbuck5 added this to the 2.4.0 milestone Jul 29, 2023
src_c/window.c Outdated Show resolved Hide resolved
src_c/window.c Outdated Show resolved Hide resolved
Copy link
Member

@MyreMylar MyreMylar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this works great on windows OS and lets me use the window class to do familiar CPU blitting and drawing.

I agree that we should remove the list of rects overload from update_from_surface() however. The realistic speed ups from this functionality on most modern platforms are too minor, and too niche, to justify continuing it and the confusion it brings when users try to use it to speed up their games. They put a bunch of effort in to maintain an accurate list of sections of the screen that have changed this loop, only to then find that it in many cases it actually makes the program run slower because they have more than 15 or so moving objects in their application and the per-rectangle-cost of updating has exceeded the saving from the reduced area updated.

See #2514 for more discussion.

src_c/window.c Outdated
@@ -1113,7 +1085,5 @@ MODINIT_DEFINE(_window)
return NULL;
}

SDL_AddEventWatch(_resize_event_watch, NULL);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just commenting because I feel this change could be independent of this PR. I think it could fix another one of those init+quit+init issues.

But I would prefer moving this to the display specific init function instead, instead of messing with base.c. The diff would also be much less I reckon

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, I assumed the changes were happening in display.c but I now see they are in window.c. My suggestion would still be along similar lines, create an "init-quit hook" (see what display.c does) for window, and set/unset this filter from there.

{"_internal_mod_init", (PyCFunction)_window_internal_mod_init, METH_NOARGS,
"auto initialize for _window module"},
{"_internal_mod_quit", (PyCFunction)_window_internal_mod_quit, METH_NOARGS,
"auto quit for _window module"},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes these hooks look good, but I think they should be called from the display init/quit hook instead of the base pygame init/quit function (like how event and time are handled right now). This is so that if someone only does pygame.display.init() instead of the full pygame.init() we would still want the window hooks to run, as they are related

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Copy link
Member

@Starbuck5 Starbuck5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get this in. Thanks yunline!

Note from testing: window resize doesn't work properly when using a borrowed window--

import pygame
from pygame._sdl2 import Window

pygame.init()

screen = pygame.display.set_mode((500,500))
window = Window.from_display_module()

window_screen = window.get_surface()
running = True
while running:
    for event in pygame.event.get():
        if event.type == pygame.QUIT:
            running = False

        if event.type == pygame.KEYDOWN:
            if event.key == pygame.K_1:
                window.size = (500,500)
            if event.key == pygame.K_2:
                window.size = (700,700)

    window.update_from_surface()
pygame.quit()

After this is merged I have several things I'd like to do myself on this. I want to change update_from_surface() to flip(), I'd like to write some things in the docs about how it's analogous to set_mode and flip(), and I'd like to add a standalone window software rendering example.

So even though there is still improvement to we can make to this API, the way forward is not to delay this PR any longer, but to get it to main.

The way forward on this is getting this PR merged. @yunline I'm leaving it to you to merge-- I think some of the commits could be squashed down, but it's up to your discretion.

@yunline yunline merged commit 9f054e1 into pygame-community:main Nov 19, 2023
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New API This pull request may need extra debate as it adds a new class or function to pygame _sdl2 pygame._sdl2 video pygame.video window
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants