[scenes/opengl] Correctly draw shadows when corner tiles are missing
ClosedPublic

Authored by zzag on Aug 13 2018, 10:59 AM.

Details

Summary

Current implementation of buildQuads assumes that corner shadow tiles
are always present:

const QRectF leftRect(
    topLeftRect.bottomLeft(),
    bottomLeftRect.topRight());

but that assumption is wrong. For example, if the default panel is on
the bottom screen edge, then the calendar popup won't have the
bottom-left shadow tile(at least on Wayland). Which means that the left
shadow tile won't be visible because
topLeftRect.left() == bottomLeftRect.right().

Corner rectangles only have to influence height of the left/right tile
and width of the top/bottom tile. Width of the left/right tile and
height of the top/bottom tile should not be controlled by corner tiles.

Overall, this is how shadow quads are computed:

  • Compute the outer rectangle;
  • Compute target rectangle for each corner tile. If some corner tile is missing, move the target rectangle to the corresponding corner of the inner shadow rect and set its width and height to 0. We need to do that to prevent top/right/bottom/left tiles from spanning over corners:

We would rather prefer something like this if the top-left tile is
missing:

  • Fix overlaps between corner tiles;
  • Compute target rectangles for top, right, bottom, and left tiles;
  • Fix overlaps between left/right and top/bottom shadow tiles.
Test Plan
  • Ran tests;
  • Resized Konsole to its minimimum size(on X11 and Wayland);
  • Opened the calendar popup(on X11 and Wayland):

Before:

After:

Diff Detail

Repository
R108 KWin
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 2086
Build 2104: arc lint + arc unit
zzag created this revision.Aug 13 2018, 10:59 AM
Restricted Application added a project: KWin. · View Herald TranscriptAug 13 2018, 10:59 AM
Restricted Application added a subscriber: kwin. · View Herald Transcript
zzag requested review of this revision.Aug 13 2018, 10:59 AM
zzag added a comment.Aug 13 2018, 11:27 AM

Just for tracking was this introduced in 7637cfc22bbdc9c6f51f2e32b24ba26fb839351d?

Yes, sorry.

zzag added a comment.Aug 13 2018, 11:31 AM

Yet, some bugs were before that. (e.g. spanning over corner tiles)

abetts added a subscriber: abetts.Aug 20 2018, 2:42 PM

From a visual perspective, +1

davidedmundson added inline comments.Aug 21 2018, 3:44 PM
plugins/scenes/opengl/scene_opengl.cpp
2140

just > ?

if they're == then we don't need to do anything

also the qAbs is not needed, we know leftRect.right > rightRect.left

2188–2189

may as well

max(top,bottom) + shadowMargins.left + shadowMargins.right

2246–2249

There's an interesting hypothetical here.

We're treating the null corners as rectangles at a single point, potentially if you had a large enough topleft shadow no topright corner you'll end up distributing it; and now our null image will get a valid rectangle.

2337

Why this change? We've redistributed topRight so using it should be fine?

zzag added inline comments.Aug 21 2018, 4:24 PM
plugins/scenes/opengl/scene_opengl.cpp
2246–2249

That's pretty good catch. I'll fix it.

But, no, it [top-right corner rect] won't be valid, so it won't be rendered.

zzag updated this revision to Diff 40162.Aug 21 2018, 6:05 PM

Review comments

zzag updated this revision to Diff 40163.Aug 21 2018, 6:07 PM

Fix typo in test name

zzag marked 4 inline comments as done.Aug 21 2018, 6:13 PM
zzag added inline comments.
plugins/scenes/opengl/scene_opengl.cpp
2337

Sorry, didn't notice this comment.

That's to handle the case when top.width() is smaller than distance between top-left and top-right corner in the texture atlas.

zzag added inline comments.Aug 21 2018, 6:18 PM
plugins/scenes/opengl/scene_opengl.cpp
2337

I.e. if the bottom tile is wider than the top tile.

davidedmundson added inline comments.Aug 21 2018, 6:43 PM
plugins/scenes/opengl/scene_opengl.cpp
2337

D14784 has the line

p.drawPixmap(width - topRight.width(), 0, shadowPixmap(ShadowElementTopRight));

zzag added a comment.Aug 21 2018, 7:09 PM

Maybe, it makes more sense to squash D14783 and D14784.

plugins/scenes/opengl/scene_opengl.cpp
2337

It draws the top-right tile in the top-right corner of the texture atlas.

For example, take a look at this texture atlas, here's bottom tile is wider than top tile.

sorry for quick & dirty drawing

davidedmundson accepted this revision.Aug 21 2018, 9:06 PM
This revision is now accepted and ready to land.Aug 21 2018, 9:06 PM

We're being inconsistent in what we correct from the client.

In the other thread, the bug of left being wider than topleft causing a gap was considered the clients fault and not kwin's problem.
But here we're going out of our way to support top being thinner than bottom.

It all seems to have ended up just as messy as before. Lets ship this and hope we never have to touch the shadow code again.

zzag added a comment.Aug 31 2018, 11:01 AM

In the other thread, the bug of left being wider than topleft causing a gap was considered the clients fault and not kwin's problem.
But here we're going out of our way to support top being thinner than bottom.

We have to stretch top/right/bottom/left tiles so that's fine in some sense.

But, yeah, I agree, it's messy a little bit. I think with the current code we do the best what we can do.

Maybe, we could put some constrains on tiles provided through the ShadowInterface, but then we maybe will break some of desktop themes.

With KDecoration2, things are more cleaner.

It all seems to have ended up just as messy as before.

Yeah... :(

This revision was automatically updated to reflect the committed changes.