[effects] Fix several animation and reflection issues with cube effect
ClosedPublic

Authored by poboiko on Jan 13 2018, 4:20 PM.

Details

Summary

This patch intends to fix Bug 213599 and incorrect reflection when rotating the cube using arrow keys (see screencasts)

The main problem was that inside the effect there was manualVerticalAngle, which did not represent the actual rotation angle of the cube during the animation, but used to calculate the position of the reflection. The actual angle was calculated on-the-fly and was not exposed outside. Yet I found the code related for such animations rather cumbersome, with lots of code duplication, so I tried to reorganize it, though keeping the logic (as I understood it).

Brief description of what the code does:

  • variables currentAngle and verticalCurrentAngle now always represent the current position of the cube. They are updated when one uses the mouse and inside the rotateCube() method, which is called in prePaintScreen().
  • two queues, animations (used for Start / Stop / Left / Right) and verticalAnimations (used for Up / Down) are used for scheduling the animations if i.e. user presses several keys in a row. The code checks whether the last animation has finished (and thus we need to start a new one) inside prePaintScreen() and postPaintScreen() (not sure if two places actually needed though).
  • when the animation starts, code saves the starting position of the cube inside startAngle, startFrontDesktop and verticalStartAngle variables, which are used to calculate the actual cube position during the animation later. This is done by startAnimation() and startVerticalAnimation(), which also calculates the QTimeLine curves needed for animation

It also fixes Bug 373101, that is incorrect reflection for cylinder (see another screencast), and adds reflection for sphere (with correct position as well).

If one uses sphere cap deformation, the caps were ill-positioned (didn't find a bug on bugzilla though), I have also fixed that

Last two issues are actually somewhat separate, I guess I can prepare two another differential revisions if needed.

Test Plan
  • All the described glitches disappeared
  • To make sure that rotateToDesktop works fine, I tried moving mouse around really fast while pressing "1" to switch to first desktop (so that current desktop might change somewhere in between pushing animations to queue and actually starting to run them). Seems to work fine (although had to fix it).
  • Tested different number of virtual desktops (3-6)
  • Tested with external monitor. It didn't broke.
  • Tested both cylinder and sphere, with different sphere cap deformations. Found another glitch, which is a bit harder to fix (screw trigonometry...): if the cap deformation is non-zero, the reflection is calculated as it is sphere there, and thus is incorrect
  • Another known glitch: the reflections of windows, if they hover above the cube, are totally messy (see screenshot, lower-left corner). I suggest to paint only the reflection of the cube itself, if hovering is enabled.

Diff Detail

Repository
R108 KWin
Lint
Lint Skipped
Unit
Unit Tests Skipped
poboiko created this revision.Jan 13 2018, 4:20 PM
Restricted Application added a subscriber: kwin. · View Herald TranscriptJan 13 2018, 4:20 PM
poboiko requested review of this revision.Jan 13 2018, 4:20 PM
zzag added a subscriber: zzag.Jan 13 2018, 7:50 PM
zzag added inline comments.
effects/cube/cube.cpp
425

Oh, Jeez.. (so mathy)

466
if (animationState == Start) {

} esle if (animationState == Stop) {

}
672
if (animationState == Start) {
    ...
} else if (animationState == Stop) {
    ...
}
1195

if ... else if ..

1207

if ... else if ...

1565

verticalCurrentAngle = qBound(-90.0f, verticalCurrentAngle, 90.0f)

Heavy change. I fear we are a little bit too late for 5.12 and I don't feel supper comfortable with going for bug fix release with such a large change. But I don't see a problem with pushing after 5.12 is branched.

Thanks for doing some work on it. It has been years since someone touched that code.

effects/cube/cube.cpp
346–347

please use {}

357

Please use {}

1444

why equal as well?

effects/cube/cube.h
146

As it is basically a new enum I suggest to use enum class

poboiko added inline comments.Jan 13 2018, 9:28 PM
effects/cube/cube.cpp
425

Yeah. I had to run that around to make sure it works...

1444

That is actually a point I forgot to rise.
Basically, on a keyboard without keypad, "+" and "=" are on the same key. However, to actually press "+" I have also to press "Shift", which is inconvenient. At least, in Dolphin / Gwenview / Okular, zoom works without shift, just with Ctrl+"=". Though I am not sure this is a proper way to handle it, looks more like a workaround. What do you think?

1565

Right, I was almost sure there should be some macro like that. Thanks for pointing!

poboiko updated this revision to Diff 25286.Jan 13 2018, 9:31 PM
  • enum -> enum class
  • more brackets, more else's
  • use qBound
poboiko updated this revision to Diff 25287.Jan 13 2018, 9:33 PM
poboiko marked 7 inline comments as done.

Woops, added wrong file. Fixing.

poboiko updated this revision to Diff 26817.Feb 9 2018, 1:04 PM

Found and fixed a problem: if rotateToDesktop() is called when animation is running (during which currentDesktop might change).
For a user, the problem shows itself as follows:

  • First desktop is active
  • User presses "right" key and instantly presses number "3" to switch to 3rd desktop
  • Since first desktop is active, the code adds two "right" rotations to queue; but, since one "right" rotation is already running, we will end up at 4th desktop instead of 3rd.
zzag added inline comments.Mar 27 2018, 4:21 PM
effects/cube/cube.cpp
356

Please compare floating point numbers properly.

377

Please use {}. As far as I remember, in KWin, 1TBS-like style is preferred.

zzag added inline comments.Mar 27 2018, 4:25 PM
effects/cube/cube.h
203

Maybe, one-declaration per line is better. :)

209

same here

poboiko updated this revision to Diff 30902.Mar 30 2018, 7:20 AM
  • Fixed glitch: user could have used mouse/keyboard when Stop animation was scheduled.
  • Fixed several issues with code style
poboiko marked 4 inline comments as done.Mar 30 2018, 7:21 AM
zzag added a comment.Mar 30 2018, 12:46 PM

Please notice that qFuzzyCompare doesn't work if any of its arguments is 0. I believe verticalCurrentAngle could be 0 so you may want to workaround the issue with qFuzzyCompare.

One possible workaround: qFuzzyIsNull(verticalCurrentAngle - 90.0f)

poboiko updated this revision to Diff 30944.Mar 30 2018, 2:20 PM

Vlad is absolutely right, my fault. I haven't noticed that qFuzzyCompare looks for relative difference. Switched to qFuzzyIsNull instead.

Ping?
I've been running this for few months already, occasionally using cube, and haven't notice any bugs/glitches related to this patch so far.

romangg accepted this revision.Jun 1 2018, 12:56 PM
romangg added a subscriber: romangg.

I can't say much about the code, but I tried out the change and didn't notice any regressions and I assume the change does improve the overall code quality and fixes some bugs. I would say let's get this effort merged into master and see afterwards if there are regressions reported. The effect is rather rarely used so breakage shouldn't be a huge problem.

Please add

Bug: 213599
Bug: 373101

to the commit message so it closes these.

This revision is now accepted and ready to land.Jun 1 2018, 12:56 PM
ngraham added a subscriber: ngraham.Jun 1 2018, 1:19 PM
This revision was automatically updated to reflect the committed changes.