[kdecoration] Refine shadows
ClosedPublic

Authored by zzag on Mar 5 2018, 8:17 PM.

Details

Summary

5.12 release introduced new decoration shadows. These new shadows are
bigger, centered, and they try to solve a problem related to depth.
Yet, there are still some problems:

  • new(5.12) shadows are hard.
  • lighting model is broken. Because 5.12 shadows have been centered and made bigger, they should be casted from north.

This change refines how decoration shadows look:

  • shadows are casted from north(shadow under window is bigger and darker, side shadows are smaller, etc);
  • shadows are made up of several separate shadows(one for overall shape, another for contrast). The separate shadows are produced by using box shadow helper(something similar to CSS box-shadow property, except there are no inset and spread properties);
  • shadow sizes(e.g. Small, Medium, and so on) haven't changed.

Because GPUs can't be used for blurring images(in our case), the box shadow
helper is using CPU to blur images. More precisely, when blur radius is < 64,
images are blurred with "separable convolutions", otherwise, images are blurred
with a two-dimensional Fourier Transform.

Desktop experience with the refined shadows

Depends on D11198

Diff Detail

Repository
R31 Breeze
Lint
No Linters Available
Unit
No Unit Test Coverage
There are a very large number of changes, so older changes are hidden. Show Older Changes

Hello,
Thanks for the patch.

  • As we discussed already in telegram, I am not convinced you absolutely need fast fourier transform nor blur, and could probably handled thing with "simple" QGradients.

If you do not have time to try implement such a solution, I can give it a shot myself whenever there is time.

  • I can also help with moving the loading and destroying of the shadow to the pluggin loading/unloading rather than to first and last window opening. (this could also go in with the current shadows in fact, and would rather be a separate patch).
  • Also, these shadows look nice but are rather strong. To me they represent a shift of paradigm with respect to the original (subtle) breeze ideas. But that is not me to decide. Still: could you also post screenshots with the smaller sizes ? And then we can discuss what the default size should be.
  • finally, I really don't think we need too separate setting for the two shadow strength. These are just implementation details. IMHO. And will make the code very hard to maintain. (how do you test that all combinations of settings look good ?)

Others opinion welcome, of course.

zzag added a comment.EditedMar 5 2018, 9:19 PM

Hello,
Thanks for the patch.

  • As we discussed already in telegram, I am not convinced you absolutely need fast fourier transform nor blur, and could probably handled thing with "simple" QGradients. If you do not have time to try implement such a solution, I can give it a shot myself whenever there is time.

It's a little bit hard(at least for me) to make good lookin shadows with gradients. For example, how would someone handle corners with gradients? Blur makes things much easier..

So, yeah, could you please implement these softer shadows with gradients?

  • I can also help with moving the loading and destroying of the shadow to the pluggin loading/unloading rather than to first and last window opening. (this could also go in with the current shadows in fact, and would rather be a separate patch).

It would be great to get help with plugins. I'm not familiar with KPlugin shenanigans

  • Also, these shadows look nice but are rather strong. To me they represent a shift of paradigm with respect to the original (subtle) breeze ideas. But that is not me to decide. Still: could you also post screenshots with the smaller sizes ? And then we can discuss what the default size should be.

They're not final. Shadow params is an open question. Yes, I'll post them later.

Also, could some of original Breeze authors review this too(or provide their input)?

  • finally, I really don't think we need too separate setting for the two shadow strength. These are just implementation details. IMHO. And will make the code very hard to maintain. (how do you test that all combinations of settings look good ?)

These shadows are softer than shadows in kstyle. So, single shadow strength option doesn't fit for both of them, it either

  • makes decoration shadows very light, and kstyle shadows normal
  • makes decoration shadows normal, and kstyle shadows really dark

I'm still thinking about it, having two different shadow styles is not OK.

zzag added a comment.EditedMar 5 2018, 9:44 PM

About KStyle: I suggest to use gradients in order to approximate decoration shadows. It doesn't have big shadows so we don't need big precision. Also, I suggest to make them lighter.

zzag added a comment.Mar 5 2018, 10:03 PM

None

Small

Medium

Large

Very Large

but background also matters

fabianr added a subscriber: fabianr.Mar 6 2018, 9:07 AM

I don't like how black the shadow is on the bottom. Once the strength adjustment works, we might experiment with reducing it.

zzag added a comment.Mar 6 2018, 12:16 PM

I don't like how black the shadow is on the bottom. Once the strength adjustment works, we might experiment with reducing it.

Yeah.. Shadow strength should at most 90%.

In D11069#219503, @zzag wrote:

About KStyle: I suggest to use gradients in order to approximate decoration shadows. It doesn't have big shadows so we don't need big precision. Also, I suggest to make them lighter.

Forget about it. Bad idea.

Hello,

  • I agree with Nathan that the bottom part is likely too hard.
  • I think the same shadows should be used for windows and menus
  • Also, thanks for posting the pictures. Is there any chance you could also post new vs old shadows side by side for the different sizes ? I have the feeling that for a given shadow size the new shadows appear larger ...

Tanks in advance !

Hugo

zzag updated this revision to Diff 28833.Mar 6 2018, 2:12 PM

control look of each separate shadow with the opacity prop

zzag planned changes to this revision.Mar 6 2018, 2:13 PM
zzag added a comment.Mar 6 2018, 2:31 PM
  • I agree with Nathan that the bottom part is likely too hard.

I've added opacity prop so different shadows could be tweaked("darkess" at the bottom is coming from the contrast shadow). Also, shadow strength is ignored... And again, shadow params is an open question.

  • I think the same shadows should be used for windows and menus

Yep, that's why I rejected the idea to approximate deco shadows. I've been thinking about moving shadow creation logic out so it could be used by KDecoration and KStyle plugin. What's your opinion on this?

I would like to get input from VDG folks how menu shadows should look like.

  • Also, thanks for posting the pictures. Is there any chance you could also post new vs old shadows side by side for the different sizes ? I have the feeling that for a given shadow size the new shadows appear larger ...

OK. I'll post later.

zzag added a comment.Mar 6 2018, 3:19 PM

Left - old
Right - new


Small

Medium

Large

Very Large


God, Phabricator is awful! Image upload sometimes works, sometimes not. Also, typing a comment is very CPU intensive.. :/

So in general, I like the more rounded edges. But I still don't like the darker shadows and the big black line on the bottom.

zzag updated this revision to Diff 28847.Mar 6 2018, 4:17 PM

take shadow strength into account

zzag planned changes to this revision.Mar 6 2018, 4:17 PM
zzag added a comment.Mar 6 2018, 4:21 PM

Because shadow strength isn't ignored anymore, kstyle shadows should be updated

zzag updated this revision to Diff 28863.Mar 6 2018, 7:53 PM

change box shadow api

zzag planned changes to this revision.Mar 6 2018, 7:53 PM

TODO:

  • handle HiDPI
zzag added a comment.Mar 6 2018, 7:59 PM

Large shadows with 80% strength

That looks pretty darn nice.

zzag updated this revision to Diff 28868.Mar 6 2018, 8:53 PM

bump default shadow strength to 80%

zzag planned changes to this revision.Mar 6 2018, 8:53 PM
zzag updated this revision to Diff 28874.Mar 6 2018, 10:43 PM

edit comments

zzag planned changes to this revision.Mar 6 2018, 10:43 PM
zzag updated this revision to Diff 28893.Mar 7 2018, 9:11 AM

take scaling factor into account

zzag planned changes to this revision.Mar 7 2018, 9:11 AM
zzag added a comment.EditedMar 8 2018, 3:36 PM

So, I'd spent last two hours trying to find good menu shadow params.. HIG guidelines don't say too much about shadows(also, navigating them is hard) so I don't know whether these shadows are good.

You can help by providing CSS shadows.

Some questions:

  • Do we really need different shadow sizes for menus, etc? Can we use single shadow size in kstyle?
  • Do menus have outlines? If you look at picture above, you can see grey "gap" between menu and submenu.
  • What's the best way to share code between KStyle and KDecoration plugin? (currently, I'm symlinking files)

In addition to the first question, it's a little bit weird that changing window decoration shadow size also changes shadows "in apps" (e.g. menu shadows would have different size, etc).

hpereiradacosta added a comment.EditedMar 8 2018, 3:45 PM

Some questions:

  • Do we really need different shadow sizes for menus, etc? Can we use single shadow size in kstyle?

I'd say yes, as soon as you provide different shadow sizes for the windows (that also answers the last question)

  • Do menus have outlines? If you look at picture above, you can see grey "gap" between menu and submenu.

yes menus have outlines. In fact, in the old shadows, windows also had an outline. (drawn with the shadow).
We want to keep it as far as I know.

  • What's the best way to share code between KStyle and KDecoration plugin? (currently, I'm symlinking files)

File links is not an option. (it breaks when you make tarballs of when compiling on e.g. windows)
You would need to move the files away in a separate folder, compile them in a library, link the library to both kstyle and kdecoration. Oxygen does that.

In addition to the first question, it's a little bit weird that changing window decoration shadow size also changes shadows "in apps" (e.g. menu shadows would have different size, etc).

Why weird ? if I want "large shadows" I want them large for everything that have shaodws (with possible hierarchy). And if I want small I want them all small.
That seems natural to me.

Other than that, screenshot on menu shadow looks pretty nice !

zzag added a comment.Mar 8 2018, 4:09 PM
  • What's the best way to share code between KStyle and KDecoration plugin? (currently, I'm symlinking files)

File links is not an option. (it breaks when you make tarballs of when compiling on e.g. windows)
You would need to move the files away in a separate folder, compile them in a library, link the library to both kstyle and kdecoration. Oxygen does that.

Thanks, I'll take a look at oxygen.

I'd say yes, as soon as you provide different shadow sizes for the windows (that also answers the last question)

Well, than I would like to get help with params. I believe computed params won't produce good looking results so they have to be hand picked, maybe I'm wrong. VDG folks, if you know CSS, you can help!

In D11069#221381, @zzag wrote:

So, I'd spent last two hours trying to find good menu shadow params.. HIG guidelines don't say too much about shadows(also, navigating them is hard) so I don't know whether these shadows are good.

As you noticed HIG has very few information on shadows, it would be great if the results of this review could also be added to the hig.
https://hig.kde.org/resources/contribute.html

zzag updated this revision to Diff 29071.Mar 9 2018, 10:55 AM

move box shadow helper out to a separate library

zzag updated this revision to Diff 29072.Mar 9 2018, 10:55 AM

fix previous update

zzag planned changes to this revision.Mar 9 2018, 10:56 AM
zzag retitled this revision from [WIP] refine decoration shadows to [WIP][kdecoration] refine shadows.
zzag added a comment.EditedMar 9 2018, 11:29 AM

TODOs(kdecoration and kstyle):

  • revise params
  • document how to fill params

@hpereiradacosta: could you please give it first round code review?

zzag updated this revision to Diff 29121.EditedMar 9 2018, 11:29 PM
  • fix KDE4 build
zzag planned changes to this revision.Mar 9 2018, 11:31 PM
zzag added a comment.EditedMar 9 2018, 11:35 PM

Planned changes:

  • split this patch(into deco shadow patch and libbreezecommon patch)
  • fix params issues
zzag updated this revision to Diff 29127.Mar 10 2018, 12:09 AM
  • move libbreezecommon to another patch
zzag planned changes to this revision.Mar 10 2018, 12:10 AM
zzag edited the summary of this revision. (Show Details)
zzag updated this revision to Diff 29197.Mar 11 2018, 1:31 AM
  • bump strength to 85%
zzag planned changes to this revision.Mar 11 2018, 1:37 AM

change the way default params are represented(right now, it's js-ish)


I'll take a break from working on these shadows.

Current look:

Menu shadows:

Looks pretty nice to me!

zzag updated this revision to Diff 29540.Mar 14 2018, 10:18 PM
  • tweak opacity values
  • bump strength to 100%
  • don't use aggregate initialization(available only in >= C++20)
zzag retitled this revision from [WIP][kdecoration] refine shadows to [kdecoration] refine shadows.Mar 14 2018, 10:20 PM
zzag edited the summary of this revision. (Show Details)
zzag added a subscriber: abetts.

Hugo, could you please review this patch?

I still think the light source is a bit too far up on the Y axis, and the bottom shadow is a bit too big compared to the side shadows. But I won't -1 it, since these new shadows are just so awesome in the aggregate!

zzag added a comment.Mar 14 2018, 10:31 PM

I still think the light source is a bit too far up on the Y axis, and the bottom shadow is a bit too big compared to the side shadows. But I won't -1 it, since these new shadows are just so awesome in the aggregate!

I haven't implemented spread property, sorry. I don't have too much time for this. :(

What is there left to be done in this implementation? Are we close to shipping?

zzag added a comment.Mar 14 2018, 10:35 PM

Oh, sorry, I misunderstood you. Bottom shadows should be bigger than side shadows because shadows are casted from north. :)

For example, raise a light source above the title bar and look at the window from infinity(I wish I could have such sight).

In D11069#226037, @zzag wrote:

Oh, sorry, I misunderstood you. Bottom shadows should be bigger than side shadows because shadows are casted from north. :)

For example, raise a light source above the title bar and look at the window from infinity(I wish I could have such sight).

Right, and I can accept that. Visually, I feel like the bottom shadow is a bit too stark and eye-catching. There's a very dark strip right under the bottom of the window that I find a bit distracting.

zzag added a comment.Mar 14 2018, 10:40 PM

What is there left to be done in this implementation?

If you're happy about current look of the shadows than nothing. :)

I'm still waiting for KWin patches to be reviewed. (see 'Depends on' in the revision's summary)

zzag added a comment.EditedMar 14 2018, 10:47 PM

Right, and I can accept that. Visually, I feel like the bottom shadow is a bit too stark and eye-catching. There's a very dark strip right under the bottom of the window that I find a bit distracting.

Well, according to the lighting model that's totally correct. So, I don't want to change that.

If bottom shadows and side shadows were equally sized, then they would give an impression like it's an aura or neon or whatever. That would be totally fine in Oxygen but here.. I dunno.

hpereiradacosta accepted this revision.Mar 20 2018, 9:10 AM

Ship it. Thanks !

This revision is now accepted and ready to land.Mar 20 2018, 9:10 AM
zzag added a comment.Mar 20 2018, 12:58 PM

Hey, everyone!

Even though the patches are accepted I won't land them because KWin patches haven't landed yet(I'm waiting for the code to be reviewed).

First, I am now running this set of patch routinely and think these new shadows are gorgeous on a dayly usage. (I think the default is too large to my taste, but using a 70% shadow strenght and medium size, I'm quite happy with the result). So no regret with accepting it.
Second, concerning code review:
I now see that you kept the door opened for horizontal offsets in your shadowParams definition and in the code implementation. I think this is quite superfluous, and could be dropped. Not sure that the current shadow model works for non centered shadows anyway.
This would result in changing .offset frop QPoint to int, and modifying the rendering code accordingly. IMHO there is no point being too generic here.

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

Rebase.

zzag edited the summary of this revision. (Show Details)May 21 2018, 4:55 PM
zzag updated this revision to Diff 34594.May 21 2018, 5:37 PM

Rebase

zzag retitled this revision from [kdecoration] refine shadows to [kdecoration] Refine shadows.May 21 2018, 5:37 PM
zzag updated this revision to Diff 35487.EditedJun 3 2018, 9:15 PM

Do not allow dpr < 1.

Before:

After:

zzag updated this revision to Diff 35488.Jun 3 2018, 9:23 PM

Wrong comparison

zzag planned changes to this revision.Jun 3 2018, 9:28 PM

OK, the issue above is still present. Need to fix this somehow.

zzag updated this revision to Diff 35489.Jun 3 2018, 9:37 PM

Drop HiDPI support.

This revision is now accepted and ready to land.Jun 3 2018, 9:37 PM
This revision was automatically updated to reflect the committed changes.