Merge lp:~mc-return/compiz/compiz.merge-fix1162484-original-cube-top-and-bottom-caps-not-deformed into lp:compiz/0.9.10

[MC Return]

Cube-addon (Cube Reflection and Deformation):

If the user disables "Draw top face"/"Draw bottom face"
we do not want to draw anything (LP: #1162484).

The original, non-deformed caps will only work for the
non-deformed cube, so we can just use the original function
in that case.

We need to clear the texture if no texture files are
specified in "Image files", otherwise the old texture
would still be drawn, even if all image files are
removed (LP: #1162711).

Now we will default back and use the cube cap
colors and opacities defined in the "Desktop Cube" plugin
(if "Draw top/bottom face" are enabled only, see above).

This way the user can choose between (for top/bottom):

1. Do not draw a cube cap face at all
   ("Draw top/bottom face" option disabled)
2. Use color and opacity specified in "Desktop Cube"
   (empty images list)
3. Use a texture for the cap
   (image is in the list, which is default)

Removed redundant mCurrent = mCurrent % mFiles.size ();
calculation, this has already been done:
cap->mCurrent = (cap->mCurrent + change + count) % count;

count and change both need to be != 0 for mCurrent to change.

Fixed indentation, removed redundant brackets and newlines,
declaration and assignment of local variables in one line,
if possible, minor cleanup.

(LP: #1162484, LP: #1162711)

Note:

Bug #1162740: 'Key-combinations to change top or bottom cap textures do not work' (also regarding the caps) is still open, so the caps functionality is just almost perfect... :(
I am hunting this bug as well, but am currently a bit stuck with it (I added details to
the bug report already).

Bug #1163880: 'Deformation fails during time of window animation' is also still open

[Daniel van Vugt (vanvugt)]

Please re-target this for lp:compiz/0.9.10

review: Needs Resubmitting

[Sam Spilsbury (smspillaz)]

I'm a bit confused about this:

5 void
6 CubeaddonScreen::CubeCap::load (bool scale, bool aspect, bool clamp)
7 {
8 - if (mFiles.empty ())
9 - return;
10 -

It seems like the point is to remove the early return if no images are available. However cubeaddon only loads images (to the best of my understanding). I'm pretty sure there's a risk of a segfault or std::out_of_range exeception just a few lines underneath it:

    mCurrent = mCurrent % mFiles.size ();

    CompString imgName = mFiles[mCurrent].s ();
    CompString pname = "cubeaddon";
    mTexture = GLTexture::readImageToTexture (imgName, pname, tSize);

Was this change really needed?

34 - cScreen->damageScreen ();
35 - }

Was there a reason for this one? The function documentation says that we should be posting a full-screen damage at that point.

/*
 * Switch cap, load it and damage screen if possible
 */
bool
CubeaddonScreen::changeCap (bool top, int change)

I'd imagine that's because if this function happened on a timer and there was no other damage or motion, we'd want the cap change to reflected as an immediate repaint.

227 -
228 CubeaddonScreen::CubeaddonScreen (CompScreen *s) :
229 PluginClassHandler<CubeaddonScreen, CompScreen> (s),
230 CubeaddonOptions (),
231 @@ -1608,4 +1605,3 @@
232
233 return true;
234 }
235 -

This is just a change in whitespace. Its not really a big problem, see if you can avoid them though because too many can clutter up diffs.

[MC Return (mc-return)]

Hi, and thanks for the review :)
A lot of surprising Compiz action happening...

> I'm a bit confused about this:
>
> 5 void
> 6 CubeaddonScreen::CubeCap::load (bool scale, bool aspect, bool clamp)
> 7 {
> 8 - if (mFiles.empty ())
> 9 - return;
> 10 -
>
> It seems like the point is to remove the early return if no images are
> available. However cubeaddon only loads images (to the best of my
> understanding). I'm pretty sure there's a risk of a segfault or
> std::out_of_range exeception just a few lines underneath it:
>
> mCurrent = mCurrent % mFiles.size ();
>
> CompString imgName = mFiles[mCurrent].s ();
> CompString pname = "cubeaddon";
> mTexture = GLTexture::readImageToTexture (imgName, pname, tSize);
>
> Was this change really needed?
>
Well, currently this cannot happen, because CubeaddonScreen::CubeCap::load
is just called by CubeaddonScreen::changeCap, where cap->mFiles.size ()
is already checked - no need to check it again, because this check already
happened a moment ago.

> 34 - cScreen->damageScreen ();
> 35 - }
>
> Was there a reason for this one? The function documentation says that we
> should be posting a full-screen damage at that point.
>
I was not sure about that one, but my logic tells me that we are already
rendering fullscreen (with fullscreen damage), when we show the cube -
but I can bring that back in.

> /*
> * Switch cap, load it and damage screen if possible
> */
> bool
> CubeaddonScreen::changeCap (bool top, int change)
>
> I'd imagine that's because if this function happened on a timer and there was
> no other damage or motion, we'd want the cap change to reflected as an
> immediate repaint.
>
Probably you are right, I'll bring it back in.
Unfortunately I could not test to see if any flickering occurs, because switching
top and bottom cap textures via keyboard shortcut is currently broken as well:
see bug #1162740

> 227 -
> 228 CubeaddonScreen::CubeaddonScreen (CompScreen *s) :
> 229 PluginClassHandler<CubeaddonScreen, CompScreen> (s),
> 230 CubeaddonOptions (),
> 231 @@ -1608,4 +1605,3 @@
> 232
> 233 return true;
> 234 }
> 235 -
>
> This is just a change in whitespace. Its not really a big problem, see if you
> can avoid them though because too many can clutter up diffs.

Unfortunately I found a lot of whitespaces, newlines and indentation problems
throughout the code...
IMHO we should fix those in the long run and beautify our code.
Existing code should also be an example how we want future code to be.
If indentation and style are a mess, code also is harder to read and bugs can
easier slip through undetected...

[Sam Spilsbury (smspillaz)]

On Tue, Apr 2, 2013 at 4:13 PM, MC Return <email address hidden> wrote:
> Hi, and thanks for the review :)
> A lot of surprising Compiz action happening...
>
>> I'm a bit confused about this:
>>
>> 5 void
>> 6 CubeaddonScreen::CubeCap::load (bool scale, bool aspect, bool clamp)
>> 7 {
>> 8 - if (mFiles.empty ())
>> 9 - return;
>> 10 -
>>
>> It seems like the point is to remove the early return if no images are
>> available. However cubeaddon only loads images (to the best of my
>> understanding). I'm pretty sure there's a risk of a segfault or
>> std::out_of_range exeception just a few lines underneath it:
>>
>> mCurrent = mCurrent % mFiles.size ();
>>
>> CompString imgName = mFiles[mCurrent].s ();
>> CompString pname = "cubeaddon";
>> mTexture = GLTexture::readImageToTexture (imgName, pname, tSize);
>>
>> Was this change really needed?
>>
> Well, currently this cannot happen, because CubeaddonScreen::CubeCap::load
> is just called by CubeaddonScreen::changeCap, where cap->mFiles.size ()
> is already checked - no need to check it again, because this check already
> happened a moment ago.

Okay. What I would suggest doing is dropping the former check instead
of the one inside CubeCap::load. That means that we don't need to have
one of CubeCap::load's preconditions be that cap->mFiles.size () is >
0. Instead, the check only happens once, and it happens inside of
CubeCap::load (which in fact, is a better design, because now the
caller doesn't need to care about how many files the cap has).

>
>> 34 - cScreen->damageScreen ();
>> 35 - }
>>
>> Was there a reason for this one? The function documentation says that we
>> should be posting a full-screen damage at that point.
>>
> I was not sure about that one, but my logic tells me that we are already
> rendering fullscreen (with fullscreen damage), when we show the cube -
> but I can bring that back in.
>
>> /*
>> * Switch cap, load it and damage screen if possible
>> */
>> bool
>> CubeaddonScreen::changeCap (bool top, int change)
>>
>> I'd imagine that's because if this function happened on a timer and there was
>> no other damage or motion, we'd want the cap change to reflected as an
>> immediate repaint.
>>
> Probably you are right, I'll bring it back in.
> Unfortunately I could not test to see if any flickering occurs, because switching
> top and bottom cap textures via keyboard shortcut is currently broken as well:
> see bug #1162740
>

Yes, we do render full-screen when the cube is active. However, we
don't post a fullscreen damage on every frame, we only post one if
something changes that requires us to update the scene. This would be
one of those things.

>> 227 -
>> 228 CubeaddonScreen::CubeaddonScreen (CompScreen *s) :
>> 229 PluginClassHandler<CubeaddonScreen, CompScreen> (s),
>> 230 CubeaddonOptions (),
>> 231 @@ -1608,4 +1605,3 @@
>> 232
>> 233 return true;
>> 234 }
>> 235 -
>>
>> This is just a change in whitespace. Its not really a big problem, see if you
>> can avoid them though because too many can clutter up diffs.
>
> Unfortunately I found a lot of whitespaces, newlines and indentation probl...

[MC Return (mc-return)]

> Okay. What I would suggest doing is dropping the former check instead
> of the one inside CubeCap::load. That means that we don't need to have
> one of CubeCap::load's preconditions be that cap->mFiles.size () is >
> 0. Instead, the check only happens once, and it happens inside of
> CubeCap::load (which in fact, is a better design, because now the
> caller doesn't need to care about how many files the cap has).
>
You're right. I'll look into that.

>
> Yes, we do render full-screen when the cube is active. However, we
> don't post a fullscreen damage on every frame, we only post one if
> something changes that requires us to update the scene. This would be
> one of those things.
>
I've corrected that now - sorry for the mistake.

[MC Return (mc-return)]

>
> Okay. What I would suggest doing is dropping the former check instead
> of the one inside CubeCap::load. That means that we don't need to have
> one of CubeCap::load's preconditions be that cap->mFiles.size () is >
> 0. Instead, the check only happens once, and it happens inside of
> CubeCap::load (which in fact, is a better design, because now the
> caller doesn't need to care about how many files the cap has).
>
Done in r3647.

Also:
Removed redundant mCurrent = mCurrent % mFiles.size (); calculation,
this has already been done in CubeaddonScreen::changeCap.

cap->mCurrent = (cap->mCurrent + change + count) % count;
could imho also be reduced to:
cap->mCurrent = (cap->mCurrent + change) % count;
because modulo will never return negative values AFAIK, but I kept it
for now...
Also we just need to do this calculation if count and change both are != 0

Bug #1162740 is still open, I suspect the problem is hidden here:

#define BIND_ACTION(opt, t, d) \
    optionSet##opt##Initiate (boost::bind (&CubeaddonScreen::changeCap, this, t, d))

    BIND_ACTION (TopNextKey, true, 1);
    BIND_ACTION (TopPrevKey, true, -1);
    BIND_ACTION (BottomNextKey, false, 1);
    BIND_ACTION (BottomNextKey, false, -1);

    BIND_ACTION (TopNextButton, true, 1);
    BIND_ACTION (TopPrevButton, true, -1);
    BIND_ACTION (BottomNextButton, false, 1);
    BIND_ACTION (BottomNextButton, false, -1);

#undef BIND_ACTION

I could not get the plugin to send those values to changeCap :(
Do you have an idea what could be wrong with that, Sam ?

review: Needs Information

[Sam Spilsbury (smspillaz)]

> >
> > Okay. What I would suggest doing is dropping the former check instead
> > of the one inside CubeCap::load. That means that we don't need to have
> > one of CubeCap::load's preconditions be that cap->mFiles.size () is >
> > 0. Instead, the check only happens once, and it happens inside of
> > CubeCap::load (which in fact, is a better design, because now the
> > caller doesn't need to care about how many files the cap has).
> >
> Done in r3647.
>

Thanks, +1
>
> Bug #1162740 is still open, I suspect the problem is hidden here:
>
> #define BIND_ACTION(opt, t, d) \
> optionSet##opt##Initiate (boost::bind (&CubeaddonScreen::changeCap, this,
> t, d))
>
> BIND_ACTION (TopNextKey, true, 1);
> BIND_ACTION (TopPrevKey, true, -1);
> BIND_ACTION (BottomNextKey, false, 1);
> BIND_ACTION (BottomNextKey, false, -1);
>
> BIND_ACTION (TopNextButton, true, 1);
> BIND_ACTION (TopPrevButton, true, -1);
> BIND_ACTION (BottomNextButton, false, 1);
> BIND_ACTION (BottomNextButton, false, -1);
>
> #undef BIND_ACTION
>
> I could not get the plugin to send those values to changeCap :(
> Do you have an idea what could be wrong with that, Sam ?

Is it that changeCap is never called or is it that the values for "top" and "change" in CubeaddonScreen::changeCap are never different for each keybinding.

If its the former, try binding the action to another keybinding, eg Control-Super-a. It might be that we don't allow actions to be bound on non-modified keybindings.

[MC Return (mc-return)]

>
> Is it that changeCap is never called or is it that the values for "top" and
> "change" in CubeaddonScreen::changeCap are never different for each
> keybinding.
>
> If its the former, try binding the action to another keybinding, eg Control-
> Super-a. It might be that we don't allow actions to be bound on non-modified
> keybindings.

It is the former, but I tried all kinds of mouse and keyboard combos.
I can confirm that keys get recognized during the cube -> for example I can
start the rotating cube screensaver and then do all kinds of things like
moving windows from viewport to viewport, minimizing/maximizing windows,
revealing/hiding Guake or type into a console...

Just those cubeaddon ones are striking :(
I really suspect the code I posted above to be the culprit...

[MC Return (mc-return)]

Unfortunately I found one other bug in cube-addon:
The deformation stops during the time windows are animated :(

[Sam Spilsbury (smspillaz)]

On Tue, Apr 2, 2013 at 11:52 PM, MC Return <email address hidden> wrote:
>>
> It is the former, but I tried all kinds of mouse and keyboard combos.
> I can confirm that keys get recognized during the cube -> for example I can
> start the rotating cube screensaver and then do all kinds of things like
> moving windows from viewport to viewport, minimizing/maximizing windows,
> revealing/hiding Guake or type into a console...

Is it just during the screensaver where the keybindings don't work?

>
> Just those cubeaddon ones are striking :(
> I really suspect the code I posted above to be the culprit...
> --
> https://code.launchpad.net/~mc-return/compiz/compiz.merge-fix1162484-original-cube-top-and-bottom-caps-not-deformed/+merge/156469
> Your team Compiz Maintainers is subscribed to branch lp:compiz.

--
Sam Spilsbury

[MC Return (mc-return)]

>
> Is it just during the screensaver where the keybindings don't work?
>
No. Do they work for you ?

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:3647
http://jenkins.qa.ubuntu.com/job/compiz-ci/115/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-gles-ci/./build=pbuilder,distribution=raring,flavor=amd64/152/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-pbuilder/./build=pbuilder,distribution=raring,flavor=amd64/504/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/compiz-ci/115/rebuild

review: Needs Fixing (continuous-integration)

[MC Return (mc-return)]

The Jenkins failures seem to be unrelated to this MP...

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:3648
http://jenkins.qa.ubuntu.com/job/compiz-ci/119/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-gles-ci/./build=pbuilder,distribution=raring,flavor=amd64/156/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-pbuilder/./build=pbuilder,distribution=raring,flavor=amd64/508/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/compiz-ci/119/rebuild

review: Needs Fixing (continuous-integration)

[Sam Spilsbury (smspillaz)]

> >
> > Is it just during the screensaver where the keybindings don't work?
> >
> No. Do they work for you ?

I haven't tried. But I suspect that the screensaver plugin doesn't take a grab when it activates, which would mean that single-key-only bindings are unlikely to work the way you might expect them to.

I don't think there's a "bug" here if the keybindings work whilst manually rotating the cube

[MC Return (mc-return)]

>
> I haven't tried. But I suspect that the screensaver plugin doesn't take a grab
> when it activates, which would mean that single-key-only bindings are unlikely
> to work the way you might expect them to.
>
> I don't think there's a "bug" here if the keybindings work whilst manually
> rotating the cube

Those keyboard/mouse shortcuts *never* work. I shouldn't have mentioned screensaver...

[MC Return (mc-return)]

Ping ?

[Sam Spilsbury (smspillaz)]

This is good as is. Try to avoid mixing whitespace change and variable initialization changes with substantive changes in future though, as it tends to confuse the diffs.

review: Approve

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'plugins/cubeaddon/src/cubeaddon.cpp'
--- plugins/cubeaddon/src/cubeaddon.cpp	2013-02-25 19:34:51 +0000
+++ plugins/cubeaddon/src/cubeaddon.cpp	2013-04-04 04:10:28 +0000
@@ -44,8 +44,15 @@
 void
 CubeaddonScreen::CubeCap::load (bool scale, bool aspect, bool clamp)
 {
+    /* we need to clear the texture, if no texture files are specified */
     if (mFiles.empty ())
+    {
+	mTexture.clear ();
+	mLoaded = false;
+	mCurrent = 0;
+
 	return;
+    }
 
     CompSize tSize;
     float    xScale, yScale;
@@ -56,9 +63,6 @@
 
     mLoaded = false;
 
-    mCurrent = mCurrent % mFiles.size ();
-
-
     CompString imgName = mFiles[mCurrent].s ();
     CompString pname = "cubeaddon";
     mTexture = GLTexture::readImageToTexture (imgName, pname, tSize);
@@ -143,23 +147,25 @@
 CubeaddonScreen::changeCap (bool top, int change)
 {
     CubeCap *cap = (top)? &mTopCap : &mBottomCap;
-    if (cap->mFiles.size ())
-    {
-	int count = cap->mFiles.size ();
+
+    int count = cap->mFiles.size ();
+
+    /* mCurrent just changes in this case */
+    if (count && change)
 	cap->mCurrent = (cap->mCurrent + change + count) % count;
-	if (top)
-	{
-	    cap->load (optionGetTopScale (), optionGetTopAspect (),
-		       optionGetTopClamp ());
-	}
-	else
-	{
-	    cap->load (optionGetBottomScale (), optionGetBottomAspect (),
-		       optionGetBottomClamp ());
-	    cap->mTexMat.scale (1.0, -1.0, 1.0);
-	}
-	cScreen->damageScreen ();
-    }
+
+    if (top)
+    {
+	cap->load (optionGetTopScale (), optionGetTopAspect (),
+		   optionGetTopClamp ());
+    }
+    else
+    {
+	cap->load (optionGetBottomScale (), optionGetBottomAspect (),
+		   optionGetBottomClamp ());
+	cap->mTexMat.scale (1.0, -1.0, 1.0);
+    }
+    cScreen->damageScreen ();
 
     return false;
 }
@@ -167,7 +173,6 @@
 bool
 CubeaddonScreen::setOption (const CompString &name, CompOption::Value &value)
 {
-
     unsigned int index;
 
     bool rv = CubeaddonOptions::setOption (name, value);
@@ -206,11 +211,9 @@
 void
 CubeaddonScreen::drawBasicGround ()
 {
-    float i;
-
     glEnable (GL_BLEND);
     glBlendFunc (GL_SRC_ALPHA, GL_ONE_MINUS_SRC_ALPHA);
-    i = optionGetIntensity () * 2;
+    float i = optionGetIntensity () * 2;
 
     GLMatrix transform;
     transform.translate (0.0f, 0.0f, -DEFAULT_Z_CAMERA);
@@ -272,10 +275,8 @@
 				       CompOutput                *output,
 				       std::vector<GLVector>     &points)
 {
-    bool status;
-
-    status = cubeScreen->cubeCheckOrientation (sAttrib, transform,
-					       output, points);
+    bool status = cubeScreen->cubeCheckOrientation (sAttrib, transform,
+						    output, points);
 
     if (mReflection)
 	return !status;
@@ -311,14 +312,14 @@
 	glCullFace (GL_FRONT);
 }
 
-bool 
+bool
 CubeaddonScreen::cubeShouldPaintViewport (const GLScreenPaintAttrib &sAttrib,
 					  const GLMatrix            &transform,
 					  CompOutput                *output,
 					  PaintOrder                order)
 {
     bool rv = cubeScreen->cubeShouldPaintViewport (sAttrib, transform,
-					      output, order);
+						   output, order);
 
     if (rv || cubeScreen->unfolded ())
 	return rv;
@@ -326,15 +327,14 @@
     if (mDeform > 0.0 && optionGetDeformation () == DeformationCylinder)
     {
 	float z[3];
-	bool  ftb1, ftb2, ftb3;
 
 	z[0] = cubeScreen->invert () * cubeScreen->distance ();
 	z[1] = z[0] + (0.25 / cubeScreen->distance ());
-	z[2] = cubeScreen->invert () * 
+	z[2] = cubeScreen->invert () *
 	       sqrtf (0.25 + (cubeScreen->distance () * cubeScreen->distance ()));
 
 	std::vector<GLVector> vPoints[3];
-	
+
 	vPoints[0].push_back (GLVector (-0.5,  0.0, z[0], 1.0));
 	vPoints[0].push_back (GLVector ( 0.0,  0.5, z[1], 1.0));
 	vPoints[0].push_back (GLVector ( 0.0,  0.0, z[1], 1.0));
@@ -345,12 +345,12 @@
 	vPoints[2].push_back (GLVector ( 0.0,  0.5, z[2], 1.0));
 	vPoints[2].push_back (GLVector ( 0.0,  0.0, z[2], 1.0));
 
-	ftb1 = cubeScreen->cubeCheckOrientation (sAttrib, transform,
-					         output, vPoints[0]);
-	ftb2 = cubeScreen->cubeCheckOrientation (sAttrib, transform,
-					         output, vPoints[1]);
-	ftb3 = cubeScreen->cubeCheckOrientation (sAttrib, transform,
-					         output, vPoints[2]);
+	bool ftb1 = cubeScreen->cubeCheckOrientation (sAttrib, transform,
+						      output, vPoints[0]);
+	bool ftb2 = cubeScreen->cubeCheckOrientation (sAttrib, transform,
+						      output, vPoints[1]);
+	bool ftb3 = cubeScreen->cubeCheckOrientation (sAttrib, transform,
+						      output, vPoints[2]);
 
 	rv = (order == FTB && (ftb1 || ftb2 || ftb3)) ||
 	     (order == BTF && (!ftb1 || !ftb2 || !ftb3));
@@ -358,15 +358,14 @@
     else if (mDeform > 0.0 && optionGetDeformation () == DeformationSphere)
     {
 	float z[4];
-	bool  ftb1, ftb2, ftb3, ftb4;
 
 	z[0] = sqrtf (0.5 + (cubeScreen->distance () * cubeScreen->distance ()));
 	z[1] = z[0] + (0.25 / cubeScreen->distance ());
 	z[2] = sqrtf (0.25 + (cubeScreen->distance () * cubeScreen->distance ()));
 	z[3] = z[2] + 0.5;
-	
+
 	std::vector<GLVector> vPoints[4];
-	
+
 	vPoints[0].push_back (GLVector ( 0.0,  0.0, z[3], 1.0));
 	vPoints[0].push_back (GLVector (-0.5,  0.5, z[2], 1.0));
 	vPoints[0].push_back (GLVector ( 0.0,  0.5, z[2], 1.0));
@@ -380,14 +379,14 @@
 	vPoints[3].push_back (GLVector ( 0.5,  0.5, z[0], 1.0));
 	vPoints[3].push_back (GLVector ( 0.5,  0.0, z[0], 1.0));
 
-	ftb1 = cubeScreen->cubeCheckOrientation (sAttrib, transform,
-						 output, vPoints[0]);
-	ftb2 = cubeScreen->cubeCheckOrientation (sAttrib, transform,
-						 output, vPoints[1]);
-	ftb3 = cubeScreen->cubeCheckOrientation (sAttrib, transform,
-						 output, vPoints[2]);
-	ftb4 = cubeScreen->cubeCheckOrientation (sAttrib, transform,
-						 output, vPoints[3]);
+	bool ftb1 = cubeScreen->cubeCheckOrientation (sAttrib, transform,
+						      output, vPoints[0]);
+	bool ftb2 = cubeScreen->cubeCheckOrientation (sAttrib, transform,
+						      output, vPoints[1]);
+	bool ftb3 = cubeScreen->cubeCheckOrientation (sAttrib, transform,
+						      output, vPoints[2]);
+	bool ftb4 = cubeScreen->cubeCheckOrientation (sAttrib, transform,
+						      output, vPoints[3]);
 
 	rv = (order == FTB && (ftb1 || ftb2 || ftb3 || ftb4)) ||
 	     (order == BTF && (!ftb1 || !ftb2 || !ftb3 || !ftb4));
@@ -406,7 +405,6 @@
 {
     GLScreenPaintAttrib sa;
     GLMatrix            sTransform;
-    int                 i, l, opacity;
     int                 cullNorm, cullInv;
     bool                wasCulled = glIsEnabled (GL_CULL_FACE);
     float               cInv = (top) ? 1.0: -1.0;
@@ -416,12 +414,13 @@
 
     glGetIntegerv (GL_CULL_FACE_MODE, &cullNorm);
     cullInv   = (cullNorm == GL_BACK)? GL_FRONT : GL_BACK;
+
     if (top)
 	color = cubeScreen->topColor ();
     else
 	color = cubeScreen->bottomColor ();
 
-    opacity = cubeScreen->desktopOpacity () * color[3] / 0xffff;
+    int opacity = cubeScreen->desktopOpacity () * color[3] / 0xffff;
 
     glEnable (GL_BLEND);
 
@@ -438,11 +437,11 @@
 
     glEnable(GL_CULL_FACE);
 
-    for (l = 0; l < ((cubeScreen->invert () == 1) ? 2 : 1); l++)
+    for (int l = 0; l < ((cubeScreen->invert () == 1) ? 2 : 1); l++)
     {
 	glCullFace(((l == 1) ^ top) ? cullInv : cullNorm);
 
-	for (i = 0; i < size; i++)
+	for (int i = 0; i < size; i++)
 	{
 	    GLVertexBuffer *streamingBuffer = GLVertexBuffer::streamingBuffer ();
 
@@ -688,17 +687,18 @@
 			       int                       size,
 			       const GLVector            &normal)
 {
-    if ((!optionGetDrawBottom () && cubeScreen->invert () == -1) ||
-        (!optionGetDrawTop () && cubeScreen->invert () == 1))
-    {
+    /* we do not want to draw anything if this option is disabled */
+    if (!optionGetDrawTop ())
+	return;
+
+    if (((!optionGetDrawBottom () && cubeScreen->invert () == -1) ||
+	 (!optionGetDrawTop () && cubeScreen->invert () == 1)) &&
+	/* the original top cap will work for the non-deformed cube only */
+	(optionGetDeformation () == DeformationNone))
 	cubeScreen->cubePaintTop (sAttrib, transform, output, size, normal);
-    }
-
-    if (!optionGetDrawTop ())
-        return;
-
-    paintCap (sAttrib, transform, output, size,
-	      true, optionGetAdjustTop ());
+    else
+	paintCap (sAttrib, transform, output, size,
+		  true, optionGetAdjustTop ());
 }
 
 void 
@@ -708,16 +708,17 @@
 				  int                       size,
 				  const GLVector            &normal)
 {
-    if ((!optionGetDrawBottom () && cubeScreen->invert () == 1) ||
-        (!optionGetDrawTop () && cubeScreen->invert () == -1))
-    {
+    /* we do not want to draw anything if this option is disabled */
+    if (!optionGetDrawBottom ())
+	return;
+
+    if (((!optionGetDrawBottom () && cubeScreen->invert () == 1) ||
+	 (!optionGetDrawTop () && cubeScreen->invert () == -1)) &&
+	/* the original bottom cap will work for the non-deformed cube only */
+	(optionGetDeformation () == DeformationNone))
 	cubeScreen->cubePaintBottom (sAttrib, transform, output, size, normal);
-    }
-
-    if (!optionGetDrawBottom ())
-        return;
-
-    paintCap (sAttrib, transform, output, size,
+    else
+	paintCap (sAttrib, transform, output, size,
 	      false, optionGetAdjustBottom ());
 }
 
@@ -911,7 +912,6 @@
     if (!(mask & PAINT_WINDOW_TRANSFORMED_MASK) && caScreen->mDeform)
     {
 	CompPoint offset;
-	int x1, x2;
 
 	if (!window->onAllViewports ())
 	{
@@ -919,8 +919,8 @@
 	    offset = window->getMovementForOffset (offset);
 	}
 	
-	x1 = window->x () - window->output ().left + offset.x ();
-	x2 = window->x () + window->width () + window->output ().right + offset.x ();
+	int x1 = window->x () - window->output ().left + offset.x ();
+	int x2 = window->x () + window->width () + window->output ().right + offset.x ();
 	if (x1 < 0 && x2 < 0)
 	    return false;
 	if (x1 > screen->width () && x2 > screen->width ())
@@ -941,16 +941,15 @@
 	int       i;
 	int       sx1, sx2, sw, sy1, sy2, sh;
 	int       offX = 0, offY = 0;
-	float     x, y, ym;
+	float     x, y;
 	GLfloat   *v, *n;
-	float     inv;
 	
 	GLVertexBuffer               *vb = gWindow->vertexBuffer ();
 	CubeScreen::MultioutputMode  cMOM = cubeScreen->multioutputMode ();
 	float                        cDist = cubeScreen->distance ();
 
-	inv = (cubeScreen->invert () == 1) ? 1.0: -1.0;
-	ym  = (caScreen->optionGetDeformation () == CubeaddonScreen::DeformationCylinder) ? 0.0 : 1.0;
+	float inv = (cubeScreen->invert () == 1) ? 1.0: -1.0;
+	float ym  = (caScreen->optionGetDeformation () == CubeaddonScreen::DeformationCylinder) ? 0.0 : 1.0;
 	
 	int vertexCount = vb->countVertices ();
 
@@ -1159,7 +1158,6 @@
 	}
 	else
 	{
-	    int i;
 	    float w;
 	    rS = cDist2 + 0.5;
 
@@ -1170,7 +1168,7 @@
 	    mCapFillNorm[1] = -1.0;
 	    mCapFillNorm[2] = 0.0;
 
-	    for (i = 0; i < CAP_ELEMENTS; i++)
+	    for (int i = 0; i < CAP_ELEMENTS; i++)
 	    {
 		w = (float)(i + 1) / (float)CAP_ELEMENTS;
 
@@ -1281,20 +1279,18 @@
 		    mYTrans    = 0.0;
 		    if (optionGetDeformation () == DeformationSphere &&
 			optionGetDeformCaps () && optionGetDrawBottom ())
-		    {
 			rYTrans = sqrt (0.5 + cDist2) * -2.0;
-		    }
 		    else
-		    {
 			rYTrans = point[1] * 2.0;
-		    }
+
 		    break;
+
 		case ModeDistance:
 		    mYTrans = 0.0;
 		    rYTrans = sqrt (0.5 + cDist2) * -2.0;
 		    break;
+
 		default:
-
 		    if (optionGetDeformation () == DeformationSphere &&
 			optionGetDeformCaps () && optionGetDrawBottom ())
 		    {
@@ -1315,13 +1311,10 @@
 		    break;
 	    }
 
-
 	    if (!optionGetAutoZoom () ||
 		((cubeScreen->rotationState () != CubeScreen::RotationManual) &&
 		 optionGetZoomManualOnly ()))
-	    {
 		mZTrans = 0.0;
-	    }
 	    else
 		mZTrans = -point2[2] + cDist;
 
@@ -1377,8 +1370,6 @@
 
 	    if (optionGetMode () == ModeAbove && mVRot > 0.0)
 	    {
-		int   j;
-		float i, c;
 		float v = MIN (1.0, mVRot / 30.0);
 		unsigned short col1[4], col2[4];
 
@@ -1391,8 +1382,8 @@
 
 		gTransform.translate (0, 0, -DEFAULT_Z_CAMERA);
 
-		i = optionGetIntensity () * 2;
-		c = optionGetIntensity ();
+		float i = optionGetIntensity () * 2;
+		float c = optionGetIntensity ();
 
 		GLfloat vertices[] =
 		{
@@ -1419,7 +1410,7 @@
 		if (streamingBuffer->end ())
 		    streamingBuffer->render (gTransform);
 
-		for (j = 0; j < 4; j++)
+		for (int j = 0; j < 4; j++)
 		{
 		    col1[j] = (1.0 - v) * optionGetGroundColor1 () [j] +
 			      (v * (optionGetGroundColor1 () [j] +
@@ -1516,7 +1507,6 @@
     cScreen->donePaint ();
 }
 
-
 CubeaddonScreen::CubeaddonScreen (CompScreen *s) :
     PluginClassHandler<CubeaddonScreen, CompScreen> (s),
     CubeaddonOptions (),
@@ -1538,7 +1528,6 @@
 
     idx = mCapFillIdx;
     for (int i = 0; i < CAP_ELEMENTS - 1; i++)
-    {
 	for (int j = 0; j < CAP_ELEMENTS; j++)
 	{
 	    idx[0] = 1 + (i * (CAP_ELEMENTS + 1)) + j;
@@ -1547,7 +1536,6 @@
 	    idx[3] = 2 + (i * (CAP_ELEMENTS + 1)) + j;
 	    idx += 4;
 	}
-    }
 
     mTopCap.mFiles = optionGetTopImages ();
     mBottomCap.mFiles = optionGetBottomImages ();
@@ -1608,4 +1596,3 @@
 
     return true;
 }
-