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

Adding SDL_GetWindowSizeInPixels for window size in pixels #6112

Merged
merged 15 commits into from
Aug 24, 2022

Conversation

NoelFB
Copy link
Contributor

@NoelFB NoelFB commented Aug 23, 2022

Description

Adds SDL_GetWindowDrawableSize to get the Window size in pixels, much like SDL_GL_GetDrawableSize, SDL_Metal_GetDrawableSize, and so on. As of 2.24 Windows can match other platforms window scaling with SDL_HINT_WINDOWS_DPI_SCALING but this means that there's no longer a clean way inside of SDL to get the window size in pixels regardless of renderer.

This pull request is a work in progress as I only implemented the function for Windows, Emscripten, and Wayland. Otherwise it falls back to SDL_GetWindowSize.

I'm also not sure about the naming. I used the same name as the rendering functions (SDL_GL_GetDrawableSize) however it feels like since this is renderer-agnostic, SDL_GetWindowSizeInPixels or something might be better?

Existing Issue(s)

#6105

@slouken
Copy link
Collaborator

slouken commented Aug 23, 2022

I was going to suggest maybe we expose the window DPI as a separate function and let the application do the math, but the Wayland implementation isn't that simple.

@icculus, what do you think about this PR? #6105 has more context.

@flibitijibibo
Copy link
Collaborator

GetWindowSizeInPixels actually lines up with what I had in mind for a display size function, so maybe that would work: #6038 (comment)

@NoelFB
Copy link
Contributor Author

NoelFB commented Aug 23, 2022

Yeah I think I like the idea of calling it SDL_GetWindowSizeInPixels which would also match your suggestion for a SDL_GetDisplaySizeInPixels ... will do a pass on some stuff shortly.

Renames SDL_GetWindowDrawableSize to SDL_GetWindowSizeInPixels
Adds a filter variable to the main API function so that driver implementations don't need to worry about width or height pointers being NULL
@NoelFB NoelFB changed the title Adding SDL_GetWindowDrawableSize for window size in pixels Adding SDL_GetWindowSizeInPixels for window size in pixels Aug 23, 2022
@slouken slouken added this to the 2.26.0 milestone Aug 23, 2022
@NoelFB
Copy link
Contributor Author

NoelFB commented Aug 23, 2022

This is maybe more of a conversation for SDL3, but it feels like all of the SDL_[RENDERER]_GetDrawableSize could be collapsed into this single SDL_GetWindowSizeInPixels. So far the implementations I've seen share the same code across each one (ex. SDL_GL_GetDrawableSize = SDL_Vulkan_GetDrawableSize = SDL_GetWindowSizeInPixels). But there may be exceptions as I haven't looked very thoroughly yet.

@slouken
Copy link
Collaborator

slouken commented Aug 23, 2022

This is maybe more of a conversation for SDL3, but it feels like all of the SDL_[RENDERER]_GetDrawableSize could be collapsed into this single SDL_GetWindowSizeInPixels. So far the implementations I've seen share the same code across each one (ex. SDL_GL_GetDrawableSize = SDL_Vulkan_GetDrawableSize = SDL_GetWindowSizeInPixels). But there may be exceptions as I haven't looked very thoroughly yet.

Let's investigate that. If that really is the case, we can collapse them all to call your new function and add it to the API.

Copy link
Collaborator

@flibitijibibo flibitijibibo left a comment

Choose a reason for hiding this comment

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

Just a pile of formatting issues, other than that this lgtm (and is a pretty big cleanup!).

The change definitely had me thinking: Should we deprecate the GL/VK drawable size functions to get people onto this instead? It cuts out a pretty annoying branch we've had to cover up in FNA3D... the only part I'm unsure of is if Cocoa/UIKit would care about this.

src/video/emscripten/SDL_emscriptenvideo.c Outdated Show resolved Hide resolved
src/video/wayland/SDL_waylandwindow.c Outdated Show resolved Hide resolved
src/video/windows/SDL_windowsopengl.c Outdated Show resolved Hide resolved
src/video/windows/SDL_windowsopengles.c Outdated Show resolved Hide resolved
@NoelFB
Copy link
Contributor Author

NoelFB commented Aug 23, 2022

The change definitely had me thinking: Should we deprecate the GL/VK drawable size functions to get people onto this instead? It cuts out a pretty annoying branch we've had to cover up in FNA3D... the only part I'm unsure of is if Cocoa/UIKit would care about this.

Yeah I'm not sure about Metal/Cocoa/UIKit either. Looking at the Cocoa driver, the Metal_GetDrawableSize falls back to the hidpi viewport if no metal context exists, and Cocoa's Vulkan_GetDrawableSize actually just already redirects to Metal_GetDrawableSize which means the whole thing can probably be collapsed into a single function that checks for a Metal context and uses that, or falls back to the viewport code if not.

I don't have a way to test Cocoa/macOS very easily right now but I can still implement those changes.

@slime73
Copy link
Contributor

slime73 commented Aug 23, 2022

Minor naming question: should the name for pixel unit functions be SizeInPixels, SizePixels, or PixelSize (or something else)? Personally I kind of prefer the latter, e.g. SDL_GetWindowPixelSize, SDL_GetDisplayPixelSize, etc.

It's not a big deal, but this seems like a naming convention that might be around for a long time either way, so it's worth thinking about.

@slime73
Copy link
Contributor

slime73 commented Aug 23, 2022

the whole thing can probably be collapsed into a single function that checks for a Metal context and uses that, or falls back to the viewport code if not.

Looking at the existing code I think that'll probably work.

@NoelFB
Copy link
Contributor Author

NoelFB commented Aug 23, 2022

Minor naming question: should the name for pixel unit functions be SizeInPixels, SizePixels, or PixelSize (or something else)? Personally I kind of prefer the latter, e.g. SDL_GetWindowPixelSize, SDL_GetDisplayPixelSize, etc.

One thing I like about SizeInPixels is it's more specific about the dimensions of the window in pixel units. Without reading the comments PixelSize feels like it could return how big the hidpi pixels are which is inaccurate (ex. 1, 1.5, 2).

the whole thing can probably be collapsed into a single function that checks for a Metal context and uses that, or falls back to the viewport code if not.

Looking at the existing code I think that'll probably work.

I'll go ahead and implement that into Cocoa/UIKit shortly then. It seems like checking for a Metal context and then falling back to the hidpi viewport like Cocoa_Metal_GetDrawableSize already does is safe to do.

@slouken slouken requested a review from icculus August 23, 2022 21:13
Note: I'm not sure if Cocoa_GetWindowSizeInPixels should check for the Metal context itself and do what Cocoa_Metal_GetDrawableSize does, or if it should be kept as-is where it only returns the viewport size.
@slouken
Copy link
Collaborator

slouken commented Aug 23, 2022

It seems like SDL_GL_GetDrawableSize() can just call SDL_GetWindowSizeInPixels() if there's no custom implementation, and then the custom implementations (wayland, emscripten, etc.) can just go away. The same for SDL_Vulkan_GetDrawableSize() and SDL_Metal_GetDrawableSize()

@slouken
Copy link
Collaborator

slouken commented Aug 23, 2022

Internally, if there are no more custom implementations of the GLES/Vulkan/Metal functions, we can remove those from the video driver function tables.

BTW, I'm really liking this change. :)

@NoelFB
Copy link
Contributor Author

NoelFB commented Aug 23, 2022

Internally, if there are no more custom implementations of the GLES/Vulkan/Metal functions, we can remove those from the video driver function tables.

So for a lot of them they do all seem to be the same (windows, wayland, cocoa, emscripten, etc), but one exception I'm currently seeing is UIKit, which has different implementations for Metal vs GLES. One option could be to do what the Cocoa_Metal_GetDrawableSize function does, which is check for a context and do that if it exists, otherwise fallback to the viewport, ex:

if (metal_context) { /* get metal context size*/ }
else if (opengl_context) { /* get opengl context size*/ }
else { /* fallback to cocoa/uikit normal viewport value */ }

but I'm not sure if that's OK to do ... I don't have much experience with the Cocoa/UIKit side of things.

BTW, I'm really liking this change. :)

Yay :D

@NoelFB
Copy link
Contributor Author

NoelFB commented Aug 24, 2022

Hm yeah both Cocoa and UIKit have some custom code, which means you'd end up with something like this which doesn't seem totally ideal. Not sure what best practices are here!

void
Cocoa_GetWindowSizeInPixels(_THIS, SDL_Window * window, int *w, int *h)
{ @autoreleasepool
{
    SDL_WindowData *data = (__bridge SDL_WindowData *)window->driverdata;
    NSView *contentView = data.sdlContentView;
    NSRect viewport = [contentView bounds];

    /* Use Metal size if it exists */
#if (SDL_VIDEO_VULKAN || SDL_VIDEO_METAL)
    SDL_cocoametalview* metalview = [contentView viewWithTag:SDL_METALVIEW_TAG];
    if (metalview) {
        CAMetalLayer *layer = (CAMetalLayer*)metalview.layer;
        SDL_assert(layer != NULL);
        *w = layer.drawableSize.width;
        *h = layer.drawableSize.height;
        return;
    }
#endif

    /* Fallback to viewport size */

    if (window->flags & SDL_WINDOW_ALLOW_HIGHDPI) {
        /* This gives us the correct viewport for a Retina-enabled view. */
        viewport = [contentView convertRectToBacking:viewport];
    }

    *w = viewport.size.width;
    *h = viewport.size.height;
}}
void
UIKit_GetWindowSizeInPixels(_THIS, SDL_Window * window, int * w, int * h)
{@autoreleasepool
{
    SDL_WindowData *data = (__bridge SDL_WindowData *)window->driverdata;
    SDL_uikitview *view = (SDL_uikitview*)data.uiwindow.rootViewController.view;

#if SDL_VIDEO_VULKAN || SDL_VIDEO_METAL
    SDL_uikitmetalview* metalview = [view viewWithTag:SDL_METALVIEW_TAG];
    if (metalview) {
        CAMetalLayer *layer = (CAMetalLayer*)metalview.layer;
        assert(layer != NULL);
        *w = layer.drawableSize.width;
        *h = layer.drawableSize.height;
        return;
    }
#endif

#if SDL_VIDEO_OPENGL_ES || SDL_VIDEO_OPENGL_ES2
    if ([view isKindOfClass:[SDL_uikitopenglview class]]) {
        SDL_uikitopenglview *glview = (SDL_uikitopenglview *) view;
        *w = glview.backingWidth;
        *h = glview.backingHeight;
        return;
    }
#endif
        
    SDL_GetWindowSize(window, w, h);
}}

@slouken
Copy link
Collaborator

slouken commented Aug 24, 2022

So those functions shouldn't be folded into GetWindowSizeInPixels(), they're specifically looking at the drawable in the window, which for custom windows might be a layer, not the whole window. I think you probably want to leave those in the corresponding Metal/GL get drawable size calls and leave them split out.

@NoelFB
Copy link
Contributor Author

NoelFB commented Aug 24, 2022

So those functions shouldn't be folded into GetWindowSizeInPixels(), they're specifically looking at the drawable in the window, which for custom windows might be a layer, not the whole window. I think you probably want to leave those in the corresponding Metal/GL get drawable size calls and leave them split out.

Ah OK! I misunderstood this:

Internally, if there are no more custom implementations of the GLES/Vulkan/Metal functions, we can remove those from the video driver function tables.

To mean removing them from the SDL_VideoDevice struct, instead of just from their respective assignments in each device.

What you said makes sense though. Will work on that shortly.

NoelFB added 2 commits August 23, 2022 17:47
SDL_[RENDERER]_GetDrawableSize falls back to SDL_GetWindowSizeInPixels, which in turn falls back to SDL_GetWindowSize
A lot of video driver GetDrawableSize are no longer needed as they now automatically fall into SDL_GetWindowSizeInPixels
@flibitijibibo
Copy link
Collaborator

Latest all still makes sense. The only question I have: Do we care that UIKit is the odd one out, in that every other platform can use this and UIKit only works after you create a context? Should we update the docs of GetDrawableSize to specify that these functions only work after creating a render context, and should we make it clear that GetWindowSizeInPixels doesn't work on UIKit (and why)? I'm mostly trying to think of how a client would implement this, not knowing what we know about the internals.

@slime73
Copy link
Contributor

slime73 commented Aug 24, 2022

I could take a closer look into how to best approach this in the UIKit backend (without accidentally breaking any timings or existing behaviour), once this is merged.

@flibitijibibo
Copy link
Collaborator

For example's sake, here's what FNA3D would be doing with the new function:

diff --git a/src/FNA3D.c b/src/FNA3D.c
index a5240e0..2f9992f 100644
--- a/src/FNA3D.c
+++ b/src/FNA3D.c
@@ -171,13 +171,21 @@ uint32_t FNA3D_PrepareWindowAttributes(void)
 
 FNA3DAPI void FNA3D_GetDrawableSize(void* window, int32_t *w, int32_t *h)
 {
-	if (selectedDriver < 0)
+	SDL_Window *handle = (SDL_Window*) window;
+#ifdef __APPLE__
+	/* Metal likes to think it's special and requires a view to be made
+	 * before you can query the drawable size.
+	 *
+	 * Don't look at me, I just work here.
+	 * -flibit
+	 */
+	if (SDL_GetWindowFlags(handle) & (SDL_WINDOW_METAL | SDL_WINDOW_VULKAN))
 	{
-		FNA3D_LogError("Call FNA3D_PrepareWindowAttributes first!");
+		SDL_Metal_GetDrawableSize(handle, w, h);
 		return;
 	}
-
-	drivers[selectedDriver]->GetDrawableSize(window, w, h);
+#endif
+	SDL_GetWindowSizeInPixels(handle, w, h);
 }
 
 /* Init/Quit */
diff --git a/src/FNA3D_Driver.h b/src/FNA3D_Driver.h
index 70bbc43..c0f13e2 100644
--- a/src/FNA3D_Driver.h
+++ b/src/FNA3D_Driver.h
@@ -811,7 +811,6 @@ typedef struct FNA3D_Driver
 {
 	const char *Name;
 	uint8_t (*PrepareWindowAttributes)(uint32_t *flags);
-	void (*GetDrawableSize)(void* window, int32_t *w, int32_t *h);
 	FNA3D_Device* (*CreateDevice)(
 		FNA3D_PresentationParameters *presentationParameters,
 		uint8_t debugMode
diff --git a/src/FNA3D_Driver_D3D11.c b/src/FNA3D_Driver_D3D11.c
index 87aa2cf..2e12bba 100644
--- a/src/FNA3D_Driver_D3D11.c
+++ b/src/FNA3D_Driver_D3D11.c
@@ -1022,8 +1022,6 @@ static void D3D11_GetTextureData2D(
 	int32_t dataLength
 );
 
-static void D3D11_GetDrawableSize(void *window, int32_t *w, int32_t *h);
-
 static void* D3D11_PLATFORM_LoadD3D11();
 static void D3D11_PLATFORM_UnloadD3D11(void* module);
 static PFN_D3D11_CREATE_DEVICE D3D11_PLATFORM_GetCreateDeviceFunc(void* module);
@@ -1502,8 +1500,8 @@ static void D3D11_SwapBuffers(
 	if (renderer->backbuffer->type == BACKBUFFER_TYPE_D3D11)
 	{
 		/* Determine the regions to present */
-		D3D11_GetDrawableSize(
-			overrideWindowHandle,
+		SDL_GetWindowSizeInPixels(
+			(SDL_Window*) overrideWindowHandle,
 			&drawableWidth,
 			&drawableHeight
 		);
@@ -2622,7 +2620,7 @@ static void D3D11_INTERNAL_CreateBackbuffer(
 	if (!useFauxBackbuffer)
 	{
 		int32_t drawX, drawY;
-		D3D11_GetDrawableSize(
+		SDL_GetWindowSizeInPixels(
 			(SDL_Window*) parameters->deviceWindowHandle,
 			&drawX,
 			&drawY
@@ -4906,15 +4904,6 @@ static uint8_t D3D11_PrepareWindowAttributes(uint32_t *flags)
 	return 1;
 }
 
-static void D3D11_GetDrawableSize(void* window, int32_t *w, int32_t *h)
-{
-#ifdef FNA3D_DXVK_NATIVE
-	SDL_Vulkan_GetDrawableSize((SDL_Window*) window, w, h);
-#else
-	SDL_GetWindowSize((SDL_Window*) window, w, h);
-#endif /* FNA3D_DXVK_NATIVE */
-}
-
 static void D3D11_INTERNAL_InitializeFauxBackbufferResources(
 	D3D11Renderer *renderer,
 	uint8_t scaleNearest
@@ -5711,7 +5700,6 @@ static HRESULT D3D11_PLATFORM_ResizeSwapChain(
 FNA3D_Driver D3D11Driver = {
 	"D3D11",
 	D3D11_PrepareWindowAttributes,
-	D3D11_GetDrawableSize,
 	D3D11_CreateDevice
 };
 
diff --git a/src/FNA3D_Driver_OpenGL.c b/src/FNA3D_Driver_OpenGL.c
index 1aef9eb..fbc53fe 100644
--- a/src/FNA3D_Driver_OpenGL.c
+++ b/src/FNA3D_Driver_OpenGL.c
@@ -1329,7 +1329,7 @@ static void OPENGL_SwapBuffers(
 		{
 			dstX = 0;
 			dstY = 0;
-			SDL_GL_GetDrawableSize(
+			SDL_GetWindowSizeInPixels(
 				(SDL_Window*) overrideWindowHandle,
 				&dstW,
 				&dstH
@@ -2848,7 +2848,7 @@ static void OPENGL_INTERNAL_CreateBackbuffer(
 ) {
 	int32_t useFauxBackbuffer;
 	int32_t drawX, drawY;
-	SDL_GL_GetDrawableSize(
+	SDL_GetWindowSizeInPixels(
 		(SDL_Window*) parameters->deviceWindowHandle,
 		&drawX,
 		&drawY
@@ -5797,22 +5797,6 @@ static uint8_t OPENGL_PrepareWindowAttributes(uint32_t *flags)
 	return 1;
 }
 
-void OPENGL_GetDrawableSize(void* window, int32_t *w, int32_t *h)
-{
-	/* When using OpenGL, iOS and tvOS require an active GL context to get
-	 * the drawable size of the screen.
-	 */
-#if defined(__IPHONEOS__) || defined(__TVOS__)
-	SDL_GLContext tempContext = SDL_GL_CreateContext(window);
-#endif
-
-	SDL_GL_GetDrawableSize((SDL_Window*) window, w, h);
-
-#if defined(__IPHONEOS__) || defined(__TVOS__)
-	SDL_GL_DeleteContext(tempContext);
-#endif
-}
-
 FNA3D_Device* OPENGL_CreateDevice(
 	FNA3D_PresentationParameters *presentationParameters,
 	uint8_t debugMode
@@ -6178,7 +6162,6 @@ FNA3D_Device* OPENGL_CreateDevice(
 FNA3D_Driver OpenGLDriver = {
 	"OpenGL",
 	OPENGL_PrepareWindowAttributes,
-	OPENGL_GetDrawableSize,
 	OPENGL_CreateDevice
 };
 
diff --git a/src/FNA3D_Driver_Vulkan.c b/src/FNA3D_Driver_Vulkan.c
index a2a4e76..fa6c5fd 100644
--- a/src/FNA3D_Driver_Vulkan.c
+++ b/src/FNA3D_Driver_Vulkan.c
@@ -6527,6 +6527,7 @@ static CreateSwapchainResult VULKAN_INTERNAL_CreateSwapchain(
 		return CREATE_SWAPCHAIN_FAIL;
 	}
 
+	/* FIXME: Would like to replace this, but Apple -flibit */
 	SDL_Vulkan_GetDrawableSize(
 		(SDL_Window*) windowHandle,
 		&drawableWidth,
@@ -12053,11 +12054,6 @@ static uint8_t VULKAN_PrepareWindowAttributes(uint32_t *flags)
 	return result;
 }
 
-static void VULKAN_GetDrawableSize(void* window, int32_t *w, int32_t *h)
-{
-	SDL_Vulkan_GetDrawableSize((SDL_Window*) window, w, h);
-}
-
 static FNA3D_Device* VULKAN_CreateDevice(
 	FNA3D_PresentationParameters *presentationParameters,
 	uint8_t debugMode
@@ -12959,7 +12955,6 @@ static FNA3D_Device* VULKAN_CreateDevice(
 FNA3D_Driver VulkanDriver = {
 	"Vulkan",
 	VULKAN_PrepareWindowAttributes,
-	VULKAN_GetDrawableSize,
 	VULKAN_CreateDevice
 };

Much cleaner, minus the Apple bits. (Also ignore our GL_GetDrawableSize, our iOS/tvOS support hasn't included OpenGL for a long while...)

@NoelFB
Copy link
Contributor Author

NoelFB commented Aug 24, 2022

Yeah, it feels like it would be ideal if SDL_GetWindowSizeInPixels on Apple + Metal could automatically do that, so all you ever need to call is just SDL_GetWindowSizeInPixels... Would it be safe to presume if you have a Metal context SDL_GetWindowSizeInPixels should return the size of that CAMetalLayer instead of the viewport size?

Ultimately I guess that would end up looking more like the thing where I initially merged all the metal stuff into the one function above (#6112 (comment))

@slouken slouken merged commit 00452e4 into libsdl-org:main Aug 24, 2022
@slouken
Copy link
Collaborator

slouken commented Aug 24, 2022

Okay, this is merged, thanks!

@slime73 is going to look at the Apple platforms for any needed cleanup / consolidation there.

@slouken
Copy link
Collaborator

slouken commented Aug 24, 2022

Yeah, it feels like it would be ideal if SDL_GetWindowSizeInPixels on Apple + Metal could automatically do that, so all you ever need to call is just SDL_GetWindowSizeInPixels... Would it be safe to presume if you have a Metal context SDL_GetWindowSizeInPixels should return the size of that CAMetalLayer instead of the viewport size?

Ultimately I guess that would end up looking more like the thing where I initially merged all the metal stuff into the one function above (#6112 (comment))

I think there's a difference between the new function, where we're calculating the pixel size of the window (and can use that to determine the DPI scaling) vs the *GetDrawableSize() functions where we're talking about the size of the backbuffer / layer being rendered in that window. It may be a subtle difference, but I don't know enough about the layer tag use case to know if that's safe to conflate or if we should keep it separate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants