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

Window(opengl=True) will now create an OpenGL context #2659

Merged
merged 15 commits into from
May 25, 2024

Conversation

oddbookworm
Copy link
Member

No description provided.

@oddbookworm oddbookworm added opengl window pygame.Window labels Jan 7, 2024
@oddbookworm oddbookworm requested a review from a team as a code owner January 7, 2024 07:23
@Starbuck5
Copy link
Member

Can you motivate this change?

I know in pygame.display, opengl displays have a different implementation of flip(), and this doesn't happen in the Window class. Makes me think this PR doesn't put opengl windows into a working state.

Does this make opengl windows a usable workflow?

@oddbookworm
Copy link
Member Author

@tank-king asked about it in the pgc discord server and mentioned that Window(opengl=True) doesn't create a context, which is what the docs say. As-is, if you don't want extra dependencies, you need Window.from_display_module() to get a window with an opengl context associated. You can get away with using contexts from other libraries, but each of them has their own window management as far as I know, so why would they use pygame-ce then? I'm not too familiar with opengl backend stuff though, so I probably missed/borked something here

@Starbuck5
Copy link
Member

Starbuck5 commented Jan 17, 2024

I'm not familiar with opengl stuff either. I don't want to merge this until I can see that it actually implements a fully working opengl sample.

Converted it to be a draft in the meantime. You can mark it ready for review if you disagree.

@Starbuck5 Starbuck5 marked this pull request as draft January 17, 2024 06:11
@oddbookworm
Copy link
Member Author

I just pushed a couple commits that gets opengl fully working. Here's a sample piece of code that uses the zengl library as the frontend of opengl, but a pygame.Window without having to go through pygame.display first

import pygame
import zengl

window_size = (1280, 720)

pygame.init()

window = pygame.Window(size=window_size, opengl=True)

ctx = zengl.context()

image = ctx.image(window_size, "rgba8unorm", samples=4)

pipeline = ctx.pipeline(
    vertex_shader="""
        #version 330 core

        out vec3 v_color;

        vec2 vertices[3] = vec2[](
            vec2(0.0, 0.8),
            vec2(-0.6, -0.8),
            vec2(0.6, -0.8)
        );

        vec3 colors[3] = vec3[](
            vec3(1.0, 0.0, 0.0),
            vec3(0.0, 1.0, 0.0),
            vec3(0.0, 0.0, 1.0)
        );

        void main() {
            gl_Position = vec4(vertices[gl_VertexID], 0.0, 1.0);
            v_color = colors[gl_VertexID];
        }
    """,
    fragment_shader="""
        #version 330 core

        in vec3 v_color;

        layout (location = 0) out vec4 out_color;

        void main() {
            out_color = vec4(v_color, 1.0);
            out_color.rgb = pow(out_color.rgb, vec3(1.0 / 2.2));
        }
    """,
    framebuffer=[image],
    topology="triangles",
    vertex_count=3,
)

clock = pygame.Clock()

while True:
    for event in pygame.event.get():
        if event.type == pygame.QUIT:
            pygame.quit()
            quit()

    ctx.new_frame()
    image.clear()
    pipeline.render()
    image.blit()
    ctx.end_frame()

    window.flip()
    clock.tick(60)

Adapted from the zengl repository. I'm also thaking @tank-king for being there to help me out (and pointing me to this example)

@oddbookworm oddbookworm marked this pull request as ready for review January 18, 2024 04:01
@oddbookworm
Copy link
Member Author

oddbookworm commented Jan 18, 2024

Sorry about the seemingly random deletions of trailing whitespace, I forgot I enabled "trim trailing whitespace on file save" in my editor 😬

@pmp-p
Copy link
Member

pmp-p commented Jan 25, 2024

Since the parameter was not usefull before, it is still time to make it - really - usefull and be able to re-use a provided glcontext ( int instead of boolean ).

This would cover for offscreen egl context, android native surfaces or anything that requires external setup.

This is actually a problem for embedding pygame on android java app or an html page with a pre-allocated canvas with no selector.

@Starbuck5
Copy link
Member

@pmp-p The parameter was not usefull before... on pygame.display? It seemed useful enough to everybody who was using it to do opengl rendering in their games.

As far as I understand this gets Window up to speed with how pygame.display supports opengl, which seems like a good thing.

It sounds like what you want should be another keyword argument into the Window, where you can optionally provide an existing glcontext.

@pmp-p
Copy link
Member

pmp-p commented Jan 25, 2024

but each of them has their own window management as far as I know, so why would they use pygame-ce then? I'm not too familiar with opengl backend stuff though, so I probably missed/borked something here

in case of opengl>0 the third parties GL handlers don't just deal with windows ( the native ones, as in EGL(mobile or headless) there's no native window stuff just a context pointer ) but more importantly with rendering contexts and their dozens of parameters.

As i see it right now opengl=True is only there to say : surface is not in CPU ram you cannot do software render on it without a frambebuffer object and uploading it to GPU later ( probably setting SDL_WINDOW_OPENGL on the sdl window id )

The only context that would make sense to me to create would be an Offscreen EGL buffer but it seems even that already requires more than 5 params.

And from_display_module() allows you to get away from that terrible GL/EGL init stuff by assuming it has already been done ( but apparently in a Window manager / OS fashion, not stricly a GPU way).

So my idea to use the int value of opengl param sounds a lot like https://wiki.libsdl.org/SDL2/SDL_CreateWindowFrom
that would bypass from_display_module() completely BUT it would allow handling EGL offscreen correctly ie

window = pygame.Window(size=window_size, opengl=True)

ctx = zengl.context()

would become

# create a context, anywhere with anything and pass int value  in context_pointer

window = pygame.Window(size=window_size, opengl=context_pointer)

having opengl non zero would imply using some GPU hardware acceleration.

Btw i'm not even sure "opengl" is a good key name there it could just be "context" or "renderer". After all opengl and egl are presumably dead.

@robertpfeiffer
Copy link
Contributor

@pmp-p This is probably something you can fudge by providing your own glcontext module, but it's also not something we have to solve in this PR. It would be nice to have, but a lot more work than this.

src_c/window.c Show resolved Hide resolved
@damusss
Copy link
Member

damusss commented Apr 25, 2024

I left long messages on the #general channel on discord so I will resume them as a comment here, After testing this branch I found an important inconsistency between the display and window API, where the window API is not updating the OpenGL viewport when user resizing (_resize_event_watch, line 214) and possibly also code resizing (window_set_size). For consistency I suggest implementing it, even if the python fix is simple (but not obvious).

P.S.(for clarity): as stated in #contributing this among other event-watch related improvements should be added to another pull request after this one is merged so that the bigger issue goes away (you can fix the viewport with python)

@robertpfeiffer
Copy link
Contributor

@ankith26 re: #2830

@ankith26
Copy link
Member

Personally I'm fine with this PR being merged on the next approval, but I'm not confident in opengl stuff to be giving a proper review myself.

I have marked this PR for 2.5.0 milestone

@ankith26 ankith26 added this to the 2.5.0 milestone May 19, 2024
@MyreMylar MyreMylar closed this May 23, 2024
@MyreMylar MyreMylar reopened this May 23, 2024
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.

Well this works. I don't have any amazing insights on an improved API here that haven't already been debated (subclasses or different functions for GL Swap versus the Software Flip).

One minor thing - we should also document in the window flip() docs that it will update the window from the back buffer if the window has an associated open GL context. See the display docs version of flip() for an example.

@oddbookworm oddbookworm merged commit 235b675 into pygame-community:main May 25, 2024
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
opengl window pygame.Window
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants