Code review comment for lp:~mc-return/compiz/compiz.merge-cube-speed-improvements

Ups - somehow I missed this review...

> 149 - memset (mCleared, 0, sizeof (Bool) * screen->outputDevs ().size
> ());
> 150 + memset (mCleared, 0, sizeof (Bool) * screen->outputDevs
> ().size ());
>
This is really badly aligned. Will fix it.

> 283 - if (xMoveAdd < -hsize / 2)
> 284 + if (xMoveAdd < -halfHsize)
> 285 xMoveAdd += hsize;
>
This has to be viewed together with the else part to make sense, but
I agree it could look better, but in this case really helps readability:

 if (xMoveAdd < -halfHsize)
     xMoveAdd += hsize;
 else if (xMoveAdd > halfHsize)
     xMoveAdd -= hsize;

> 473 GLScreenPaintAttrib sa = sAttrib;
> 474 - sa.xRotate += xRotate;
> 475 - sa.vRotate += vRotate;
> 476 + sa.xRotate += xRotate;
> 477 + sa.vRotate += vRotate;
>

I will revert the 47x changes, they really do not help much.

> While whitespace alignment can sometimes be helpful, use it judicially.
> Typically, it makes a lot of sense in mutliple-assignment type scenarios.
> Otherwise it can become distracting. I've picked out some examples where I
> think such alignment hurts more than it helps.
>

Yes, sometimes I try to be too perfectionistic, but I try my best to judge
carefully between useful and useless aligning.

> 490 - /* distance we move the camera back when unfolding the cube.
> 491 + /* TODO: Distance we move the camera back when unfolding the
> cube.
> 492 currently hardcoded to 1.5 but it should probably be
> optional. */
>
> The relevant todo is the fact that it could be optional, not the distance the
> cube is being moved.
>
Yes, you are right -> this could be misinterpreted... I'll fix it.

> The other changes are mostly fine for now.
>
+1. Thanks for your review.

> Something to keep in mind: The linked bug report about the redundant state
> changes goes more towards the fundamental architecture of compiz rather than
> something that can be fixed on a case by case basis. Currently the only
> mechanism that we have in order to avoid such calls into OpenGL is effectively
> more calls into OpenGL by querying the state, which in some cases can actually
> be worse than the state change because it requires looking up the current
> state of the pipeline as it might have been modified internally.
>
Well, I have not found a better way to do that faster...

> In this case, it doesn't really matter too much - we check to see if a
> particular state was enabled or disabled before doing both (eg, if enabled,
> leave it enabled and don't disable it later, if not enabled, disable it
> later).

This should be the fastest and our only way to do it correctly...
The plan of course is to do that for all global OpenGL state changes, not only
for GL_BLEND...
But GL_BLEND is a state that is often used by multiple plugins at the same time,
especially when activating the cube, so instead of enabling/disabling blending
redundantly in each plugin (for example: cube+cubeaddon+gears+rotate) we just do
it once and skip the other state changes...
I think this should be much more efficient, but if you have other ideas I would
be happy to hear those...