Merge raw-gl-context into baseview to allow OpenGL contexts to be created during window creation #115
Conversation
This should fix the inability to create OpenGL contexts with alpha channels in raw-gl-context, but it seems like that still needs a bit more work.
The main change is that all of these types are simplified, there are more different OS-specific window handle types, and they are no longer gated behind the respective targets which makes the library a bit easier to use for applications.
Including @prokopyl's PR that adds more X11 error handling: glowcoil/raw-gl-context#15 Commit: prokopyl@raw-gl-context@98cd3cf1104ee254a67e3fed30e4e6b2ae2b6821 (with `cargo fmt`) Branch base: RustAudio/baseview@b68a05b
This only needed a couple changes for the raw-window-handle migration, and for the different module paths.
There are now three todo!()s when compiling with the OpenGL flag that need to be filled in, with the only nontrivial one being the X11 version.
This should in theory work! When requesting an OpenGL context, the window visual is determined based on the matched framebuffer config.
We're going to need this for the other platforms as well.
The things remaining are all of the cursor things in the X11 implementation (there's _a lot_ of it, so there's probably a reason why it's all still there but unused), and the super unsound immutable reference to mutable reference cast in the macOS implementation that Clippy automatically errors out on.
b4a3d2b missed some immutable borrows, but these borrows need to be local anyways because of the way the closing is implemented.
src/gl/x11/errors.rs
Outdated
| use std::sync::atomic::{AtomicPtr, Ordering}; | ||
| use std::sync::Mutex; | ||
|
|
||
| static CURRENT_ERROR_PTR: AtomicPtr<Mutex<Option<xlib::XErrorEvent>>> = |
There was a problem hiding this comment.
I left a comment on the raw-gl-context PR describing how this needs to be changed: glowcoil/raw-gl-context#15 (review)
There was a problem hiding this comment.
I noticed that the handler also throws away earlier errors if multiple errors occur during the handle() call. I think it might make more sense to at least hold on to the earliest error (since that's likely the cause of all other errors), but should the other errors be thrown away silently? Some other options would be to print to STDERR (probably not a very desirable solution) or to store multiple errors in a Vec<XErrorEvent> (but realistically you only care about either the first one or about none at all).
There was a problem hiding this comment.
handle lets the closure check the value of the most recent error at any time during its execution (via XErrorHandler::check), so I don't think this is actually an issue.
There was a problem hiding this comment.
I changed this to use a thread local variable instead of an atomic pointer and to also keep the first instead of the last error.
There was a problem hiding this comment.
Thanks! One last nitpick: can you change the Mutex to a Cell or RefCell now that it's a thread local?
There was a problem hiding this comment.
Ah yeah good one, changed it to a RefCell.
| @@ -357,10 +371,28 @@ struct WindowState { | |||
| handler: Box<dyn WindowHandler>, | |||
| scale_policy: WindowScalePolicy, | |||
| dw_style: u32, | |||
|
|
|||
| #[cfg(feature = "opengl")] | |||
| gl_context: Arc<Option<GlContext>>, | |||
There was a problem hiding this comment.
This shouldn't need to be Arc, right? Any reason it can't just live in WindowState?
There was a problem hiding this comment.
I guess it can - I just thought this was a bit cleaner. The alternative would be to replace this function:
With some nasty function that borrows from the RefCell (so you need to pray that the Win32 message handler isn't running at the same time or you get a panic) and then does some lifetime transmutations to extend the lifetime of the context beyond the RefCell borrow. So I went with an Arc instead.
There was a problem hiding this comment.
That's fair. I think the right way to do this would be to split up the fields of WindowState to use multiple Cells and RefCells, so e.g. the GlContext could be borrowed immutably while the WindowHandler is borrowed mutably, but this is fine for now.
So in the situation that multiple X11 clients are handling errors from different threads they don't see and interfere with each other's errors. As mentioned in glowcoil/raw-gl-context#15 (review).
OpenGL context creation is now handled by baseview instead of by raw-gl-context for the reasons outlined in RustAudio/baseview#115. This fixes issues on a variety of desktop Linux configurations. raw-window-handle and keyboard-types have also been updated to match those used by baseview.
OpenGL context creation is now handled by baseview instead of by raw-gl-context for the reasons outlined in RustAudio/baseview#115. This fixes issues on a variety of desktop Linux configurations. raw-window-handle and keyboard-types have also been updated to match those used by baseview.
|
Thanks a lot for the work on this. It fixed my long standing issues with context creation in HexoTK (which used baseview, raw_gl_context and femtovg)! |
OpenGL context creation is now handled by baseview instead of by raw-gl-context for the reasons outlined in RustAudio/baseview#115. This fixes issues on a variety of desktop Linux configurations. raw-window-handle and keyboard-types have also been updated to match those used by baseview.
As discussed on the Rust Audio Discord, this is necessary to be able to create an OpenGL context under X11. The OpenGL context configuration determines which framebuffer configuration should be used, but the framebuffer configuration in turn determines which visual should be used for the window the context will be used with. Because of that, it's not possible to create an OpenGL context for an X11 window after the fact. Because of that, the context creation from raw-gl-context's X11 module has now been split up into two parts: initializing the framebuffer config and determining a visual, and another one for actually creating the context after the window has been created. All of the other code from raw-gl-context is unchanged. The version of raw-gl-context used is from @prokopyl's PR adding more error checks on X11 (glowcoil/raw-gl-context#15).
This is all put behind a new
openglfeature that's disabled by default. The idea is that you can provide the OpenGL context options alongside the other window options, and if that's provided then callingwindow.gl_context()will return anOption<&GlContext>so you can access the OpenGL context in your window handler.I've also taken the liberty to fix a lot of warnings. The main things remaining are a lot of unused cursor functions in the X11 implementation, and the immutable reference to mutable reference cast in the macOS implementation.
I've only tested this on Linux, so I would very much appreciate it if someone on macOS or Windows could test those implementations! On Linux it solves glowcoil/raw-gl-context#2 for both NVIDIA drivers and Mesa, and for 0 and 8 bit alpha buffers. There's currently no example in the repo, but you can try this version of egui-baseview that's updated to work with these changes instead:
This supersedes #114.