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

Fix adaptive icon painting #5989

Merged
merged 1 commit into from
Jan 31, 2021
Merged

Conversation

smlu
Copy link
Contributor

@smlu smlu commented Jan 24, 2021

This PR fixes adaptive icon being drawn incorrectly on Windows. The change was introduced in PR #5851.
See 5851 for more info.

Note: Fix was tested only on Windows, if it causes problems on other platform I'll scope it to Windows platform only.

Type of change

  • ✅ Bug fix (non-breaking change that fixes an issue)

@phoerious
Copy link
Member

phoerious commented Jan 25, 2021

I wonder why the previous implementation didn't work. It does pretty much the same as the upstream code: https://code.woboq.org/qt5/qtbase/src/gui/image/qicon.cpp.html#_ZL31qt_effective_device_pixel_ratioP7QWindow
Maybe the painter didn't return the correct dpr.

@droidmonkey
Copy link
Member

Icons looks very sharp. It still does not fix the paperclip and clock icon showing up as filled in blocks on the entry list.

@smlu
Copy link
Contributor Author

smlu commented Jan 25, 2021

I wonder why the previous implementation didn't work. It does pretty much the same as the upstream code: https://code.woboq.org/qt5/qtbase/src/gui/image/qicon.cpp.html#_ZL31qt_effective_device_pixel_ratioP7QWindow
Maybe the painter didn't return the correct dpr.

I went on to investigate what is going on and here are some preliminarily results:
TLDR; Setting render hint SmoothPixmapTransform smooths the edges of rendered icon, therfore icon rendered via painter->drawPixmap(..., icon.pixmap(...)) looks sharp. Icon painted via QIcon::paint method is rendered at original size and not affected by DPI scale, whereas QIcon::pximap function returns pixelmap of size internally scaled by DPI factor.

1.) painter->device()->devicePixelRatioF() always returns 1.0. Calling qApp->devicePixelRatio() returns correct scaling factor for the screen. I don't know if this is adaptive for multi-monitor environment.
2.) But it looks like it doesn't matter if size is scaled prior to calling QIcon::pixmap because pixmap function determines this factor for returned QPixmap: qicon.cpp.html#891
3. Icon's pixelmap is then scaled via QIconLoaderEngine::virtual_hook#858-866. Example of what's going on here for message-close.svg icon at DPI=1.25:

  • original size is 20x20 but is scaled in qicon.cpp.html#891 to 25x25.
  • integerScale = qCeil(1.25) = 2
  • entryForSize looks for an entry of size 13x13 at scale 2. The first for loop is skipped because 2 info.entries have QIconDirInfo::scale = 1.
    And the second loop returns first entry with data: QIconDirInfo{maxSize=256,minSize=1,path="scalable/actions",scale=1,size=48,threshold=2,type=Scalable}
    svgIcon.d = 0x0
  • icon's pixelmap is then returned by ScalableEntry::pixmap -> QSvgIconEngine::pixmap at size 25x25

4.) setting render hint SmoothPixmapTransform smooths the edges of rendered icon and icon looks sharp. But icon rendered via QIcon::paint method still looks sharper at 225% scale setting.
5.) Icon painted via QIcon::paint -> QIconLoaderEngine::paint is not affected by DPI scale and is rendered at original size (20x20 for message-close.svg)

Additional note: Drawing icon via painter->drawPixmap(..., icon.pixmap(...)) renders sharp icon if rect is scaled by DPI factor and pixelmap to be rendered is generated with original rect.size(). In this case icon is drawn out of requested rectangle due to scaled rect, therefore not suitable for this fix.

dpr = qApp->devicePixelRatio();
QSize pixmapSize = rect.size() * dpr;
auto aa = QRect(0,0, pixmapSize.width(), pixmapSize.height());
painter->drawPixmap(aa, m_baseIcon.pixmap(rect.size(), mode, state));

@phoerious
Copy link
Member

painter->device()->devicePixelRatioF() always returns 1.0. Calling qApp->devicePixelRatio() returns correct scaling factor for the screen. I don't know if this is adaptive for multi-monitor environment.

That's what I suspected. Qt itself queries dpr from the window, not the painter.

How sharp it looks exactly is probably to do with whether you are scaling in full integer increments or in arbitrary floats.

@smlu
Copy link
Contributor Author

smlu commented Jan 26, 2021

I should had read the docs more carefully because it clearly states that if AA_UseHighDpiPixmaps is set, the returned pixelmap is larger then requested size. By knowing that, this issue can be also solved by dividing rect.size() by dpi ratio and resulting size is the requested pixelmap size. e.g:

double dpr = qApp->devicePixelRatio();
QSize pixmapSize = rect.size() / dpr;
painter->drawPixmap(rect, m_baseIcon.pixmap(pixmapSize, mode, state));

But using this solution renders ugly icons in QTreeView so I'd stick with PR solution.

@smlu
Copy link
Contributor Author

smlu commented Jan 26, 2021

Icons looks very sharp. It still does not fix the paperclip and clock icon showing up as filled in blocks on the entry list.

I investigated a bit why icons in QTreeView are not rendered properly when using Icon object, and It seems that the core problem could be the fact that the painter device is a QWidget in this case which usually doesn't support alpha channel. Also icon is rendered via QIcon::paint and AdaptiveIconEngine::pixmap method is not invoked at all.
I've also tried different composition modes and if I set composition mode to CompositionMode_Source i get this:
img
I tried to set transparency mode, transparent background to EntryView and it's viewport but it didn't help.
Also, I've tried to draw the icon to the QImage object first and then render the image via painter and it didn't make any difference.

@smlu
Copy link
Contributor Author

smlu commented Jan 26, 2021

Btw there is a small bug on line DatabaseOpenWidget.cpp#L74 and DatabaseOpenWidget.cpp#L76. Calling pixmap on returned QIcon doesn't initialize AdaptiveIconEngine for the set icon, therefore changing theme mode doesn't affect the icon color.

@phoerious
Copy link
Member

Yes, pixmaps are larger than requested when scaling is on, which is exactly the behaviour I was going after with the original implementation. Be careful with what is scaled where. I believe when pixmap scaling is off, they are still being scaled when drawn, but with NN scaling, which looks crisp. With pixmap scaling on, you can get smoother icons, but if they are not the right size, they will still be interpolated and look fuzzy instead.

@smlu
Copy link
Contributor Author

smlu commented Jan 26, 2021

I'd also guess that incorrect icon size has to be the reason why icons are not rendered sharp in some cases.
Probably, this could be also the reason why I'm getting fuzzy icons in QTreeView rows, but the header icons are rendered ok when using last method.
img

@phoerious phoerious merged commit c7323ac into keepassxreboot:develop Jan 31, 2021
@phoerious phoerious added pr: bugfix Pull request that fixes a bug and removed bug labels Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: bugfix Pull request that fixes a bug user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants