[scenes/opengl] Fix overlapping shadow tiles
ClosedPublic

Authored by zzag on Feb 24 2018, 11:24 PM.

Details

Summary

This problem appears if shadow corner tiles are too big and
some window has size smaller than 2 * shadowTileSize.

This change tries to address the problem above by exclusing
overlapping tile parts. If there are any two overlapping corners
then tile between them(top/right/bottom/left) is not rendered.

Also, because some corner tile parts can be excluded, corner tiles
are expected to be symmetrical(i.e. if we remove right half from
the top-left tile and left half from the top-right tile and
stick them together, they still look fine, there are no misalignments, etc).
Most shadows(e.g. shadows from Breeze) have such behaviour.

No tiles are overlapping

Overlapping tiles

And this is how it supposed to be

Test Plan
  • apply D11069 to Breeze
  • in System Settings/Application Style/Window Decorations, choose "Very Large" shadow size
  • open Konsole
  • resize it to a minimum possible size

Diff Detail

Repository
R108 KWin
Branch
scenes-opengl-fix-overlapping-shadow-tiles
Lint
No Linters Available
Unit
No Unit Test Coverage
zzag created this revision.Feb 24 2018, 11:24 PM
Restricted Application added a project: KWin. · View Herald TranscriptFeb 24 2018, 11:24 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
zzag requested review of this revision.Feb 24 2018, 11:24 PM
zzag edited the summary of this revision. (Show Details)Feb 24 2018, 11:25 PM
zzag edited the summary of this revision. (Show Details)
zzag added a comment.Feb 24 2018, 11:27 PM

Sorry, for a little bit big patch.

Also, there are repetitions of this code(old buildQuads). Do they also have to be changed?

zzag edited the summary of this revision. (Show Details)Feb 24 2018, 11:30 PM
anemeth added a comment.EditedFeb 25 2018, 12:35 PM

I applied both patches.
Looks like the shadow generation is done when a window is opened when there are no other windows open and it freezes the whole desktop while doing it.
Opening any window while another window is open does not freeze the system.

zzag added a comment.EditedFeb 25 2018, 1:25 PM

@anemeth: freezing is caused by the patch from the testing plan. (it's doing blurring on CPU several times).

That patch and this one are not related.

Edit

Just to make thing clear: the patch in the test plan is just to show the problem.

graesslin requested changes to this revision.Feb 25 2018, 1:36 PM

I would really like to see unit tests for these changes. E.g. extend the scene_opengl_test.cpp and the scene_qpainter_test.cpp

plugins/scenes/opengl/scene_opengl.cpp
2157–2159

To explain why this was added: in KPart based applications it could happen that a menu is empty. In that case all we got was the shadow. IIRC kdevelop was able to trigger this.

This revision now requires changes to proceed.Feb 25 2018, 1:36 PM
zzag added a comment.EditedFeb 25 2018, 2:11 PM

@graesslin: Also, why SceneOpenGLShadow overrides buildQuads? Is it just to define texture coordinates?

zzag added inline comments.Feb 25 2018, 2:18 PM
plugins/scenes/opengl/scene_opengl.cpp
2157–2159

Is there a test plan to see what it looks like without this if statement?

In D10811#213678, @zzag wrote:

@graesslin: Also, why SceneOpenGLShadow overrides buildQuads? Is it just to define texture coordinates?

I don't remember.

plugins/scenes/opengl/scene_opengl.cpp
2157–2159

I hoped to find a bug report with the issue, but the commit which introduced that if statement doesn't have one linked: b837a3fca184fce0c7da317b2866d409892e71eb

zzag added a comment.Feb 26 2018, 7:16 PM

@graesslin: Does the qpainter backend render shadows?

Here's my workflow

KWIN_COMPOSE=Q kwin_wayland --windowed --xwayland

DISPLAY=:1 kate
DISPLAY=:1 konsole
In D10811#214616, @zzag wrote:

@graesslin: Does the qpainter backend render shadows?

good questions. Let's ask the code... There's a SceneQPainter::Window::renderShadow so it should render a shadow.

zzag added a comment.EditedFeb 27 2018, 12:01 AM

good questions. Let's ask the code... There's a SceneQPainter::Window::renderShadow so it should render a shadow.

I saw that method too. ;-)

Anyway, I'll dig later why QPainter backend doesn't render shadows.

Update

QPainter backend draws shadows only for windows which support KWin's shadow protocol and do not have decorations.

zzag added a comment.EditedFeb 27 2018, 2:05 AM

Is it possible to set custom window decoration in tests? AbstractClient::setDecoration is protected. DecorationBridge is a singleton. Beside this, Qt Test, seems, doesn't have any tools to create mocks/stubs, and because this is C++ I can't do monkey patching, etc. So, how would I test this?

PS: I could derive a class from SceneOpenGLShadow, pass to the Shadow some dummy Toplevel object, override toplevel(), topOffset(), rightOffset(), bottomOffset(), leftOffset() and elementSize(), but I don't like it, that's wrong.

zzag added a comment.Feb 27 2018, 11:23 AM

I've implemented painting decoration shadows in the QPainter backend, fixed overlapping tiles in the decoration kcm and worked around the issue with turning shadows off when window is small. Yet, there are no tests.. I really would like to have some help with testing. :(

https://github.com/zzag/kwin/commits/overlapping-shadow-tiles

zzag updated this revision to Diff 28325.Mar 1 2018, 2:12 PM

add tests

zzag retitled this revision from fix overlapping shadow tiles to [scenes/opengl] fix overlapping shadow tiles.Mar 1 2018, 2:12 PM
zzag updated this revision to Diff 28352.EditedMar 1 2018, 7:11 PM

formatting: add final newline to autotests/integration/fakes/cmakelists.txt,
delete tab which was added by an accident

zzag updated this revision to Diff 28635.Mar 4 2018, 9:03 PM

use ">=" instead of ">" when checking for shadow tile overlaps

zzag updated this revision to Diff 29616.Mar 15 2018, 5:02 PM

tests: rename test helpers

@graesslin, looks like the tests are in now. Does this look shippable, or are additional changes needed?

@graesslin ping. This is the last patch awaiting review in the whole chain.

zzag edited the test plan for this revision. (Show Details)Apr 10 2018, 1:16 PM
zzag edited the summary of this revision. (Show Details)Apr 10 2018, 1:19 PM
zzag added a comment.Apr 15 2018, 5:57 PM

Friendly ping.

zzag planned changes to this revision.May 20 2018, 8:46 PM

Delete WindowQuadListWrapper

I was reading some libkwineffects tests and noticed https://github.com/KDE/kwin/blob/master/autotests/libkwineffects/windowquadlisttest.cpp#L23
I think that is better than having a wrapper class.

zzag updated this revision to Diff 34587.May 21 2018, 4:31 PM

Delete WindowQuadListWrapper in tests

zzag retitled this revision from [scenes/opengl] fix overlapping shadow tiles to [scenes/opengl] Fix overlapping shadow tiles.May 21 2018, 4:31 PM
davidedmundson accepted this revision.May 22 2018, 8:15 PM
davidedmundson added a subscriber: davidedmundson.

Code ended up quite a bit neater, good stuff.

zzag updated this revision to Diff 34705.May 23 2018, 12:57 PM

Add missing const qualifiers.

zzag added a comment.May 31 2018, 11:42 AM

@davidedmundson What's the status of this diff? Should I wait for Martin's review?

If I give it a ship it and Martin doesn't comment in a week or so, go for it.

Are you still without a dev account?

zzag added a comment.May 31 2018, 11:46 AM

If I give it a ship it and Martin doesn't comment in a week or so, go for it.

Got it.

Are you still without a dev account?

No, I got it back.

davidedmundson accepted this revision.Jun 1 2018, 11:05 AM
This revision was not accepted when it landed; it landed in state Needs Review.Jun 7 2018, 9:08 AM
This revision was automatically updated to reflect the committed changes.