internal: Remove the winit dependency from the renderers in the winit backend #1956

tronical · 2022-12-07T13:47:23Z

This prepares for the ability to move renderers into a separate crate and let them have their own API, for advanced embedding of Slint.

tronical · 2022-12-07T14:15:50Z

internal/backends/winit/glcontext.rs

-        window_builder: winit::window::WindowBuilder,
+        _window: &dyn raw_window_handle::HasRawWindowHandle,
+        _display: &dyn raw_window_handle::HasRawDisplayHandle,
+        _size: PhysicalSize,
        #[cfg(target_arch = "wasm32")] canvas_id: &str,


This is somewhat of a kludge. It would be better if RawWindowHandle would have a better way of identifying the canvas element. ALAS the int id in the web handle is local to winit and not unique in the DOM. But when a future version of raw-window-handle hopefully improves this, we can remove this.

Is there an upstream issue for that? Maybe rust-windowing/raw-window-handle#102 ?

Yes, that's the one I saw as well. Wasn't sure about creating a backlink heh, but that's done now.

But i think that could be good enough actually.
We can set the data-raw-handle to some unique value somewhere else. Possibly in our js specific API.
Anyway, this is unrelated to this change.

I don't think that we can set this value, unless we re-implement HasRawWindowHandle and return our own id. Winit creates an event-loop local integer id and sets the attribute. We can find the DOM element then - that's fine. But if you think of our future JS API then this might become an issue:

If somebody uses the JS API and wasm module for one HTML canvas and a second instance (and was module) for another, then both would share the same IDs.

We could of course decide to ignore that and say that there must be only one.

(And yes, agreed, this is unrelated :)

tronical · 2022-12-07T14:23:01Z

internal/backends/winit/lib.rs

+            window: &dyn raw_window_handle::HasRawWindowHandle,
+            display: &dyn raw_window_handle::HasRawDisplayHandle,


These could be combined into one impl as well, but I think for the public API of the renderer later we'll end up with dyn anyway - so I went for two dyn.

ogoffart · 2022-12-07T14:15:06Z

internal/backends/winit/glcontext.rs

-                    )
-                    .unwrap()
+            let window = crate::event_loop::with_window_target(|event_loop| {
+                window_builder.build(event_loop.event_loop_target()).unwrap()


I wonder if we really should unwrap() here or give a better, more actionable error message.
Is that not what people will see if there is a problem with the display or something?
Or is it the following unwrap.

(Ideally, we should have made our API so slint user would be able to catch that error)

Good point, this something to consider for the API. So before this PR we unwrapped and still do after this, but I agree that we should consider passing this to the caller in the public API.

ogoffart · 2022-12-07T14:23:06Z

internal/backends/winit/glcontext.rs

-        window_builder: winit::window::WindowBuilder,
+        _window: &dyn raw_window_handle::HasRawWindowHandle,
+        _display: &dyn raw_window_handle::HasRawDisplayHandle,
+        _size: PhysicalSize,
        #[cfg(target_arch = "wasm32")] canvas_id: &str,


Is there an upstream issue for that? Maybe rust-windowing/raw-window-handle#102 ?

ogoffart · 2022-12-07T14:27:34Z

internal/backends/winit/renderer/sw.rs

+        display: &dyn raw_window_handle::HasRawDisplayHandle,
+        size: PhysicalWindowSize,
+    ) -> Self::Canvas {
+        let opengl_context = crate::OpenGLContext::new_context(window, display, size);


Unrelated, but is there still no way to create a software context?

Do you mean a software OpenGL context?

I mean something onto which the software renderer could render without requiring femtovg

Ah no, this remains a missing piece / missing crate - unfortunately. That said, I think for improved compatibility on Windows I think it would make sense to switch that over to Skia.

tronical · 2022-12-07T15:02:24Z

Ok, on Windows this creates a window resize loop 🤪

tronical · 2022-12-07T15:53:31Z

Rebased to include commit e573f5b

ogoffart · 2022-12-08T09:19:39Z

internal/backends/winit/glwindow.rs

@@ -202,7 +202,9 @@ impl<Renderer: WinitCompatibleRenderer + 'static> WinitWindow for GLWindow<Rende

        self.pending_redraw.set(false);

-        self.renderer.render(&window.canvas);
+        let inner_size = window.window.inner_size();
+        let physical_size = PhysicalSize::new(inner_size.width, inner_size.height);


A bit sad that we lose type safety there. Looking at this snippet of code, i don't know that inner_size is indeed a physical size. Maybe the type could be made explicit. Or going through a conversion function that do the conversion in a type safe manner.

But feel free to ignore.

No no, you have a valid point! I can clean this up with a helper trait I think, that converts winit’s physical size to our physical. Then it’s safe and less LOC here.

ogoffart · 2022-12-08T09:26:26Z

internal/backends/winit/renderer/femtovg.rs

@@ -433,7 +431,6 @@ impl super::WinitCompatibleCanvas for FemtoVGCanvas {
    }

    fn resize_event(&mut self, size: PhysicalWindowSize) {
-        self.size = size;
        self.opengl_context.ensure_resized(size)


does that mean that the opengl_context might have the wrong size?

But it is really strange that we don't get the resize_event before a draw.

Maybe this is the queue of events:

draw (previous frame)

resize event

draw (current frame)

but when we process the draw event, the window already has been resized.

Just a theory, but a bit strange that window.inner_size() is already updated before we have time to draw, i thought it would be done in a single thread so events should be mixed like that. unless the call to inner_size() actually do a call to the display server.

does that mean that the opengl_context might have the wrong size?

Yes, using the last value from the resize event results in the OpenGL context having the wrong size.

But it is really strange that we don't get the resize_event before a draw.

Maybe this is the queue of events:

draw (previous frame)

resize event

draw (current frame)

but when we process the draw event, the window already has been resized.

Just a theory, but a bit strange that window.inner_size() is already updated before we have time to draw, i thought it would be done in a single thread so events should be mixed like that. unless the call to inner_size() actually do a call to the display server.

Yes, it's strange. And it's worse on wayland, see #1671 .

ogoffart · 2022-12-08T10:57:02Z

internal/backends/winit/glwindow.rs

@@ -34,17 +34,33 @@ fn position_to_winit(pos: &corelib::api::WindowPosition) -> winit::dpi::Position
    }
 }

-fn size_to_winit(pos: &corelib::api::WindowSize) -> winit::dpi::Size {


I'm not a fan of using trait for this kind of stuff where you just add one function to a type. I kind of prefer free floating function as before. I don't really know why though. But if you prefer trait, i'm ok with it.

Good question, in reverse I don't know why I prefer a trait when ... your suggestion is literally less lines of code. I'll squash some of the other changes and change this one to use a free standing function. Thanks for the nudge :)

…ument We can unwrap internally.

…w_handle

GLWindow (yes, misnomed) is now responsible to creating and owing the winit::window::Window.

Encapsulate the direct conversion between physical sizes in functions, so that the conversion remains type safe and the code remains easy to read.

tronical commented Dec 7, 2022

View reviewed changes

ogoffart approved these changes Dec 7, 2022

View reviewed changes

tronical force-pushed the simon/renderer-winit-split branch from d9582cb to 7602d58 Compare December 7, 2022 15:53

ogoffart approved these changes Dec 7, 2022

View reviewed changes

ogoffart approved these changes Dec 8, 2022

View reviewed changes

tronical added 6 commits December 8, 2022 11:58

Cleanup: simplify internal render() signature by removing adapter arg…

e8a24b2

…ument We can unwrap internally.

Remove glutin-winit dependency

43e2cbc

Implement windowing system name helper function by means of raw_windo…

a14fab1

…w_handle

Remove winit dependency from the renderers

be8490f

GLWindow (yes, misnomed) is now responsible to creating and owing the winit::window::Window.

Simplify size conversions between winit and Slint

53d2cc2

Encapsulate the direct conversion between physical sizes in functions, so that the conversion remains type safe and the code remains easy to read.

Rename window member in MappedWindow struct to winit_window

0c43c7b

tronical force-pushed the simon/renderer-winit-split branch from fc42f60 to 0c43c7b Compare December 8, 2022 11:11

tronical merged commit 7fb7804 into master Dec 8, 2022

tronical deleted the simon/renderer-winit-split branch December 8, 2022 11:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal: Remove the winit dependency from the renderers in the winit backend #1956

internal: Remove the winit dependency from the renderers in the winit backend #1956

tronical commented Dec 7, 2022

tronical Dec 7, 2022

ogoffart Dec 7, 2022

tronical Dec 7, 2022

ogoffart Dec 7, 2022

tronical Dec 7, 2022

tronical Dec 7, 2022

ogoffart Dec 7, 2022

tronical Dec 7, 2022

ogoffart Dec 7, 2022

ogoffart Dec 7, 2022

tronical Dec 7, 2022

ogoffart Dec 7, 2022

tronical Dec 7, 2022

tronical commented Dec 7, 2022

tronical commented Dec 7, 2022

ogoffart Dec 8, 2022

tronical Dec 8, 2022

ogoffart Dec 8, 2022

tronical Dec 8, 2022

ogoffart Dec 8, 2022

tronical Dec 8, 2022

		window: &dyn raw_window_handle::HasRawWindowHandle,
		display: &dyn raw_window_handle::HasRawDisplayHandle,

internal: Remove the winit dependency from the renderers in the winit backend #1956

internal: Remove the winit dependency from the renderers in the winit backend #1956

Conversation

tronical commented Dec 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tronical commented Dec 7, 2022

tronical commented Dec 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment