Beep6581 / RawTherapee

A powerful cross-platform raw photo processing program
https://rawtherapee.com
GNU General Public License v3.0
2.94k stars 327 forks source link

Scalable graphics - HiDPI #3547

Closed Beep6581 closed 5 years ago

Beep6581 commented 7 years ago

The aim of this issue is to get RawTherapee to look well when scaled up, something which is becoming increasingly common. The change should not cause a performance penalty.

Reading SVG icons is slow, but once they are read they will be drawn just as fast as PNG icons. As startup time does matter to us, we will use PNG icons. The icons will be stored as SVG in the repository, but will be converted to PNG for builds for performance reasons.

Steps:

https://wiki.gnome.org/HowDoI/HiDpi

Hombre57 commented 7 years ago

How long do you expect to support Gtk2 ?

Hombre57 commented 7 years ago

I definitely vote for create a single iconset file (one for Dark, one for Light... do we still want to support the Light theme btw ?)

Beep6581 commented 7 years ago

I discussed this with Ingo the other day. RT5 should have a GTK2 version, but after that I'll only invest time in merging as long as the time required is short - there are better things in life than supporting GTK2.

Basically we will decide at some point in the near future to commit all changes to the gtk3 branch first (we can rename master to gtk2 for clarity, we could also rename gtk3 to master though that is not a Git requirement and it could be confusing when looking at Git history), and then I could merge changes from gtk3 to gtk2 as long as the merge conflicts do not require much time to solve. Once we hit a merge conflict which would require much time to solve, I will cease merging. Of course anyone is free to take over.

Beep6581 commented 7 years ago

do we still want to support the Light theme btw ?

Considering all RT themes are dark and for good reason, I don't see a need to complicate things by shipping a light theme.

reagle commented 7 years ago

I updated to Rawtherapee 5.2 and the browser thumbnails are no longer 1/4 size as mentioned in #3936 . Thanks! Everything is much larger now, so I then configured the font sizes down and it looks great.

samdoshi commented 6 years ago

I've done a little bit of investigation on this.

For example the following diff applied to thumbbrowserentrybase.cc:

diff --git a/rtgui/thumbbrowserentrybase.cc b/rtgui/thumbbrowserentrybase.cc
index 2fff9590..1c221999 100644
--- a/rtgui/thumbbrowserentrybase.cc
+++ b/rtgui/thumbbrowserentrybase.cc
@@ -111,6 +111,8 @@ void ThumbBrowserEntryBase::updateBackBuffer ()
     }

     Cairo::RefPtr<Cairo::ImageSurface> surface = backBuffer->getSurface();
+    double device_scale = w->get_scale_factor();
+    cairo_surface_set_device_scale(surface->cobj(), device_scale, device_scale);

     bbSelected = selected;
     bbFramed = framed;

Will render the thumbnails in the thumbnail browser in HiDPI (e.g. GDK_SCALE=2 ./rawtherapee), with some major caveats:

Fundamentally these issues are related to a mismatch between application pixels and device pixels (e.g. at GDK_SCALE=2 each application pixel is 2 device pixels).

The difficulty is in deciding how to deal with this conflict between pixel types. It's possible that someone with enough knowledge of the codebase could fudge the numbers to get it to work properly, but I suspect that approach will lead to a lot of regressions unless all developers continuously test against different device scales.

Alternatively, you could encapsulate all the places that need to deal with the 2 types of pixels. I've only briefly looked over the source code, but perhaps adding a full blown GUI widget to display a BackBuffer and no longer rendering text directly onto it's surface, but using normal GTK widgets (afaik the navigator window does this...) might be the best long term approach.


For reference:

Beep6581 commented 6 years ago

https://standards.freedesktop.org/icon-theme-spec/icon-theme-spec-latest.html

Beep6581 commented 6 years ago

Once we have regular-scale and 2x-scale icons, how will RT pick the right set and the right theme on HiDPI screens?

Jacob-Bishop commented 6 years ago

Are there plans to deal with the fuzzy image aspect of this (https://github.com/Beep6581/RawTherapee/issues/4367_ ? Are there any tests someone with a Hi-Dpi screen (me) could do to help?

Beep6581 commented 6 years ago

The icon work is mostly done. What is needed now is for someone to add code which loads the double-sized icons when a high PPI or scaling is detected.

The double-sized icons are not in the repo yet - I will generate them as soon as they're needed.

regs01 commented 6 years ago

Double size is good, but there is also 150% scale popular on 27'' 4K screens. Unless they are svg and automatically scaled up and down.

Edit: I'm not into GTK and C++. But quick look up shows it's not that difficult.

There seem to be CSS property -gtk-dpi. There is also function gtk_css_style_get_value that can be used to read it. And then it simple. Set the right folder in rtengine::setPath respective of dpi. 96 -> 16, 120 -> 20, 144 -> 24, 168 -> 28, 192 -> 32, 240 -> 40, 288 -> 48, 384 -> 64 etc

Sometimes devs using other naming, like 100, 125, 150, 175, 200, 250, 300, 400 etc. As 96 dpi iconset can contain some images larger than 16x16.

Beep6581 commented 6 years ago

https://wiki.gnome.org/HowDoI/HiDpi https://bugzilla.gnome.org/show_bug.cgi?id=768081

Beep6581 commented 6 years ago

Here is a summary of a conversation with GTK+ devs regarding HiDPI:

regs01 commented 6 years ago

It is definitely not expected. And speaking of GTK way of 2x only scaling it was highly criticized. Almost all of high dpi users consider it's a defect. Canonical was doing its own workaround with Unity. And not every GTK IDE accepts it either. Lazarus, for example, does fractional scaling.

Hombre57 commented 6 years ago

Is that a definition of "mess" ?

Anyway, I think I'll be able to correctly size our icons, but not the Adwaita ones. I need to know if it's possible to programmatically select the theme icon size, I'm still searching.

Hombre57 commented 6 years ago

Replying here to the comment made in issue #4803 : I know that using SVG straight into icons is more deprecated, unless you use them to build a cache full of png counterparts, for the desired resolution. That way, you don't need to provide 2 sizes.

I'm also assigning myself for the last list entry.

Hombre57 commented 6 years ago

From here :

Replace dpi and font size with the current settings. That’s all and I think you don’t need more than 15 min. to implement it.

.view:not(check):not(radio), image:not(check):not(radio), spinbutton button, cellview {
    -gtk-icon-transform: scale(calc(( 144/ 96 ) * ( 9 / 9 )));
}

@TooWaBoo Is it possible to declare a variable and set its value in plain CSS ? e.g. like this with the CSS standard.

Otherwise I'll add this change to the code in a separate branch, in order to merge it into dev before the HiDPI branch..

Hombre57 commented 6 years ago

Oh, and btw, I'll ignore Gtk's devs recommendation and force the screen's resolution and scale, so that we get ride of this mess around the Cairo scale factor painfully handled by GTK.

TooWaBoo commented 6 years ago

@Hombre57 GTK knows only color variables. Before you inject the css-style, you must replace dpi and font-size with the current settings. ad503b9809ff6596f7619abfcf7c66adc35ae4cc

btw. Scaling with css doesn't scale the physical size of the icons. If you use padding or margin on the image, it's calculated from the original unscaled icon. Thats why it looks good only with the TWB-theme.

gaaned92 commented 6 years ago

Anyway, I think I'll be able to correctly size our icons, but not the Adwaita ones

I don't know if it is relevant, but adwaita icons are only shiped in SVG in my recent windows buids (scalable).

Hombre57 commented 6 years ago

@TooWaBoo Is this change valid for the RawTherapee theme as well ?

TooWaBoo commented 6 years ago

@Hombre57 Which change do you mean?

Hombre57 commented 6 years ago

This one.

TooWaBoo commented 6 years ago

If you mean this btw. Scaling with css doesn't scale the physical size of the icons. If you use padding or margin on the image, it's calculated from the original unscaled icon. Thats why it looks good only with the TWB-theme. -> Yes, but it's not a cange, this is how GTK works. The TWB-theme is sizing everything.

Hombre57 commented 6 years ago

@TooWaBoo I meant, should I add this trick to RawTherapee.ccs too ?

.view:not(check):not(radio), image:not(check):not(radio), spinbutton button, cellview {
    -gtk-icon-transform: scale(calc(( 144/ 96 ) * ( 9 / 9 )));
}

(with the correct values, I know)

TooWaBoo commented 6 years ago

The RT-theme would look like this and needs to be adaptet. grafik

Hombre57 commented 6 years ago

@TooWaBoo At least it's done for your themes, in twb-autoscaled-icons branch. IT's soft, but that was to be expected. For something crisper, it will be with my branch, but maybe it's better than nothing for RT 5.5 ? (ping @Beep6581)

So no need to adapt RawTherapee's theme for now.

TooWaBoo commented 6 years ago

@Hombre57 Compiling in progress. 😀

Hombre57 commented 6 years ago

I'll be away for 5 days, so unfortunately I won't be able to commit any changes til then (just to let you know).

TooWaBoo commented 6 years ago

@Hombre57 So far it works great. Good job. 👍

Beep6581 commented 6 years ago

These changes will require extensive testing by the community before being merged, so please leave them for 5.6.

TooWaBoo commented 6 years ago

@Hombre57 It looks like you don't force GTK_SCALE to 1. At 192dpi I get a blurry image.

@Beep6581 I agree but waiting for 5.6 means another year without HiDPI support. Adding a checkbox to the preferences "HiDPI support On/Off (Beta)" could be a solution. grafik

Thanatomanic commented 6 years ago

Including a hardly tested feature in a release is tricky business. The beta option could be something to consider. That also makes it implicitly clear we like to have user feedback on the feature.

Beep6581 commented 6 years ago
  1. This is not HiDPI support per GTK+'s definition of HiDPI but an in-house solution at trying to get sharp icons.
  2. The branch makes invasive changes and requires extensive testing.
  3. We are 4 days away from a release candidate. Hombre is away for 5 days.
  4. 5.6 is not a year from now.
Hombre57 commented 6 years ago

@TooWaBoo Forcing GTK_SCALE to 1 will require testing on X.org/Wayland/putYourFlavorHere, MacOS and Windows. I'll add that on Saturday probably.

Hombre57 commented 6 years ago

@Thanatomanic @Floessie @agriggio and anyone who know Glib : do you know how to use a pointer in a Gtk::TreeModelColumn, e.g. Gtk::TreeModelColumn<MyClass*> icon1 ? This seem sooooooooo complex to use, that it makes me sooooooooo ANGRY :rage: :rage: :rage: :rage: .

Hombre57 commented 6 years ago

I finally removed all the custom CellRenderer BS. Now I have something that compile and is usable finally! I'll try to finish this next w.e.

Hombre57 commented 5 years ago

@TooWaBoo @Beep6581 @gaaned92 @Thanatomanic I've added SVG support for icons in commit https://github.com/Beep6581/RawTherapee/commit/81407cd663b1ca459b5137dee620626c48a736a4 . It handle a cache with this path structure :

options.cacheBaseDir/svg2png/dpiValue

where dpiValue will be e.g. 96, 144, 192, etc.. This dpi value is tweaked to include the selected font size vs 9 pt base font size, as requested by @TooWaBoo . Tested with Gtk3.24, but there's a special font size handling for Gtk <3.20, where the size is set in px per default it seem. This is untested.

I also tried to update the GUI once the user close the Preferences window, but without success :disappointed: , so a restart will be required.

I also had to disobey to the Gtk recommendation (as DT did) and force GDK_SCALE to 1 in every case, after reading and setting the scale value in a global variable (see main.cc). I'd rather recommend to use the RTScalable static function to get the scale, the dpi and the tweaked dpi.

This branch is not finish, there are some things to address, like line thickness in custom widgets, but the SVG part is at least completely done (bugs aside).

@TooWaBoo Your themes are definitely better at handling HiDPI, e.g. for buttons frame thickness and spacings. Would you mind adding your science in Rawtherapee theme for this kind of stuff (not the color scheme please) ?

I'm using librsvg to render the icons, so it's one more dependency added to the cmakelist. The good news is that I now know how to handle and render SVG files, and adding a watermark/signature tool should be doable.

CAVEATS

  1. The PNG files are created in the cache on first demand, but then it will stay there forever. If an SVG file evolve, the cache won't be updated because RT will find the PNG file and use it. If you think about a mechanism that makes life easier for users and devs, I'm listening, I don't see anything else than a Clear cache button in Preferences.
  2. The cursor are resized as well, by using the tweaked DPI value. It may appear big, but using only the DPI would make things more complex. Let's say that it's a good thing :angel:
Hombre57 commented 5 years ago

PS : There's bunch of Pango error/warning, don't know what's the problem here. I'll have a look later this week.

Hombre57 commented 5 years ago

@regs01

There seem to be CSS property -gtk-dpi. There is also function gtk_css_style_get_value that can be used to read it. And then it simple. Set the right folder in rtengine::setPath respective of dpi. 96 -> 16, 120 -> 20, 144 -> 24, 168 -> 28, 192 -> 32, 240 -> 40, 288 -> 48, 384 -> 64 etc

I'll explore this for Adwaita's theme icons.

TooWaBoo commented 5 years ago

@Hombre57 I can do some parts to change to em. Scrollbars, sliders, progressbar and borders are easy to do. Most spacing is also possible. All other stuff has to be done from scratch. If I would do the other stuff, it would end up in a TWB theme.😁 To make the RT-theme HiDPI aware there has to be sized everything which is not done yet. What I can offer is doing half of the work and give support if necessary.

Hombre57 commented 5 years ago

@TooWaBoo Hmm... I'll see if the other way round wouldn't be more interesting, i.e. forking your theme to go back to the Rawtherapee theme. No need to start now then.

Btw, the new mechanism allow us to customize the icon color if necessary.

TooWaBoo commented 5 years ago

What do you mean with "...the other way round"?

Why not cancel the RT-theme and make the TWB-themes the RT-theme. It's OK for me to remame it to RawTherapee.

TooWaBoo commented 5 years ago

The PNG files are created in the cache on first demand, but then it will stay there forever. If an SVG file evolve, the cache won't be updated because RT will find the PNG file and use it. If you think about a mechanism that makes life easier for users and devs, I'm listening, I don't see anything else than a Clear cache button in Preferences.

Clear cache on start if version has changed in "About this build".

Hombre57 commented 5 years ago

@TooWaBoo

What do you mean with "...the other way round"?

I mean

forking your theme to go back to the Rawtherapee theme

i.e. trying to reproduce the RawTherapee theme by tweaking yours.

Clear cache on start if version has changed in "About this build".

I'll explore this.

Hombre57 commented 5 years ago

Now tested on Linux Mint 18.1. For an unknown reason, some images doesn't scale, and they aren't even saved to HDD !?

[EDIT] Correction ; with a fresh install, everything works as expected :roll_eyes:

regs01 commented 5 years ago

@TooWaBoo

I can do some parts to change to em.

Wouldn't rem be better, as based on the base font size. It's pretty sad that GTK doesn't have scalable pixel unit, so would scale depend on DPI rather than current font size.

TooWaBoo commented 5 years ago

rem is based on the root font size and this is always 10pt (EDIT) in Windows.

Beep6581 commented 5 years ago

rem, or "root em", is resolved using the initial font size value, which is not quite the same as the CSS definition of rem.

The rem size depends on the system-wide GTK3 font setting which users can adjust themselves.

Hombre57 commented 5 years ago

@regs01 Could you point me to your source about -gtk-dpi ? This page doesn't mention it.