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

Switch pyglet to programmable pipeline #158

Merged
merged 3 commits into from
Jul 17, 2020

Conversation

matthewturk
Copy link
Contributor

Pyglet 2.0 in dev requires using the programmable pipeline, and pyglet 1.5 allows it. This lets the in-dev pyglet version work with the pyglet integration with no additional changes.

@coveralls
Copy link

coveralls commented Jul 16, 2020

Coverage Status

Coverage decreased (-0.8%) to 53.43% when pulling 1a744ea on matthewturk:pyglet_programmable into c84315e on swistakm:master.

@swistakm
Copy link
Member

Hi @matthewturk! Thanks for PR!

Unfortunately, I couldn't get that running with the built-in pyglet example on macOS. I suppose that default GL context created by Pyglet on macOS on my mac is still 2.1 and I haven't find a way to successfully create higher level context with recent Pyglet version.

As proposed change is quite small code-wise but has large backwards-incompatibility risk here is my suggestion:
Let's create that programmable pipeline integration as a separate integration class and provide some deprecation / future deprecation warnings to make current users aware of the future changes. That way we could get this merged sooner and maybe switch completely to programmable pipeline once we will have all the things working and battle tested on all supported OSes. I don't know pyglet very well so will leave proper naming and detailed decisions to you but here are some ideas to consider:

class PygletRenderer:
        # deprecation warning on instantiation telling user to use eaither
       # PygletFixedPipelineRenderer or PygletProgrammableRenderer

class PygletFixedPipelineRenderer:
        # future deprecation warning telling user that it won't work
        # past pyglet>=2.0, 
        # (optionally) maybe issue it starting from pyglet versions that actually support
        # programmable pipelines?
         
class PygletProgrammableRenderer:
       # new programmable pipeline renderer

# totally optional!
def create_renderer(context|window):
      # factory function that could match fixed or programmable pipeline renderer class
      # by introspecting either windows.context or bare context for GL context version
      # whatever would be most convenient for pyglet users. 

Of course if we allow for both integrations it would be great to have some working example in docs/examples and some documentation that better explains why we have these two classes. But this is of course optional.

@matthewturk
Copy link
Contributor Author

This is great -- thank you for the feedback and suggestions! I'll get on this.

@matthewturk
Copy link
Contributor Author

I think this version hits what you've suggested, @swistakm . I tried to implement some checks based on the OpenGL context, but after reading pyglet/pyglet#76 I came to the conclusion that it's safer to look at pyglet versions rather than the contexts. I've tested this with both the development pyglet and the stable pyglet and both work using create_renderer with the default config values for creating a window.

I wasn't sure where else to add things to the documentation, but I added a note about the difference here to the part that mentions pyglet.

self.on_mouse_scroll,
self.on_resize,
)
class PygletRenderer(PygletFixedPipelineRenderer):
Copy link
Member

Choose a reason for hiding this comment

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

👍

DeprecationWarning)
super(PygletRenderer, self).__init__(window, attach_callbacks)

def create_renderer(window, attach_callbacks=True):
Copy link
Member

Choose a reason for hiding this comment

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

👍

@swistakm
Copy link
Member

Great! I'm merging it :)

@swistakm swistakm added enhancement / feature integrations Anything regarding imgui.integrations subpackage release pending Merged but still needs official release labels Jul 17, 2020
@swistakm swistakm merged commit dec6ebc into pyimgui:master Jul 17, 2020
@swistakm
Copy link
Member

Feature released in 1.2.0

@swistakm swistakm removed the release pending Merged but still needs official release label Jul 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement / feature integrations Anything regarding imgui.integrations subpackage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants