[libbreezecommon] Add box shadow helper
ClosedPublic

Authored by zzag on Mar 10 2018, 12:08 AM.

Details

Summary

Box shadow helper is a helper which draws box shadows, similar to CSS box-shadow property. The only thing it's missing is the spread property but we don't need it pretty much.

Demo

Let's draw a box shadow for the rect QRect(300, 200, 200, 200) with the following params:

  • blur radius: 96
  • vertical offset: 50
  • color: black
Breeze::BoxShadowHelper::boxShadow(
    &painter,
    QRect(300, 200, 200, 200),
    QPoint(0, 50),
    96,
    QColor(0, 0, 0));

the size of the canvas is QSize(800, 600)

Diff Detail

Repository
R31 Breeze
Lint
No Linters Available
Unit
No Unit Test Coverage
zzag created this revision.Mar 10 2018, 12:08 AM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 10 2018, 12:08 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
zzag requested review of this revision.Mar 10 2018, 12:08 AM
zzag planned changes to this revision.Mar 10 2018, 12:08 AM
zzag added a dependent revision: D11069: [kdecoration] Refine shadows.
zzag updated this revision to Diff 29535.Mar 14 2018, 10:01 PM

Delete #includes.

zzag retitled this revision from [WIP][libbreezecommon] add box shadow helper to [libbreezecommon] add box shadow helper.Mar 14 2018, 10:07 PM
zzag edited the summary of this revision. (Show Details)
zzag edited the summary of this revision. (Show Details)
zzag edited the summary of this revision. (Show Details)Mar 14 2018, 10:09 PM
zzag edited the summary of this revision. (Show Details)Mar 14 2018, 10:12 PM
zzag added a comment.Mar 14 2018, 10:15 PM

Hugo, could you please review this patch?

abetts added a subscriber: abetts.Mar 14 2018, 10:16 PM
broulik added inline comments.
libbreezecommon/config-breezecommon.h.cmake
2

This file is generated and should not be checked in

zzag added inline comments.Mar 15 2018, 1:43 PM
libbreezecommon/config-breezecommon.h.cmake
2

What do you mean? We need to pass an input file to the configure_file.

zzag updated this revision to Diff 29843.Mar 18 2018, 6:43 PM
zzag edited the summary of this revision. (Show Details)
  • add comments
libbreezecommon/breezeboxshadowhelper.h
30

I think the "rule" is to not use forward declarations for classes external to once project.
Reason is that the upstream library might decide to turn a class into a struct, or an alias into future changes, which will then break your code.
Please include the headers here directly, and remove them from the cpp

zzag updated this revision to Diff 29858.Mar 18 2018, 8:54 PM

don't forward declare Qt stuff

zzag marked an inline comment as done.Mar 18 2018, 8:55 PM
hpereiradacosta accepted this revision.Mar 20 2018, 9:05 AM

Fix it (std::erf), then ship it !
I have had no time to work on an alternative QGradient blur, and wont in the near future. In the meanwhile lets use this code (that you have spent quite some time on already).
There is still time to revisit it in the future.
Thanks !

libbreezecommon/breezeboxshadowhelper.cpp
42

erf -> std::erf (since c++11).
same elsewhere. I think it is clearer to add the namespace explicitly, now that it is properly defined.

This revision is now accepted and ready to land.Mar 20 2018, 9:05 AM
zzag updated this revision to Diff 29989.Mar 20 2018, 12:54 PM

prepend std:: to erf

zzag added a comment.Mar 20 2018, 12:56 PM

@hpereiradacosta could you please accept this diff again? (I updated the diff)

In D11198#229870, @zzag wrote:

@hpereiradacosta could you please accept this diff again? (I updated the diff)

Must I ? Here the revision is marked accepted and ready to be shipped. Or is it a problem with ARC ?

zzag added a comment.Mar 20 2018, 1:03 PM
In D11198#229870, @zzag wrote:

@hpereiradacosta could you please accept this diff again? (I updated the diff)

Must I ? Here the revision is marked accepted and ready to be shipped. Or is it a problem with ARC ?

I have updated the diff so the "Accepted" should have gone away because I could introduce a bug, etc. Most likely it's a bug, I don't know ... Or Phabricator has AI so it could recognize what changes you wanted and what changes the new diff introduced.

I have updated the diff so the "Accepted" should have gone away because I could introduce a bug, etc. Most likely it's a bug, I don't know ... Or Phabricator has AI so it could recognize what changes you wanted and what changes the new diff introduced.

No no that's not how it works.
I deliberately accepted the revision, despite having still some comments about what should be implemented, because I trusted you that you would implement this and only this.
Basically when a revision is accepted, disregarding how many new diffs are uploaded to it, untill someone changes the status back to "request changes".
So ... Fill free to push

zzag added a comment.Mar 21 2018, 12:19 AM

I deliberately accepted the revision, despite having still some comments about what should be implemented, because I trusted you that you would implement this and only this.
Basically when a revision is accepted, disregarding how many new diffs are uploaded to it, untill someone changes the status back to "request changes".

Thanks for explanation. The more I live, the more I learn. :)

zzag added a comment.Apr 3 2018, 1:32 AM

If someone(in the future, I guess) will decide to optimize the box shadow helper or get rid of fftw, here's a patch to use box blur : https://phabricator.kde.org/P187

Even though box blur may be faster(I haven't benchmarked it), I still would prefer the "true" Gaussian because it looks better to me.

on the left hand side: box blur, on the right hand size: fft blur

zzag updated this revision to Diff 31808.Apr 10 2018, 12:54 PM

Rebase.

zzag updated this revision to Diff 31846.EditedApr 11 2018, 12:10 AM

Because this patch hasn't been landed yet, I would like to post here
my recent work on optimizing the box shadow helper...

Summary of changes:

  • re-write naive blur helper
  • modify only alpha channel in blur helpers
  • reserve memory for kernel in computeGaussianKernel
  • sqrt -> std::sqrt

See, benchmark results https://docs.google.com/spreadsheets/d/1ykAUyspF6BgCFb_U7-muLDEYzrOTNiT5CvFgqCU2xTs/edit?usp=sharing

zzag requested review of this revision.Apr 11 2018, 12:18 AM
zzag updated this revision to Diff 32789.Apr 22 2018, 11:43 AM

Fix invalid read of size 1

Valgrind output:

==8054== Invalid read of size 1
==8054==    at 0x1D8818C6: Breeze::BoxShadowHelper::blurAlphaNaivePass(QImage const&, QImage&, QVector<double> const&) (in /home/vlad/KDE/usr/lib64/libbreezecommon.so.5.12.80)
==8054==    by 0x1D8819F3: Breeze::BoxShadowHelper::blurAlphaNaive(QImage&, int) (in /home/vlad/KDE/usr/lib64/libbreezecommon.so.5.12.80)
==8054==    by 0x1D8822DA: Breeze::BoxShadowHelper::boxShadow(QPainter*, QRect const&, QPoint const&, int, QColor const&) (in /home/vlad/KDE/usr/lib64/libbreezecommon.so.5.12.80)
==8054==    by 0x1D60C8CC: Breeze::ShadowHelper::shadowTiles() (in /home/vlad/KDE/usr/lib64/plugins/styles/breeze.so)
==8054==    by 0x1D60C341: Breeze::ShadowHelper::loadConfig() (in /home/vlad/KDE/usr/lib64/plugins/styles/breeze.so)
==8054==    by 0x1D618B51: Breeze::Style::loadConfiguration() (in /home/vlad/KDE/usr/lib64/plugins/styles/breeze.so)
==8054==    by 0x1D613D3E: Breeze::Style::Style() (in /home/vlad/KDE/usr/lib64/plugins/styles/breeze.so)
==8054==    by 0x1D63C52F: Breeze::StylePlugin::create(QString const&) (in /home/vlad/KDE/usr/lib64/plugins/styles/breeze.so)
==8054==    by 0x7B18B62: QStyleFactory::create(QString const&) (in /usr/lib/libQt5Widgets.so.5.10.1)
==8054==    by 0x7AABA9B: QApplication::style() (in /usr/lib/libQt5Widgets.so.5.10.1)
==8054==    by 0x7AABDF5: QApplicationPrivate::initialize() (in /usr/lib/libQt5Widgets.so.5.10.1)
==8054==    by 0x7AABE5A: QApplicationPrivate::init() (in /usr/lib/libQt5Widgets.so.5.10.1)
==8054==  Address 0x16b332f7 is 3 bytes after a block of size 19,044 alloc'd
==8054==    at 0x4C2CEDF: malloc (vg_replace_malloc.c:299)
==8054==    by 0x82FDEDA: QImageData::create(QSize const&, QImage::Format) (in /usr/lib/libQt5Gui.so.5.10.1)
==8054==    by 0x82FE06C: QImage::QImage(QSize const&, QImage::Format) (in /usr/lib/libQt5Gui.so.5.10.1)
==8054==    by 0x82FE0A5: QImage::QImage(int, int, QImage::Format) (in /usr/lib/libQt5Gui.so.5.10.1)
==8054==    by 0x1D8819C5: Breeze::BoxShadowHelper::blurAlphaNaive(QImage&, int) (in /home/vlad/KDE/usr/lib64/libbreezecommon.so.5.12.80)
==8054==    by 0x1D8822DA: Breeze::BoxShadowHelper::boxShadow(QPainter*, QRect const&, QPoint const&, int, QColor const&) (in /home/vlad/KDE/usr/lib64/libbreezecommon.so.5.12.80)
==8054==    by 0x1D60C8CC: Breeze::ShadowHelper::shadowTiles() (in /home/vlad/KDE/usr/lib64/plugins/styles/breeze.so)
==8054==    by 0x1D60C341: Breeze::ShadowHelper::loadConfig() (in /home/vlad/KDE/usr/lib64/plugins/styles/breeze.so)
==8054==    by 0x1D618B51: Breeze::Style::loadConfiguration() (in /home/vlad/KDE/usr/lib64/plugins/styles/breeze.so)
==8054==    by 0x1D613D3E: Breeze::Style::Style() (in /home/vlad/KDE/usr/lib64/plugins/styles/breeze.so)
==8054==    by 0x1D63C52F: Breeze::StylePlugin::create(QString const&) (in /home/vlad/KDE/usr/lib64/plugins/styles/breeze.so)
==8054==    by 0x7B18B62: QStyleFactory::create(QString const&) (in /usr/lib/libQt5Widgets.so.5.10.1)

The reason: I forgot that the kernel is of size 2 * radius + 1 so when blurAlphaNaivePass
convolving near ends it doesn't take 1 into account. Overall, fix looks like this

diff --git a/libbreezecommon/breezeboxshadowhelper.cpp b/libbreezecommon/breezeboxshadowhelper.cpp
index 625cb26a..17d18ecd 100644
--- a/libbreezecommon/breezeboxshadowhelper.cpp
+++ b/libbreezecommon/breezeboxshadowhelper.cpp
@@ -118,7 +118,7 @@ void blurAlphaNaivePass(const QImage &src, QImage &dst, const QVector<qreal> &ke
         }

         for (int x = src.width() - radius; x < src.width(); x++) {
-            const uchar *window = in + (x - radius) * alphaStride;
+            const uchar *window = in + (x - radius - 1) * alphaStride;
             qreal alpha = 0;
             const int outside = x + radius - src.width();
             for (int k = 0; k < kernel.size() - outside; k++) {
zzag updated this revision to Diff 34593.May 21 2018, 5:35 PM

Rebase

zzag retitled this revision from [libbreezecommon] add box shadow helper to [libbreezecommon] Add box shadow helper.May 21 2018, 5:36 PM
ngraham added a subscriber: ngraham.Jun 1 2018, 1:01 PM

Instead of using pure black, how about instead using a slightly lighter dark gray color from the standard breeze color scheme, like Shade Black (35,38,39)?

https://community.kde.org/KDE_Visual_Design_Group/HIG/Color

I think that might alleviate some of my concern regarding the harsh blackness at the bottom of the windows with this patch chain. Either that, or maybe just increase the spread closer to the center of the box? Or both?

zzag added a comment.Jun 1 2018, 1:29 PM

Instead of using pure black, how about instead using a slightly lighter dark gray color from the standard breeze color scheme, like Shade Black (35,38,39)?

https://community.kde.org/KDE_Visual_Design_Group/HIG/Color

I think that might alleviate some of my concern regarding the harsh blackness at the bottom of the windows with this patch chain. Either that, or maybe just increase the spread closer to the center of the box? Or both?

I didn't change shadow color. It's still rgb(35, 38, 39), IIRC. :)

Oh OK never mind then, ignore me...

zzag added a comment.Jun 12 2018, 9:16 AM
In D11198#238841, @zzag wrote:


on the left hand side: box blur, on the right hand size: fft blur

Also, if you think box blur looks okay, I will switch box shadow helper to box blur. (that way, we don't need fftw)

In D11198#277321, @zzag wrote:
In D11198#238841, @zzag wrote:


on the left hand side: box blur, on the right hand size: fft blur

Also, if you think box blur looks okay, I will switch box shadow helper to box blur. (that way, we don't need fftw)

To be honest, I can't really see strong differences between the two. But then I have no objection either against keep the dependence on fftw.

zzag added a comment.Jun 12 2018, 11:59 AM

To be honest, I can't really see strong differences between the two. But then I have no objection either against keep the dependence on fftw.

OK, then I would like to leave it as is.

zzag added a comment.Jun 30 2018, 12:20 PM

Plasma VDG folks, what's the status of the shadow patches?

I thought this was accepted and ready to go.

ngraham accepted this revision as: VDG.Jul 2 2018, 12:29 AM

+1 visually. Sadly @hpereiradacosta stepped down as Breeze maintainer recently, but since he's already given his stamp of approval, I think this can go in.

zzag added a comment.Jul 6 2018, 3:00 PM

Landing it...

This revision was not accepted when it landed; it landed in state Needs Review.Jul 6 2018, 3:01 PM
This revision was automatically updated to reflect the committed changes.
zzag added a comment.Jul 6 2018, 3:02 PM

If there are serious problems, we still can revert it. Also, there is a chance that we need to notify sysadmins about new dependency.

Fails to build from source:

https://build.neon.kde.org/job/bionic_unstable_kde_breeze_bin_armhf/44/consoleFull

06:47:17 CMakeFiles/Makefile2:118: recipe for target 'libbreezecommon/CMakeFiles/breezecommon4.dir/all' failed
06:47:17 /workspace/build/libbreezecommon/breezeboxshadowhelper.cpp: In function ‘void Breeze::BoxShadowHelper::blurAlphaNaive(QImage&, int)’:
06:47:17 /workspace/build/libbreezecommon/breezeboxshadowhelper.cpp:138:56: error: conversion from ‘QVector<double>’ to non-scalar type ‘const QVector<float>’ requested
06:47:17 const QVector<qreal> kernel = computeGaussianKernel(radius);
06:47:17 ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~
06:47:17 In file included from /usr/include/qt4/QtGui/qpolygon.h:45:0,
06:47:17 from /usr/include/qt4/QtGui/qmatrix.h:45,
06:47:17 from /usr/include/qt4/QtGui/qtransform.h:44,
06:47:17 from /usr/include/qt4/QtGui/qimage.h:45,
06:47:17 from /usr/include/qt4/QtGui/qpixmap.h:50,
06:47:17 from /usr/include/qt4/QtGui/qpainter.h:49,
06:47:17 from /usr/include/qt4/QtGui/QPainter:1,
06:47:17 from /workspace/build/libbreezecommon/breezeboxshadowhelper.h:27,
06:47:17 from /workspace/build/libbreezecommon/breezeboxshadowhelper.cpp:21:
06:47:17 /usr/include/qt4/QtCore/qvector.h: In instantiation of ‘int QVector<T>::sizeOfTypedData() [with T = double]’:
06:47:17 /usr/include/qt4/QtCore/qvector.h:503:49: required from ‘void QVector<T>::realloc(int, int) [with T = double]’
06:47:17 /usr/include/qt4/QtCore/qvector.h:340:32: required from ‘void QVector<T>::reserve(int) [with T = double]’
06:47:17 /workspace/build/libbreezecommon/breezeboxshadowhelper.cpp:62:30: required from here
06:47:17 /usr/include/qt4/QtCore/qvector.h:323:49: warning: cast from ‘QVector<double>*’ to ‘const Data* {aka const QVectorTypedData<double>*}’ increases required alignment of target type [-Wcast-align]
06:47:17 return reinterpret_cast<const char *>(&(reinterpret_cast<const Data *>(this))->array[1]) - reinterpret_cast<const char *>(this);
06:47:17 ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
06:47:17 make[5]: [libbreezecommon/CMakeFiles/breezecommon4.dir/breezeboxshadowhelper.cpp.o] Error 1
06:47:17 make[4]: [libbreezecommon/CMakeFiles/breezecommon4.dir/all] Error 2

zzag added a comment.Jul 13 2018, 7:18 AM

Gosh, I forgot that qreal can be also a float.

On it.

zzag added a comment.Jul 16 2018, 6:46 AM

@rikmills Should build now.

In D11198#292783, @zzag wrote:

@rikmills Should build now.

@zzag It does. Thank you