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

WX: Comprehensive HiDPI Patch #4068

Merged
merged 16 commits into from Oct 4, 2016
Merged

Conversation

EmptyChaos
Copy link
Contributor

@EmptyChaos EmptyChaos commented Jul 30, 2016

This is a comprehensive patch for DolphinWX that fixes every HiDPI problem I could find, and also fixes a few tangential segfaults and memory leaks I found along the way. It touches everything in DolphinWX so it's pretty big.
mainwindow150
[Blue circle: high-res asset. Red circle: Upscaled low-res asset (no high-res available)]

inputconfigdiag150

  • CFrame / CGameListCtrl
  • About Dolphin
  • ISOProperties / ARCodeEdit / PatchEdit
  • Memory Card Manager
  • FifoPlayer Dialog
  • Cheat Manager
  • Controller Config Dialog
  • Input Config Dialog
  • General Configuration Dialog
  • TAS Input
  • Video Configuration Dialog / Postprocess Shader Configuration Dialog
  • Debugger Tools
  • NetPlay
  • Clean build on all platforms

This does not include my other PRs (#4048, #4015). It has been tested on Windows and Linux but not OS X, it should work on OS X though.


This change is Reviewable

@JMC47
Copy link
Contributor

JMC47 commented Jul 30, 2016

Well this is a pleasant surprise. I don't have a HiDPI monitor to really mess with this on though.

@MayImilae
Copy link
Contributor

Oh, Wii banners are twice the resolution of GameCube banners, and that could be HiDPI. Looking at the image it doesn't seem like you got that... You do want to try it?

@EmptyChaos
Copy link
Contributor Author

@MaJoRoesch I didn't actually know that, but fortunately I just checked and the way I wrote the code is adaptive so it already takes advantage of that fact so I don't need to change anything.

@JMC47 You can manually increase the DPI of your screen in the resolution settings on Windows if you want to mess around with it.

@lioncash
Copy link
Member

Review status: 0 of 98 files reviewed at latest revision, 7 unresolved discussions.


Source/Core/DolphinWX/DolphinSlider.h, line 7 [r1] (raw file):

#pragma once

#include "wx/slider.h"
#include <wx/slider.h>

Source/Core/DolphinWX/Frame.cpp, line 479 [r1] (raw file):

  // Start debugging maximized (Must be after the window has been positioned)
  if (UseDebugger)
    this->Maximize(true);

this isn't necessary (as far as I can tell)


Source/Core/DolphinWX/Frame.h, line 122 [r1] (raw file):

  const CGameListCtrl* GetGameListCtrl() const;
  wxMenuBar* GetMenuBar() const override;
  const wxSize& GetToolbarBitmapSize() const  // Needed before the toolbar exists

The definition should be in the cpp file for consistency.


Source/Core/DolphinWX/FrameAui.cpp, line 975 [r1] (raw file):

                                         wxNO_BORDER;

  wxAuiNotebook* const nb =

You can just use auto* here, since it's obvious what the type is based on the right-hand side of the assignment.


Source/Core/DolphinWX/Main.h, line 23 [r1] (raw file):

  // Can be called from any thread
  bool IsActiveThreadsafe() const { return m_is_active_threadsafe.load(std::memory_order_relaxed); }

The definition should be in the cpp file for consistency.


Source/Core/DolphinWX/Cheats/CheatSearchTab.cpp, line 103 [r1] (raw file):

  // TODO: Implement between search
  wxArrayString filters;
  filters.Add(_("No Filter"));

The reason this was 'Unknown' was to signify an unknown initial value. Which is the same terminology Cheat Engine uses.


Source/Core/DolphinWX/Cheats/CheatSearchTab.cpp, line 303 [r1] (raw file):

  m_lview_search_results->SetItem(index, 2, buf);

  // FIXME: Is this actually useful? doubles are 64bits wide.

This isn't related to DPI changes; neither is the constexpr-ifying below. Please include these in a different PR if necessary.


Comments from Reviewable

@EmptyChaos EmptyChaos force-pushed the wx-hidpi branch 2 times, most recently from 7f1c249 to e7db1ac Compare July 30, 2016 03:53
@EmptyChaos
Copy link
Contributor Author

Review status: 0 of 98 files reviewed at latest revision, 7 unresolved discussions.


Source/Core/DolphinWX/DolphinSlider.h, line 7 [r1] (raw file):

Previously, lioncash (Mat M.) wrote…

#include <wx/slider.h>
Done.

Source/Core/DolphinWX/Frame.cpp, line 479 [r1] (raw file):

Previously, lioncash (Mat M.) wrote…

this isn't necessary (as far as I can tell)

The code was cut and pasted from the top of the function. Fixed.

Source/Core/DolphinWX/Frame.h, line 122 [r1] (raw file):

Previously, lioncash (Mat M.) wrote…

The definition should be in the cpp file for consistency.

Done.

Source/Core/DolphinWX/FrameAui.cpp, line 975 [r1] (raw file):

Previously, lioncash (Mat M.) wrote…

You can just use auto* here, since it's obvious what the type is based on the right-hand side of the assignment.

Done.

Source/Core/DolphinWX/Main.h, line 23 [r1] (raw file):

Previously, lioncash (Mat M.) wrote…

The definition should be in the cpp file for consistency.

Done.

Source/Core/DolphinWX/Cheats/CheatSearchTab.cpp, line 103 [r1] (raw file):

Previously, lioncash (Mat M.) wrote…

The reason this was 'Unknown' was to signify an unknown initial value. Which is the same terminology Cheat Engine uses.

I'm not sure this is good user design. I had no idea what that did until I read the code, and after reading the code I still have no idea why this option even exists. Reverted.

Source/Core/DolphinWX/Cheats/CheatSearchTab.cpp, line 303 [r1] (raw file):

Previously, lioncash (Mat M.) wrote…

This isn't related to DPI changes; neither is the constexpr-ifying below. Please include these in a different PR if necessary.

They were just incidental things I noticed while working on the surrounding code. Removed.

Comments from Reviewable

@ghost
Copy link

ghost commented Jul 30, 2016

@EmptyChaos @MaJoRoesch This might be a good time to add in 2x star images instead of continuing to deal with the low res assets. Do you know if those are Dolphin originals or from some icon pack?

@MayImilae
Copy link
Contributor

I have noooo idea where they came from. It's probably best to replace them.

I had some difficulty replacing them before, from the small size and complex shape, but I can try again!

@ghost
Copy link

ghost commented Jul 30, 2016

@MaJoRoesch I tried doing a reverse image search but I haven't been able to find a source for the stars. If the size is a problem we could probably make them taller although they would also need to take up more horizontal space as well if anyone cares about that.

@EmptyChaos Taking a closer look those game banners seems excessively blurry, even compared to the original 1x images. Do you have any control over the scaling used there or is that all just handled by wxWidgets?

@EmptyChaos EmptyChaos force-pushed the wx-hidpi branch 2 times, most recently from b1aa325 to 78c6690 Compare July 30, 2016 07:10
@EmptyChaos
Copy link
Contributor Author

@pringo The images are scaled up using Bicubic filtering (from wxWidgets). At 1x images are not scaled at all, the screenshot is taken at 150% DPI (144DPI). The available filters are bicubic, bilinear, box and point.

If you're using a low-DPI screen to look at the screenshot then it will seem worse than it is because your pixel density is too low, it's important to remember that the physical size of the image in inches/centimeters is the same at high-DPI as the current GUI is in inches/centimeters. i,e. the current images are 1 inch by 1/3 inch, that will remain constant at higher DPI settings.


Regarding the image assets, the code I wrote supports quarter fractionals so you can have xyz@1.5.png, xyz@1.25.png etc. The required image sizes for Windows are @1, @1.25, @1.5 and @2. I believe OS X requires @1, @2 and @3.

@ghost
Copy link

ghost commented Jul 30, 2016

@EmptyChaos Okay that might be my issue then. I only have a 1080p monitor.

@mbc07
Copy link
Contributor

mbc07 commented Jul 30, 2016

Tested on my laptop (15.6" @ 1080p, Windows scale settings at 125% -- 120 DPI) and while most of the overlapping buttons/controls were fixed, Dolphin still loads low-res bitmaps, making them blurry (except for Wii game banners):
capturar

@ghost
Copy link

ghost commented Jul 30, 2016

So Windows will only use the 2x assets if scaling is set to 200% or higher? That is a bit unfortunate. Is there no option to load the 2x asset and downscale for settings between 100% and 200%?

@EmptyChaos
Copy link
Contributor Author

Windows doesn't provide any specific support, I had to write all the relevant code so I can change it however is needed. The implementation I went with is to look for an exact match then search for a nearest match so if the DPI is 125% it will look for @1.25, @1.5 then @1 and scale the nearest match to the correct size. Currently Dolphin only has assets for @2 and @1 so for 100% and 125% it will use @1, for 150% and 200% it will use @2. [This is why I mentioned Windows will need @1.5 (and, ideally, @1.25) resources added]

The theory was that scaling from nearest would produce better results than always scaling down from a bigger size but I could revise the algorithm to more aggressively search for larger sizes so that it will always find the @2 resources. The results are still likely to be significantly worse than prepared assets though.

@ghost
Copy link

ghost commented Jul 31, 2016

I think it might be worth trying to load the ceiling of the scaling factor when available, so 1.25x, 1.5x -> 2x. 1x or less -> 1x, etc. Do you know how OS X works with non-integer scaling? I was under the impression that they don't use assets like 1.25x and it gets good results. I generally figured it just downscaled from the next highest version available.

Edit: Reading about it more it sounds like OS X when using fractional scaling renders the entire desktop at the next highest integer and then downscales the entire image to the display resolution setting.

@EmptyChaos
Copy link
Contributor Author

EmptyChaos commented Jul 31, 2016

I've altered the image search to scan upwards then fallback to smaller sizes if it fails. I've also implemented scale2x/EPX image scaling and applied it to the game banners.
mainwindow125
The banners are less blurry than before but there is artifacting if you look closely.

@ghost
Copy link

ghost commented Jul 31, 2016

@EmptyChaos I'm not sure I'm a fan of the results with scale2x. The banners looked blurry with bicubic but now they look a bit off although somewhat sharper. I'm not sure if this will be any better but would you be willing to try bilinear and see what the results look like with that? I think sharper and maybe somewhat pixely might be fine compromise.

@EmptyChaos
Copy link
Contributor Author

@pringo Here's a full set of combinations:
algorithms

@ghost
Copy link

ghost commented Jul 31, 2016

@EmptyChaos Thanks! Looking at them side by side I think I do like the bilinear results the best. Bicubic is too blurry and the Scale2x variants produce noticeable artifacts in some banners.

@EmptyChaos
Copy link
Contributor Author

I've switched the banner scaling to bilinear.
mainwindow150

@ghost
Copy link

ghost commented Jul 31, 2016

LGTM

@mbc07
Copy link
Contributor

mbc07 commented Jul 31, 2016

Retested and no more blurry bitmaps for the ones that have higher res available, LGTM too...
capturar

Portable flexible HiDPI image loading and other support for Windows/GTK/OSX.
And related ARCodeAddEdit/PatchAddEdit.

Change ISOFile to use wxImage instead of wxBitmap since bitmaps require
a screen context and banner images have a fixed resolution.
Required a partial rewrite of the image loading code because it was working in
unscaled wxBitmaps. Needed to make it produce wxImages and scale them instead.
Changed the Cheat Search tab to disable the scan buttons while there is
not a game running and enable when it starts. Also added double-click to
create code to the result list.
Minor appearance change to align wiimote and gamecube sections.
Slight redesign of Control Configuration sub-window since SL_LABELS can't be
used with DolphinSlider.
Resolved "TODO" for Texture Cache safety, added explanation message.
Resolved "TODO" for default description, no longer uses default text for sizing

Fixed a memory leak in PostProcessingConfigDiag where it was never freeing any
of the objects it allocated in its constructor.

Minor design change to PostProcessingConfigDiag to give padding around elements
consistent with the rest of Dolphin's user interface (5px).
Several refactors of GUI creation into separate functions where the
function was too large or intermixed different concerns making it hard
to modify.
Changes:
  - MemoryWindow was cleaned up and gives more feedback on searches.

Some bugs were fixed as well:
  - A complex bug that allowed tearing off tabs and opening multiple
    copies of a debug panel which lead to segfaults
  - Another segfault related to right-click menus on code/memory views
    when those tools were floating in their own window.
Setting a single icon at a single resolution doesn't scale well,
Windows requires a 16x16 icon for the window and a 32x32/48x48 for
the taskbar. Providing all icons produces less pixellated results at
HiDPI.
@MayImilae
Copy link
Contributor

@endrift I haven't tested it since I don't have a retina macbook. I could, by using my 1440p monitor and the hack I have enabled, but that's a bit of work.

@shuffle2
Copy link
Contributor

shuffle2 commented Oct 4, 2016

In windows it is just a setting. Isn't it the same on OS X?

@EmptyChaos
Copy link
Contributor Author

@shuffle2 That should be the DPI setting on Windows 10, yes.

@shuffle2
Copy link
Contributor

shuffle2 commented Oct 4, 2016

@EmptyChaos yea i'm just pointing out that @MaJoRoesch probably doesn't need a special screen to test.

@EmptyChaos
Copy link
Contributor Author

I think OS X has a "more space"/"bigger text" setting somewhere in the control panel that functions the same way.

@MayImilae
Copy link
Contributor

MayImilae commented Oct 4, 2016

@shuffle2 It is not a setting on MacOS. It uses a whitelist, and you have to use what is essentially a registry hack to allow scaling on 3rd party monitors. Even after the hack, it requires 1440p or above to enable scaling, hence having to use my monitor.

It would be really handy if I had a retina MacBook for testing things like this, but um, good luck convincing @delroth to get me one! 😜

@shuffle2
Copy link
Contributor

shuffle2 commented Oct 4, 2016

welp

@shuffle2 shuffle2 merged commit b8731eb into dolphin-emu:master Oct 4, 2016
@EmptyChaos EmptyChaos deleted the wx-hidpi branch October 4, 2016 03:48
@aldelaro5
Copy link
Member

aldelaro5 commented Oct 4, 2016

After applying this PR, I found 2 new issues about the debugger UI.

The first one is the memory view, it doesn't load the debugger font, evrything else loads it sucessfully.

The second issue is the spacing of the different parts of instructions. More precisely, there's 2 places where the spacing is not what it shoudl be.

When using any non fixed width font (tested with noto sans, serif and a few others such as arial, as long as it's not fixed width), the address and the opcode name don't have any spacing between each other. On monospace and consolas however, this doesn't seem to be an issue.

For comparision, here's with arial:
screenshot from 2016-10-04 02-07-09

and with consolas:
screenshot from 2016-10-04 02-07-33

The second place is between the symbol name and the branches lines, they actually overlap each other no matter the font:
screenshot from 2016-10-04 02-07-54

I have my UI scaling set to "normal" in my cinnamon settings and a text scaling facotr of 1.0.

@EmptyChaos
Copy link
Contributor Author

@aldelaro5 #4290

Copy link
Contributor

@shuffle2 shuffle2 left a comment

Choose a reason for hiding this comment

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

Noticed log config not saving...

m_LogManager = LogManager::GetInstance();
CreateGUIControls();
LoadSettings();
}

void LogConfigWindow::OnClose(wxCloseEvent& event)
{
SaveSettings();

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

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