Add PlatformBuilder API for requesting backend features #6510

ianhattendorf · 2024-10-10T18:48:30Z

For now, allow the API consumer to request an OpenGL API (OpenGL or OpenGL ES) and a backend (Skia, femtovg, or software).

This is a rough draft:

Currently only implemented for winit.
I'm not a fan of how I threaded OpenGLApi everywhere (especially in functions that have nothing to do with OpenGL like SoftwareSurface::new() but share a trait with OpenGL functions), but I wanted to get a rough initial implementation up to start the conversation.
- Specifically, it feels awkward passing it through WinitCompatibleRenderer::resume but that seemed to be the common point between Skia and femtovg where the OpenGL context is created.

Related discussion: #6482

ChangeLog: Add PlatformBuilder API for requesting backend features

tronical

Looks good overall - thank you!

A few comments inside - I'm wondering if it can be simplified a tiny little bit. The support for the Qt backend is something I could add afterwards.

internal/core/api.rs

tronical · 2024-10-11T15:12:23Z

internal/backends/winit/renderer/femtovg/glcontext.rs

+                    major: 3,
+                    minor: 2,


Since this changes the default from 2.0 to GLES 3.2, perhaps it would be better to have two GLES variants in the OpenGL API enum - or something like Glutin uses: https://docs.rs/glutin/latest/glutin/context/enum.ContextApi.html#

I'm not sure why I updated this, possibly a testing change that made it into a commit? I intended to keep the default GLES version here, as it appears to use GLES 3.2 for me when this is set to major: 2 anyway. I'll revert the default version change.

I do think it would be useful to be able to request a specific version, and considered adding that but wanted to get feedback first. I was thinking of the Glutin approach as well, I'll go with that.

Pushed up a commit adding an APIVersion field to the OpenGLAPI items.

tronical · 2024-10-11T15:16:07Z

internal/backends/selector/lib.rs

+    }
+
+    /// Configures this builder to use the specified OpenGL API when building the platform later.
+    pub fn with_opengl_api(mut self, opengl_api: OpenGLApi) -> Self {


In the future I'd like to add a GraphicsAPI kind of enum that covers Vulkan, OpenGL, Metal, etc. and we could have a with_graphics_api(...) function in this builder as well. Using with_opengl_api would then imply a call to with_graphics_api(OpenGL). This is just a thought for the future, regardless of this PR :)

Yes, "how far do I go" was something I asked myself multiple times while looking into this 🙂. I figured at least before getting feedback I would keep it simple, but I agree that being able to request specific graphics APIs/features would be ideal.

Something like:

struct ApiVersion { major: u8, minor: u8 } enum OpenGLApi { GL(Option<ApiVersion>), GLES(Option<ApiVersion>) } enum GraphicsApi { OpenGL(OpenGLApi), Vulkan(VulkanApi) }

or maybe it makes sense to have something like

struct OpenGLOptions { version: Option<ApiVersion> } struct OpenGLESOptions { version: Option<ApiVersion> } enum OpenGLApi { GL(Option<OpenGLOptions>), GLES(Option<OpenGLESOptions>) }

for selecting version + more to allow more flexibility down the road 🤔

(It looks like there's already a GraphicsAPI enum for WebGL/OpenGL selection, maybe a slightly different name would be better to avoid confusion.)

I don't mind going with this approach for this PR, or we can leave it as-is for now to keep it simpler. Let me know your preference.

I agree, I think it’s okay to leave it as-is for now. I think it fits well with a future extension.

tronical · 2024-10-11T15:18:33Z

internal/backends/winit/winitwindowadapter.rs

@@ -244,6 +244,7 @@ pub struct WinitWindowAdapter {
    fullscreen: Cell<bool>,

    pub(crate) renderer: Box<dyn WinitCompatibleRenderer>,
+    opengl_api: Option<OpenGLApi>,


Instead of storing that here, maybe it's easier to pass it through to the renderer constructors and leave it there, instead of carrying it around here. Eventually the thing we pass through could be a large option anyway that also has the overall graphics API, not just the GL version. But in scope of this PR, would you be willing to try and see if passing to new_suspended() is easier? If it's not easier, then we can go with this way :)

Sure, I briefly looked into this but had issues with areas like this and this expecting new*_suspended to not take any arguments.

I would definitely prefer this approach as well though, I'll think on it a little more.

You’re right. The parameter you’re adding is similar to the window attributes, and it’s fine to keep them in sync and pass them around the same way.

ianhattendorf · 2024-10-12T05:55:12Z

internal/core/api.rs

@@ -279,6 +279,26 @@ impl<'a> core::fmt::Debug for GraphicsAPI<'a> {
    }
 }

+/// This enum specifies which OpenGL API should be used.
+#[derive(Debug, Clone, PartialEq)]
+pub enum OpenGLApi {


Should this be OpenGLAPI? I noticed this repo tends to prefer capitalizing initialisms, but then it's hard to read if there are two next to each other (what's a "GLAPI"?).

Yes, I think it should be API. We might revisit this before the next release as we do another API (🤣) review on the changes since the previous release.

Force pushed a reword to API along with my other changes.

ogoffart · 2024-10-14T10:07:18Z

I think the function to select the graphics api makes sense.
I wonder what's the use case to select the renderer itself with a function though. Is it not tnough to select it through the feature flags?

ianhattendorf · 2024-10-15T00:37:37Z

I wonder what's the use case to select the renderer itself with a function though. Is it not tnough to select it through the feature flags?

PlatformBuilder somehow needs to select a renderer. It can use the default, but in some cases I might want to dynamically select a renderer (based on the user's machine, a feature flag, troubleshooting, etc.). IMO that's easiest if PlatformBuilder exposes a simple override instead of having to set an environment variable to influence the default.

ianhattendorf · 2024-10-17T01:46:25Z

I still need to feature flag the winit portion of PlatformBuilder and indicate that it currently only supports winit, but I think the rest should be good to go.

ianhattendorf · 2024-10-17T01:48:26Z

internal/backends/selector/lib.rs

+    }
+
+    /// Configures this builder to use the specified renderer when building the platform later.
+    pub fn with_renderer(mut self, renderer: SlintRenderer) -> Self {


One issue with this approach: it's currently winit specific. Ideas here? Leave it for a future PR after Qt/KMS support is added?

I think it's okay to keep it.

ianhattendorf · 2024-10-18T01:21:25Z

I still need to feature flag the winit portion of PlatformBuilder and indicate that it currently only supports winit, but I think the rest should be good to go.

PlatformBuilder::build() impl is now feature flagged. Also force pushed a correction to the first commit for metal/vulkan/d3d Surface::new trait impl.

Right now this just allows selecting between OpenGL and OpenGL ES.

If we're explicitly requesting a platform with specific properties, we should fail to allow the caller to react accordingly.

Co-authored-by: Simon Hausmann <hausmann@gmail.com>

tronical reviewed Oct 11, 2024

View reviewed changes

ianhattendorf commented Oct 12, 2024

View reviewed changes

ianhattendorf force-pushed the opengl-api-non-gles branch from 15f3f43 to 0bbb6fc Compare October 17, 2024 01:42

ianhattendorf commented Oct 17, 2024

View reviewed changes

ianhattendorf force-pushed the opengl-api-non-gles branch from 0bbb6fc to c354325 Compare October 18, 2024 01:19

ianhattendorf force-pushed the opengl-api-non-gles branch 2 times, most recently from a02cd87 to b2dff51 Compare October 18, 2024 04:46

ianhattendorf and others added 6 commits October 18, 2024 11:23

Add PlatformBuilder API to allow selecting OpenGL API

a5d7da0

Right now this just allows selecting between OpenGL and OpenGL ES.

Allow choosing renderer via PlatformBuilder

e30fc9d

Don't allow fallback in PlatformBuilder

5310bf0

If we're explicitly requesting a platform with specific properties, we should fail to allow the caller to react accordingly.

Add version to OpenGLAPI

1bd36e9

Fix wording in SlintRenderer comment

f44b9b8

Co-authored-by: Simon Hausmann <hausmann@gmail.com>

PlatformBuilder::build() impl requires winit feature

e8c0d97

ianhattendorf force-pushed the opengl-api-non-gles branch from b2dff51 to e8c0d97 Compare October 18, 2024 18:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PlatformBuilder API for requesting backend features #6510

Add PlatformBuilder API for requesting backend features #6510

ianhattendorf commented Oct 10, 2024 •

edited

Loading

tronical left a comment

tronical Oct 11, 2024

ianhattendorf Oct 12, 2024

ianhattendorf Oct 17, 2024

tronical Oct 11, 2024

ianhattendorf Oct 12, 2024

tronical Oct 12, 2024

tronical Oct 11, 2024

ianhattendorf Oct 12, 2024

tronical Oct 12, 2024

ianhattendorf Oct 12, 2024

tronical Oct 12, 2024

ianhattendorf Oct 17, 2024

ogoffart commented Oct 14, 2024

ianhattendorf commented Oct 15, 2024

ianhattendorf commented Oct 17, 2024

ianhattendorf Oct 17, 2024

tronical Oct 18, 2024

ianhattendorf commented Oct 18, 2024

Add PlatformBuilder API for requesting backend features #6510

Are you sure you want to change the base?

Add PlatformBuilder API for requesting backend features #6510

Conversation

ianhattendorf commented Oct 10, 2024 • edited Loading

tronical left a 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

Choose a reason for hiding this comment

ogoffart commented Oct 14, 2024

ianhattendorf commented Oct 15, 2024

ianhattendorf commented Oct 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianhattendorf commented Oct 18, 2024

ianhattendorf commented Oct 10, 2024 •

edited

Loading