-
-
Notifications
You must be signed in to change notification settings - Fork 50
Canvas/context inversion of control #764
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
base: main
Are you sure you want to change the base?
Conversation
|
Thanks @Korijn! Executive summary: I agree! I should start by saying that I share your feeling that the context feels somewhat awkward. Although for me the confusion lies more in how it ties Have you seen pygfx/rendercanvas#124? In short, it proposes that rendercanvas implements its own context class, and (only if necessary) wraps a The main reason why our To go in a bit more detail ... in JS, both the With that said, I think we can justify adopting a different API for wgpu-py. One that makes wgpu-py oblivious of any canvas, and that makes code outside of wgpu-py responsible for keeping the context up-to-date. That makes cases like A few more practical comments:
|
👍 Sweet!
It seems okay but it depends on the implementation details I think. RenderCanvas should be free to choose any API it wants to, because the wgpu-py API allows for it, without imposing requirements on the implementation details. Ideally wgpu-py just provides some public functions and a context object with public members, and you can work with that in any way you like.
I get that! 💯 In the case of libs like glfw, sdl2, imgui, there's no concept of a "widget" or "window" or "canvas" tired directly to an OOP model like there is in the browser or in PySide. I think we need to realize that the latter is a higher level abstraction than the former. We need to make sure our API supports the lower level style first (primary priority), so that the higher level style can be built on top (secondary priority).
Music to my ears. :)
I tried to implement this in this branch, but I think I broke the bitmap present method in the process. In any case it seems to work just fine for the screen present method! The gui_direct example still runs just fine, including resizing. 🎉
I guess this is where I got things mixed up while attempting to implement this. :) It needs some more untangling then I guess.
I implemented this as well! Is it correct that vsync cannot be changed during a reconfigure?
That would be appreciated! A thought I had while working on this; there's only a handful of spots left where |
Actually, we could, but the current API to set vsync on canvas instantiation together with update_mode and max_fps probably makes more sense.
Proposal:
|
For |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole file can be removed, as well as any references to WgpuCanvasInterface
wgpu/_classes.py
Outdated
| def set_physical_size(self, size): | ||
| """Set the current framebuffer physical size (width, height). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In rendercanvas we use (width, height) as arguments (instead of a tuple). Let's stay consistent with that.
wgpu/_classes.py
Outdated
| color_space: str = "srgb", | ||
| tone_mapping: structs.CanvasToneMappingStruct | None = None, | ||
| alpha_mode: enums.CanvasAlphaModeEnum = "opaque", | ||
| size: tuple[int, int] = (320, 240), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In retrospect, I think the size should only be set via set_physical_size, not via the config.
|
If your hands are itchy feel free to take over this branch... |
I know this branch is hideous and broken, but I wanted to put it up in draft anyway to communicate something that's bothered me about wgpu-py's API from the start. It's the hidden coupling between wgpu-py and the GUI layer. This self-referential class object thing just feels wrong. Below is an AI generated summary that I thought helps to illustrate my thoughts on this subject.
You can check out this branch and run the
gui_direct.pyexample to see that it works (including resizes).My main intention of opening this draft PR is to initiate a discussion with you @almarklein :)
In my opinion it's really not worth it to litter the codebase with class to get_context via a canvas weakref via a wrapper object, just to be able to call get_physical_size, and require all users of wgpu-py to work with this awkward class interface.
I strongly believe that implementing this decoupling API change will make wgpu-py much easier to apply in a wide variety of applications using libs such as glfw, sdl and so on.
Summary
Replace the implicit self-referential canvas/context wrapper with a plain, caller-owned GPUCanvasContext. The application is now responsible for window/canvas lifetime and must explicitly supply canvas-related state (for example, physical framebuffer size) to the context.
What changed
GPUCanvasContext no longer depends on being wrapped by a canvas object that calls back into itself.
Applications may construct a GPUCanvasContext directly and must call
context.set_physical_size((w, h))and update it on resizes.Example:
gui_direct.pycreates GPUCanvasContext directly and callsset_physical_size()from GLFW frame-buffer queries.Rationale
Example (conceptual)