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

stop fighting SDL2 resizing code #1705

Merged
merged 4 commits into from May 29, 2020

Conversation

robertpfeiffer
Copy link
Contributor

@robertpfeiffer robertpfeiffer commented May 3, 2020

The resizing API has been changed between SDL1 and SDL2. This branch implements resizing the "proper" way, according to the documentation and comments by the SDL2 team forums and bug trackers.

Instead of swallowing window events and re-requesting a pointer to the display surface every frame, we generate additional VIDEORESIZE events, and install an event watcher to automatically handle resizing. On creating a new VIDEORESIZE event, we remove the old ones from the queue. SDL2 already automatically handles resizing when SDL_Renderer is involved.

The current display surface is still automatically re-sized when a SIZE_CHANGED SDL_WINDOWEVENT is detected, but this behaviour can be disabled with pg.display._set_autoresize(False).

In that case, you must call this code, to get a valid display surface:

    if event.type==pg.VIDEORESIZE:
        screen=pg.display.get_surface()

Otherwise, the display surface is updated automatically, and you can just query the new size like this:

    if event.type==pg.VIDEORESIZE:
        screen_width, screen_height = screen.get_size()

If you're using SCALED or SDL_Renderer directly, there is no need to call anything on resize, and there is no more reason to make expensive calls to pygame.display.set_mode() for every VIDEORESIZE event any more.

@robertpfeiffer robertpfeiffer force-pushed the actually-fix-resize branch 2 times, most recently from f6a670c to c91053e Compare May 3, 2020 12:30
@nthykier
Copy link
Contributor

nthykier commented May 9, 2020

Looks like it would fix #1671 as well.

Copy link
Contributor

@nthykier nthykier left a comment

Choose a reason for hiding this comment

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

I spotted two compiler warnings that I think we should fix.

src_c/display.c Show resolved Hide resolved
src_c/display.c Show resolved Hide resolved
@nthykier
Copy link
Contributor

nthykier commented May 9, 2020

With this branch, I notice a weird resize in the Endgame: Singularity game

gh-1705-weird-resize

On resize, it is sometimes "stuck" with a larger window that is not properly redrawn (see the bottom and the right side of the window). I suspect we are swallowing an event too much here. But at last the "wobbly" resizing effect appears to be gone.

@robertpfeiffer
Copy link
Contributor Author

With this branch, I notice a weird resize in the Endgame: Singularity game

I'll have to look into this game to tell you what's going on. Maybe we'll need to emulate the old beaviour by default (do not resize display surface in flip, let user handle VIDEORESIZE) and switch to new resize behaviour when requested (do not resize display surface in flip, invalidate automatically, user must handle SIZE_CHANGED).

In any case, only issuing one VIDEORESIZE at a time and only allowing one of them to be in the queue at the same time is the right thing to do.

@robertpfeiffer
Copy link
Contributor Author

The tests run through (click on the travis and appveyor links), but I haven't yet added an interface to disable the legacy resizing behaviour or investigated the bug in singularity.

@nthykier
Copy link
Contributor

The tests run through (click on the travis and appveyor links), but I haven't yet added an interface to disable the legacy resizing behaviour or investigated the bug in singularity.

Thanks. :)

For singularity, you would need to look at the master branch and you can find the code at https://github.com/singularity/singularity. Note that no released code of singularity the resize code, so I am happy to rewrite the singularity resizing code as long as there is a way to support both SDL1 and SDL2 resizing.

@robertpfeiffer
Copy link
Contributor Author

robertpfeiffer commented May 17, 2020

as long as there is a way to support both SDL1 and SDL2 resizing.

That should be easy enough. If we add a way to disable generation of VIDEORESIZE events from SDL2 SIZE_CHANGED events, you can listen for SIZE_CHANGED events instead.
Of course, SIZE_CHANGED is not a thing in SDL1.2 or pygame 1.9.6, so it's not backward compatible.

@nthykier
Copy link
Contributor

nthykier commented May 23, 2020

My tests so far:

  • The default works under pygame2/SDL2 in singularity. It occasionally gets the resize slightly wrong and in correctly renders artefacts as shown previously, but the resizing is generally smooth. So a net improvement since the last review.
  • When using pygame.event._set_gen_videoresize(False), then resizing stops working for me. The display is updated but the application is not notified of the change and still renders on its old (cached) window size. AFAICT, there is no new event to listen for (and VIDEORESIZE disappears), but it sounded like there should be a new SIZE_CHANGED event.

@illume
Copy link
Member

illume commented May 23, 2020

Looks like there's some conflicts with the recently merged SCALED PR.

@robertpfeiffer
Copy link
Contributor Author

@illume I am 90% sure the CI is broken, but I'll need to re-test the code anyway in its current state

Copy link
Contributor

@nthykier nthykier left a comment

Choose a reason for hiding this comment

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

I could do with an example of how an application in Pygame 2/SDL 2 can get notified about being resized (so it can update its understanding of how large the window is and repaint the difference) when using the "newstyle resizing" (VIDEORESIZE is called legacy as of these changes, so I assume the newstyle should react to a different event)

Beyond that, I think this is a net improvement and I presume it would deal nicely with #1492.

commit 98f00db5dd73148cd47f49fe2ec7496c2aff0791
Author: Robert Pfeiffer <bob@blubberquark.de>
Date:   Tue May 26 17:26:24 2020 +0200

    Relax integer scaling for maximised windows

commit 3056a2ec700342e991a54ab0e5a0b99d09fab7f6
Author: Robert Pfeiffer <bob@blubberquark.de>
Date:   Tue May 26 16:56:46 2020 +0200

    un-revert

commit 96902ddb3659bbaee439520c90a9981d0a45ed70
Author: Robert Pfeiffer <bob@blubberquark.de>
Date:   Tue May 26 16:49:42 2020 +0200

    .

commit 5f596e5
Author: Robert Pfeiffer <bob@blubberquark.de>
Date:   Mon May 18 11:08:46 2020 +0200

    make surface types in base.c more specific than PyObject

commit e02c265
Author: Robert Pfeiffer <bob@blubberquark.de>
Date:   Mon May 18 10:41:29 2020 +0200

    configurable VIDEORESIZE emulation

commit 4077bfb
Author: Robert Pfeiffer <bob@blubberquark.de>
Date:   Wed May 13 15:03:11 2020 +0200

    casts to make compiler happy

commit d3861d2
Author: Robert Pfeiffer <bob@blubberquark.de>
Date:   Tue May 5 19:37:38 2020 +0200

    pass through events properly, delete duplicates

     RESIZED events shouldn't be edited into pygame VIDEORESIZE events (effectively
    removing all RESIZED events from the queue), and SIZE_CHANGED  shouldn't be
    eaten by the event filter either, because other parts of SDL2 (like
    SDL_Renderer implementations) listen for these events.

    VIDEORESIZE events in pygame 1.9.6 used to work differently from SIZE_CHANGED
    behaviour anyway: The game still had to react to an SDL1.2 VIDEORESIZE event by
    calling pygame.display.set_mode(). That's not really necessary in SDL2. The
    window size is changed by SDL2 at the same time when the event is generated,
    and the game is just informed by SDL2 that the window has already been changed.

    This is signalled with a SIZE_CHANGED event (accompanied by a RESIZED event if
    it was caused by the user dragging the corner of the window). Games in SDL2 are
    supposed to listen to SIZE_CHANGED and then get the new window surface - unless
    they are using SDL_Render, because a Renderer listens for these events through
    an event watcher.

    Editing events in the filter broke this feature, but it broke two more things:
       1. The aforementioned resizing listener in SDL_Renderer
       2. When a new RESIZED event is generated, all old SIZE_CHANGED and RESIZED
          events are deleted from the event queue by SDL2. SDL2 doesn't know anything
          about VIDEORESIZE events and cannot remove them, leading to
          duplicate/obsoleted VIDEORESIZE events in the queue.
       3. pygame.display.set_mode() changes the display and generates a new
          SIZE_CHANGED event, but not a RESIZED event.

commit 6b55262
Author: Robert Pfeiffer <bob@blubberquark.de>
Date:   Sun May 3 17:16:07 2020 +0200

    backwards compatibility

commit 26fb925
Author: Robert Pfeiffer <bob@blubberquark.de>
Date:   Sun May 3 14:09:18 2020 +0200

    stop fighting SDL2 resizing code

    fixes pygame#1698, fixes pygame#1624, fixes pygame#1492
@illume
Copy link
Member

illume commented May 28, 2020

Did some testing on MacOS and windows...

MacOS catalina SDL 2.0.12, metal:

  • setmodescale.py works nicely resizing.
  • eventlist.py is working better than before. The slowness of resizing is gone. But the screen surface still gets corrupted. when clicking the maximize button goes into fullscreen, then pops into a full-desktop sized window, the other ones go into fullscreen mode and stay there.
  • resizing_new.py seems to work ok
  • I'm getting a GLError with the glcube example, unrelated to the resizing branch.

Windows 10 home, SDL 2.0.12:

  • setmodescale, and eventlist work ok. There's no corrupted display with eventlist like on MacOS.
  • resizing_new.py seems to work ok
  • glcube resizes ok with RESIZABLE, and with RESIZABLE | SCALED. With SCALED it starts a bit too big. The window is placed maybe 20 pixels or so above the top of the screen.

Copy link
Member

@illume illume left a comment

Choose a reason for hiding this comment

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

👍 🎉 thanks :)

@nthykier are you happy with it now?

@illume illume added base pygame.base display pygame.display event pygame.event labels May 28, 2020
@nthykier
Copy link
Contributor

I think I still missing a "This is how you are supposed to do resizing with SDL 1 vs. SDL 2 when the game needs to be notified of resizing" before I can review it any further.

@nthykier
Copy link
Contributor

(my comment still stands, but I do not want to be holding this PR back as it is probably a lot better than status quo)

@illume
Copy link
Member

illume commented May 29, 2020

I think I still missing a "This is how you are supposed to do resizing with SDL 1 vs. SDL 2 when the game needs to be notified of resizing"

Ok, let's look at this in another PR :)

@illume illume merged commit 3ba4b39 into pygame:master May 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
base pygame.base display pygame.display event pygame.event
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants