[3.x] New: Detect display scaling, cleanup in X11 API #1324
Conversation
scaled and raw because it was calculating scaling incorrectly
swapping the values referenced for width and height between scaled and raw to fix scaling calculation issue on macOS
|
Values were the wrong way round, but now confirmed as working on Retina. |
|
Very nice addition to have! As usual the Linux backend is going to be quite a pain here right? No standard API to get the stuff you want. Maintaining the platform backends is worthwhile as we think about the future versions of OpenTK where we long-term want to be more independent of current third-party libraries like GLFW. |
|
Nope sadly not. Retina is the one I'm concerned about, as most / all new Macs are this. Mac users don't tend to be the most technical either, unlike Linux :p |
|
Some more minor cleaning in the X11 driver bases, in preparation for possibly trying to read the X-atom. The entire Linux backend however is somewhat of a mess, and I get the very strong suspicion that we could probably remove a bunch of it without doing any damage whatsoever. Last real update for most parts seems to have been ~2010. TLDR: I don't especially want to start a wholesale backend rewrite (not to mention that I'm hacking this together on the fly as I go along....) but at the same time, some of this needs to be dumped to make the thing more maintainable |
|
@NogginBops @varon A query: My instinct is to get rid of these as a total maintenence burden, but you might want to keep. |
…nt resolution, not initial load resolution (#2) * screen scaling value is now a dynamic value * scalefactor of a displaydevice class is now from the displaydevice class * fix issue where sdl2 displaydevice initalizer accidentally had a vector2 input
|
Bumping this--I was the other person who helped make the PR. Are there any updates on this PR or implementing this? |
Ideally this should be handled on a case by case basis. Some comments/code can be useful even if they are commented out as they contain some kind of knowledge that will be relevant to anyone touching the code. Other times they are just fluff and should be removed. Is there some way to get a more proper X11 implementation for this PR or is that a huge workload? |
|
Added some basic X11 DPI detection and a bit of further junk removal. This is how GLFW originally did it ( glfw/glfw@3b070ea ), along with polling the X11 resources. We don't currently have any code accessing the X11 resources, but this should work as a basic first step. |
|
Fudge, typo.... Might as well check for zero physical w / h while I'm at itp |
Presumably it got rewritten at some stage in the past and this abandoned....
|
Dealt with. Have also removed some more duplicate cruft- Plenty more of that to go, but probably is going to want a new PR for major cleaning.... |
leezer3
changed the title
|
Added some more brackets and a comment. |
|
Just wondering--how far are we with code review? |
|
I haven't looked at this since the last review. |
Archived mailing list search finds that the (broken) linked bug refers to behaviour of Metacity circa 2002
|
Minor ping :) I've added XML-doc to essentially all the X11 function imports we're actually using (so much easier to see what's going on), and done some further marginal junk removal. None of this is really relevant to the original core PR, but makes things a lot more useful in there for future changes & actually understanding what's happening. Haven't really touched any serious cleanup / re-writes at the minute, as noted above that's really a future PR at some stage. |
| @@ -148,7 +146,6 @@ internal sealed class X11GLNative : NativeWindowBase | |||
| private readonly IntPtr EmptyCursor; | |||
|
|
|||
| #pragma warning disable 414 // Field assigned but never used, we do that on purpose | |||
| private readonly bool xi2_supported; | |||
There was a problem hiding this comment.
Why is this field removed and not the others here? To me it would only make sense to keep the all or remove them all.
There was a problem hiding this comment.
We actually use the version number itself here:
opentk/src/OpenTK/Platform/X11/X11GLNative.cs
Line 1017 in 3cfc0ac
As the opcode is technically part of the version number string, keeping it seemed logical too. Left the pragma there to suppress the fact we don't use it.
Could probably re-hack slightly to use the underlying variable this refers to inside the X11 mouse / keyboard driver, but that's heading towards the re-write.
There was a problem hiding this comment.
I still think we could keep this field, it's unnecessary to remove it.
If we are really unlucky someone uses reflection to read the value of this variable.
|
Ping @NogginBops anything else you want? We've also just discovered what I think is an unrelated issue caused by X11 DnD in our fork, which I'm going to try and look into this week. |
|
Bump. One further small bugfix in the SDL2 backend, but nothing else of interest. |
|
Ping @NogginBops |
|
Another ping @NogginBops |
| public abstract bool TryChangeResolution (DisplayDevice device, DisplayResolution resolution); | ||
| public abstract bool TryRestoreResolution (DisplayDevice device); | ||
| public abstract Vector2 GetDisplayScaling (DisplayIndex displayIndex); | ||
|
|
||
| // Gets the DisplayDevice that corresponds to the specified index. | ||
| public virtual DisplayDevice GetDisplay(DisplayIndex index) | ||
| public virtual DisplayDevice GetDisplay (DisplayIndex index) |
There was a problem hiding this comment.
As a small style nitpick we tend to not have spaces between the method name and the parens.
| public override Vector2 GetDisplayScaling (DisplayIndex displayIndex) | ||
| { | ||
| return new Vector2 (1, 1); | ||
| } |
There was a problem hiding this comment.
Why do we always return (1, 1) here?
(also the space between the method name and parens 🙂 )
There was a problem hiding this comment.
Not implemented in this specific backend at the minute. (2x native Linux backends....)
There was a problem hiding this comment.
Are there plans to implement it in this backend?
| //Copy the current display mode and take a pointer to it | ||
| IntPtr displayMode = CG.CopyDisplayMode(currentDisplay); | ||
| //Pull out the scaled width and height | ||
| int scaledWidth = (int)CG.GetModeWidth(displayMode); | ||
| int scaledHeight = (int)CG.GetModeHeight(displayMode); | ||
| //Pull out the raw width and height | ||
| int rawWidth = (int)CG.GetModePixelWidth(displayMode); | ||
| int rawHeight = (int)CG.GetModePixelHeight(displayMode); | ||
| //Remember to release the display mode | ||
| CG.ReleaseDisplayMode(displayMode); | ||
|
|
||
| float pixelScaleW = (float)rawWidth / scaledWidth; | ||
| float pixelScaleH = (float)rawHeight / scaledHeight; |
There was a problem hiding this comment.
pixelScaleW and pixelScaleH are never used in this code? I'm guessing this is something that never got removed?
| else | ||
| { | ||
| return XCreateWindow(display, parent, x, y, width, height, border_width, depth, | ||
| (int)@class, visual, (IntPtr)valuemask, null); | ||
| } | ||
|
|
||
| return XCreateWindow(display, parent, x, y, width, height, border_width, depth, | ||
| (int)@class, visual, (IntPtr)valuemask, null); |
There was a problem hiding this comment.
I would like to keep this else, attributes.HasValue has two outcomes and it not really a guard clause.
| else if (vsync_mesa_supported) | ||
| if (vsync_mesa_supported) | ||
| { | ||
| return Glx.Mesa.GetSwapInterval(); | ||
| } | ||
| else if (vsync_sgi_supported) | ||
| if (vsync_sgi_supported) | ||
| { | ||
| return sgi_swap_interval; | ||
| } |
There was a problem hiding this comment.
Why should this no longer be an else-if chain?
There was a problem hiding this comment.
Somewhat depends on the style you want here, both are exactly the same in practice.
This way is IMHO cleaner (and is what ReSharper prefers)
Thoughts FWIW:
You first should check for the XExtension version of vsync, as this is the newest / most common method of doing things.
Checking for the mesa vysnc support is a reasonable fallback.
SGI_SWAPINTERVAL is mega old, and quite often broken, and should be ignored unless we have anything else.
There was a problem hiding this comment.
I think this should still be an else-if chain to make clear that two of these should not be true at the same time. I would be confused by this code if it was written without the else-if chaining.
| else if (e.ClientMessageEvent.message_type == _atom_xdnd_leave) | ||
| { | ||
| break; | ||
| } |
There was a problem hiding this comment.
I'll have to go back and check, but IIRC it can never be true from here.
| @@ -30,5 +30,6 @@ internal interface IDisplayDeviceDriver | |||
| bool TryChangeResolution(DisplayDevice device, DisplayResolution resolution); | |||
| bool TryRestoreResolution(DisplayDevice device); | |||
| DisplayDevice GetDisplay(DisplayIndex displayIndex); | |||
| Vector2 GetDisplayScaling (DisplayIndex displayIndex); | |||
There was a problem hiding this comment.
Nitpick: Space between method name and paren.
| { | ||
| return null; | ||
| } | ||
| return AvailableDevices [(int)index]; |
There was a problem hiding this comment.
Nitpick: Space between name and parens.
| float pixelScaleD, pixelScaleW, pixelScaleH; | ||
| //Get display DPI from SDL | ||
| SDL.GetDisplayDPI(d, out pixelScaleD, out pixelScaleW, out pixelScaleH); | ||
| //Convert to scale factor using default 96DPI | ||
| pixelScaleD /= 96.0f; | ||
| pixelScaleW /= 96.0f; | ||
| pixelScaleH /= 96.0f; |
There was a problem hiding this comment.
pixelScaleD, pixelScaleW, and pixelScaleH are never used here, guessing this is a mistake?
Experimental, not handled by Win or Mac yet...
Purpose of this PR
This PR adds detection of the current display scaling factor to OpenTK 3.x
The Windows, SDL2 and OS-X backends have been implemented.
It adds a single Vector2 to the DisplayDevice properties, containing the X / Y scaling factors of the display. What it doesn't do is to attempt to do anything with this- It's left for the developer to handle appropriately as they desire.
The Linux backend at the minute hasn't got any DPI / scale factor detection. From some digging, this (as seems usual) can be found in any one of 3-4 places, depending on the DE, WM etc.
It also does a considerable amount of cleanup and commenting in the X11 API.
Testing status
Comments
I'm ambivilant as to whether the backend should silently handle the scaling as 4.x does.
As 3.x is largely EOL however, this seems to be a simple quality of life addition, and allows those of us using 3.x to handle this better than at the minute.
#47