-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Add function to query window content scale #5827
Conversation
src/video/SDL_video.c
Outdated
int width, height, drawWidth, drawHeight; | ||
|
||
SDL_GetWindowSize(window, &width, &height); | ||
SDL_GL_GetDrawableSize(window, &drawWidth, &drawHeight); |
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.
SDL_GL_GetDrawableSize
is only valid when the window has an OpenGL context associated with it, which might not always be the case (especially if an app is using Vulkan or Metal or SDL_Render, which have their own GetDrawableSize functions in SDL, or when an app hasn't created any rendering surface yet.)
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.
Well crap.
Would it make more sense then to try the appropriate functions based on initialized graphics, and if that fails return 1 as fallback value?
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.
Rathar than hacking up things in this new function, shouldn't there be a plain SDL_GetDrawableSize
that dispatches to the underlying renderer/ws, and maybe falls back to SDL_GetWindowSize
? (the GL version already does this fallback)
Though I guess there's probably a good reason for SDL_GL_GetDrawableSize
existing, and I just don't know what it is.
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.
SDL_GetDrawableSize that dispatches to the underlying renderer/ws
This might also get confusing to users if they call it after creating a highdpi-capable window but before attaching any graphics drawable/renderer to it, since it wouldn't return pixel units in that case.
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.
I've changed this fallback to use SDL_GetWindowSizeInPixels
instead, which I hope should be sufficient.
SDL does support fractional scaling on Wayland. This approach has some issues. You can't have fractional pixels, so in cases of odd window sizes, where multiplying the window size and scale factor doesn't result in an integer and rounding is involved, dividing the drawable size by the window size won't give you the 'true' desktop scale (e.g. with a window dimension of 497 and desktop scale of 1.25, the scale given by this method would either be 1.2515 or 1.2494 depending on how the backbuffer size was rounded off). Additionally, if a client tries to deduce the backbuffer size from the provided desktop scale, it will need to ensure that it uses the same rounding as the backend used when creating the backbuffer, or you could end up a pixel off (e.g. with a window size of 497 and scale of 1.5 you get 745.5, round down when the backend rounded up to calculate the backbuffer size and you've got problems). There's also no guarantee that all platforms employ the same rounding rules when performing these calculations, so something that's correct on Windows might be a pixel off on Wayland or vice versa. This seems like a lot of unnecessary, error-prone int->float->int work when a universal |
The GetDrawableSize APIs might still run into issues when they're used outside of the context of the window's exact pixel / dpi-scaled dimensions (and when those dimensions have been rounded), right? i.e. the discussion here #5778 (comment) Maybe something like macOS has these APIs, which do something similar to that Convert API: |
Having a universal
That issue would be fixed by the Wayland backend implementing the new My thinking is, this new API is to work around the fact SDL's window size/positions are |
Is this on fullscreen then? Because as far as I understand it Wayland still does not have a functional protocol for windowed fractional scaling.
Yeah, that's why I made this PR in the first place (since this was the previously recommended approach, despite the clearly obvious issues). See the linked PR comments. This current path is very much a fallback because I don't exactly have the ability to write implementations for platforms other than Win32.
Yes, it would.
They still should but that's a separate issue. Even if you make it float coords you'll still have this problem (but with float imprecision instead) which is "eh". At least with this new approach you'll get a consistent value for a specific OS scale factor. |
Fullscreen and windowed. The official fractional scaling protocol is still in the works, but as long as certain output information is exposed, it can be figured out, and Wayland supports viewports, so you can display any arbitrarily sized buffer in any arbitrarily sized window. It's the same way that Firefox currently does it and all of the major desktop environments support it. The proposed fractional scaling protocols works the same way as well, except the compositor tells the application the preferred scale vs having to figure it out ourselves, so support will be fairly trivial to add when it's finalized.
Adding |
I believe his ended up getting made redundant via SDL_GetWindowSizeInPixels; it's not exactly the same but you can now do the math by comparing against SDL_GetWindowSize. |
That's the workaround this PR aims to make obsolete. It's nice that it's easier now (don't need GL_ specific functions etc) but it still doesn't solve the problems I've previously mentioned in this thread and #5778 I should dust off this PR and finish it though. The new function should address the review comment I never got around to fixing. |
I think each API has separate use cases - The |
Cleaned this PR up, should be good now? |
Patch below fixes all those. Other than those: Please squash your commits into single The rest is up to maintainers. diff --git a/test/Makefile.in b/test/Makefile.in
index 52a671d..821a281 100644
--- a/test/Makefile.in
+++ b/test/Makefile.in
@@ -356,6 +356,9 @@ testmessage$(EXE): $(srcdir)/testmessage.c
testdisplayinfo$(EXE): $(srcdir)/testdisplayinfo.c
$(CC) -o $@ $^ $(CFLAGS) $(LIBS)
+testdpi$(EXE): $(srcdir)/testdpi.c
+ $(CC) -o $@ $^ $(CFLAGS) $(LIBS)
+
testqsort$(EXE): $(srcdir)/testqsort.c
$(CC) -o $@ $^ $(CFLAGS) $(LIBS)
diff --git a/test/testdpi.c b/test/testdpi.c
index 10aeda1..21ad3b1 100644
--- a/test/testdpi.c
+++ b/test/testdpi.c
@@ -70,13 +70,13 @@ loop(void)
float scale_h, scale_v;
int mouse_x, mouse_y;
int size_h, size_v;
- SDL_Rect mouse_rect = { 0 };
+ SDL_Rect mouse_rect;
if (!state->renderers[i]) {
continue;
}
- // Render a red square under the cursor, properly scaled of course.
+ /* Render a red square under the cursor, properly scaled of course. */
renderer = state->renderers[i];
window = state->windows[i];
diff --git a/test/watcom.mif b/test/watcom.mif
index 2189fd4..08cc775 100644
--- a/test/watcom.mif
+++ b/test/watcom.mif
@@ -7,7 +7,7 @@ LIBS = SDL2.lib SDL2test.lib testutils.lib
CFLAGS+= $(INCPATH)
-TARGETS = testatomic.exe testdisplayinfo.exe testbounds.exe testdraw2.exe &
+TARGETS = testatomic.exe testdisplayinfo.exe testdpi.exe testbounds.exe testdraw2.exe &
testdrawchessboard.exe testdropfile.exe testerror.exe testfile.exe &
testfilesystem.exe testgamecontroller.exe testgeometry.exe testgesture.exe &
testhittesting.exe testhotplug.exe testiconv.exe testime.exe testlocale.exe &
@@ -44,6 +44,7 @@ needs_audio = &
needs_display = &
testbounds.exe &
+ testdpi.exe &
testdisplayinfo.exe
TESTS = $(noninteractive) $(needs_audio) $(needs_display) |
include/SDL_video.h
Outdated
* like text to match the user's preferences | ||
* (usually based on DPI of the monitor). | ||
* | ||
* This is effectively a more convenient and precise version of doing |
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 comment isn't accurate - it's less precise if it's used for backbuffer-related things unless the same rounding is used in user code as what the OS (or SDL backend code in some cases) does. I think documentation saying that it's always better will be misleading.
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.
Good point
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.
New comment:
SDL_GetWindowSizeInPixels() divided by SDL_GetWindowSize() is approximately equivalent to these values, differing slightly due to rounding behaviors.
0da6642
to
d767431
Compare
@sezero done, and thanks for the help. I've left the DPI test program a separate commit, since I figure that's a clean way to keep them separate. |
f17bf86
to
85502f0
Compare
Analogous to glfwGetWindowContentScale(). See comments on libsdl-org#5778
Thanks to some help from @ sezero
85502f0
to
cae966f
Compare
The OpenWatcom tests are still failing for some reason, and I have no idea why:
I figured I'd make this |
FYI, SDL 3.0 now has SDL_GetDisplayContentScale() and SDL_GetWindowDisplayScale() and associated events when this changes. |
Description
Analogous to glfwGetWindowContentScale(). Gets you the raw intended DPI scale from the OS, without jumping through hoops by dividing drawable size by window size:
I only implemented a proper driver function on Windows, all other platforms guess by dividing drawable size by window size. At least on Wayland and macOS this should be exact since they only have integer scaling currently anyways.
Existing Issue(s)
See comments on #5778