'Re: Review Request: Introduce a helper class to automatically push/pop Shaders'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kwin
Subject:    Re: Review Request: Introduce a helper class to automatically push/pop Shaders
From:       "Commit Hook" <null () kde ! org>
Date:       2012-09-29 13:31:34
Message-ID: 20120929133134.32082.85812 () vidsolbach ! de
[Download RAW message or body]

[Attachment #2 (multipart/alternative)]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106521/#review19577
-----------------------------------------------------------

This review has been submitted with commit 3f2d3e0f62abc0e81afd691e8a7c5a6c=
463f5875 by Martin Gr=C3=A4=C3=9Flin to branch master.

- Commit Hook

On Sept. 28, 2012, 2:18 p.m., Martin Gr=C3=A4=C3=9Flin wrote:
> =

> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106521/
> -----------------------------------------------------------
> =

> (Updated Sept. 28, 2012, 2:18 p.m.)
> =

> =

> Review request for kwin.
> =

> =

> Description
> -------
> =

> Introduce a helper class to automatically push/pop Shaders
> =

> The ShaderBinder class can be used for the case that a block of code
> should be executed with a given Shader being bound. This is useful for
> all the cases where there is a if-block for OpenGL2 execution with a
> Shader being pushed in the first line to the ShaderManager and popped in
> the last line of the block. With the helper this can be simplified to:
> =

> ShaderBinder binder(myCustomShader);
> =

> or
> =

> ShaderBinder binder(ShaderManager::GenericShader);
> =

> The ctor of ShaderBinder pushes the given Shader to the stack and once
> the helper goes out of scope it will be popped again from the stack.
> =

> In addition the helper can take care of OpenGL 1 compositing, that is it
> just does nothing. So it can also be used where there is a shared OpenGL1
> and OpenGL2 code path where the Shader should only be pushed in OpenGL2.
> This basically removes all the checks for the compositing type before
> pushing/popping a Shader to the stack.
> =

> REVIEW: 106521
> =

> Do not use ShaderManager::isValid to check for OpenGL2 compositing
> =

> The main usage of ShaderManager::isValid was to have OpenGL2 specific
> code pathes. Now we have an actual OpenGL2Compositing type and we know
> that the ShaderManager is valid if we have this compositing type and we
> know that it is not valid on OpenGL1Compositing. This gives us a much
> better check and allows us to use the isValid method just for where we
> want to check whether the shaders compiled successfully.
> =

> In addition some effects require OpenGL2, so we do not need to check
> again that the ShaderManager is valid. Such usages are removed.
> =

> Introduce dedicated OpenGL1 and OpenGL2 compositing types
> =

> The CompositingType enum turns into flags and two new values are
> introduced: OpenGL1Compositing and OpenGL2Compositing.
> =

> Those new values are or-ed to OpenGLCompositing so that a simple check
> for the flag OpenGLCompositing works in case of one of those two new
> values. To make the generic check for OpenGL compositing easier a method
> in EffectsHandler is introduced to just check for this.
> =

> The scenes now return either OpenGL1Compositing or OpenGL2Compositing
> depending on which Scene implementation. None returns OpenGLCompositing.
> =

> =

> Diffs
> -----
> =

>   kwin/client.cpp fefdb4036c46a85566962748e04809316fec6445 =

>   kwin/composite.cpp 11958961a74593b23744dfcbff1397da972a551b =

>   kwin/effects.cpp dabb96aeca162302cc4c6ecea475723ba23b8196 =

>   kwin/effects/blur/blur.cpp 2b32b3a49cc9da3a012bb21e1ca6ec19fe14e5d2 =

>   kwin/effects/blur/blurshader.cpp 1564c5a7f2a5908bb92523e2e66f48796eb31a=
44 =

>   kwin/effects/coverswitch/coverswitch.cpp 22c9576a69990ec6a419321aa27a44=
989ccb29af =

>   kwin/effects/cube/cube.cpp 4b1391776d53d91b51f7567417e64f047e90943a =

>   kwin/effects/cube/cubeslide.cpp 1d0f02fc73bacefeddc289e1cab41652491ad67=
c =

>   kwin/effects/explosion/explosion.cpp 15fdc6e0a1793d74aacbd8be86e83a5fcb=
bdbfda =

>   kwin/effects/flipswitch/flipswitch.cpp a88a6d3933e54ca6f9d7cf9d2ccb556b=
3c939c38 =

>   kwin/effects/glide/glide.cpp 540844fece0b15a1be944a4fa1b248f6fd1f1987 =

>   kwin/effects/invert/invert.cpp 258e2ecadd01783c082f1dbcaafab24a22e2471e =

>   kwin/effects/logout/logout.cpp a78460d1bbf0348aa6da884e071d7eb06277192f =

>   kwin/effects/lookingglass/lookingglass.cpp c532fe1a3d814dfabf43cd57e2e3=
dba10b3510fe =

>   kwin/effects/magiclamp/magiclamp.cpp c151ed1175f96d7572ebf40c32e741e4a1=
ee44a8 =

>   kwin/effects/magnifier/magnifier.cpp 0103257fbd104f6b8685f9a4a500ae6447=
e5b63c =

>   kwin/effects/mousemark/mousemark.cpp 0a3229b3ec0cd256d883bf6d219cff37d7=
72c64f =

>   kwin/effects/presentwindows/presentwindows.cpp 96d9a68a27884a48592183b4=
735f6a50e5b97e3f =

>   kwin/effects/resize/resize.cpp 410b6748f6b82a0164bc5bed78e4f6785010a400 =

>   kwin/effects/screenshot/screenshot.cpp ea56e80c8b6443a2d09b024396d5f78c=
f098a0b1 =

>   kwin/effects/sheet/sheet.cpp 58af9b4c0d0536de0637a2b741c81450ffb18263 =

>   kwin/effects/showfps/showfps.cpp b3dd9b74943becfb1545f3159bb5aec8d8efb3=
1e =

>   kwin/effects/showpaint/showpaint.cpp c17178ce54beb4c77c45c2688b2fdc9630=
c64381 =

>   kwin/effects/snaphelper/snaphelper.cpp 7d4be144b4041b61e547b0aaa74c7b80=
d3c38314 =

>   kwin/effects/startupfeedback/startupfeedback.cpp cab7d43dcfd765bd240063=
22de3436b88035302e =

>   kwin/effects/taskbarthumbnail/taskbarthumbnail.cpp 0c67a3b84a72b22e9b6d=
1f6b9e34db8c3445962e =

>   kwin/effects/trackmouse/trackmouse.cpp cf6575456a03259fb8de10b2792291ed=
cb06ea1a =

>   kwin/effects/wobblywindows/wobblywindows.cpp dbfbdc35167dfb9301aadd15ae=
653c031d1868a6 =

>   kwin/effects/zoom/zoom.cpp 587de0328d2390b2200e024dbff7f90c24d68308 =

>   kwin/lanczosfilter.cpp 654d2cf95ad5da4707bab7fef9a0019702c57d0f =

>   kwin/libkwineffects/kwineffects.h 2c2f7bf5edae3c0f93e255833faab842939f6=
3d1 =

>   kwin/libkwineffects/kwineffects.cpp bae85e79b5b38314e3eb964dd6922bf6252=
8c3e2 =

>   kwin/libkwineffects/kwinglobals.h 689fe4da2a1604bee9d2022377ce7f7940105=
a53 =

>   kwin/libkwineffects/kwinglutils.h 14fdd5ba9b5e645eb839cc530e7f6b1d91007=
589 =

>   kwin/scene_opengl.h 8ff0006a650b47f426fb775e7681724f2315e513 =

>   kwin/scene_opengl.cpp 3e4290a556e58cf5580278b554d2f199bb2c9a60 =

>   kwin/shadow.cpp de989c7955fa267aa533f40126066e6e0ccb26d9 =

>   kwin/workspace.cpp 5b37aee2bc85181ed2b11bd00c21e38fd61a38dd =

> =

> Diff: http://git.reviewboard.kde.org/r/106521/diff/
> =

> =

> Testing
> -------
> =

> running the code right now and seems to work
> =

> =

> Thanks,
> =

> Martin Gr=C3=A4=C3=9Flin
> =

>

[Attachment #5 (text/html)]

<html>
 <body>
  <div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
   <table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;">  <tr>
     <td>
      This is an automatically generated e-mail. To reply, visit:
      <a href="http://git.reviewboard.kde.org/r/106521/">http://git.reviewboard.kde.org/r/106521/</a>
  </td>
    </tr>
   </table>
   <br />

 <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This review has been \
submitted with commit 3f2d3e0f62abc0e81afd691e8a7c5a6c463f5875 by Martin Gräßlin to \
branch master.</pre>  <br />

<p>- Commit</p>

<br />
<p>On September 28th, 2012, 2:18 p.m., Martin Gräßlin wrote:</p>

<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;">  <tr>
  <td>

<div>Review request for kwin.</div>
<div>By Martin Gräßlin.</div>

<p style="color: grey;"><i>Updated Sept. 28, 2012, 2:18 p.m.</i></p>

<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
 <table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0">  <tr>
  <td>
   <pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Introduce a helper class to automatically push/pop Shaders

The ShaderBinder class can be used for the case that a block of code
should be executed with a given Shader being bound. This is useful for
all the cases where there is a if-block for OpenGL2 execution with a
Shader being pushed in the first line to the ShaderManager and popped in
the last line of the block. With the helper this can be simplified to:

ShaderBinder binder(myCustomShader);

or

ShaderBinder binder(ShaderManager::GenericShader);

The ctor of ShaderBinder pushes the given Shader to the stack and once
the helper goes out of scope it will be popped again from the stack.

In addition the helper can take care of OpenGL 1 compositing, that is it
just does nothing. So it can also be used where there is a shared OpenGL1
and OpenGL2 code path where the Shader should only be pushed in OpenGL2.
This basically removes all the checks for the compositing type before
pushing/popping a Shader to the stack.

REVIEW: 106521

Do not use ShaderManager::isValid to check for OpenGL2 compositing

The main usage of ShaderManager::isValid was to have OpenGL2 specific
code pathes. Now we have an actual OpenGL2Compositing type and we know
that the ShaderManager is valid if we have this compositing type and we
know that it is not valid on OpenGL1Compositing. This gives us a much
better check and allows us to use the isValid method just for where we
want to check whether the shaders compiled successfully.

In addition some effects require OpenGL2, so we do not need to check
again that the ShaderManager is valid. Such usages are removed.

Introduce dedicated OpenGL1 and OpenGL2 compositing types

The CompositingType enum turns into flags and two new values are
introduced: OpenGL1Compositing and OpenGL2Compositing.

Those new values are or-ed to OpenGLCompositing so that a simple check
for the flag OpenGLCompositing works in case of one of those two new
values. To make the generic check for OpenGL compositing easier a method
in EffectsHandler is introduced to just check for this.

The scenes now return either OpenGL1Compositing or OpenGL2Compositing
depending on which Scene implementation. None returns OpenGLCompositing.</pre>
  </td>
 </tr>
</table>

<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0">  <tr>
  <td>
   <pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">running the code right now and seems to work</pre>  </td>
 </tr>
</table>

<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">

 <li>kwin/client.cpp <span style="color: \
grey">(fefdb4036c46a85566962748e04809316fec6445)</span></li>

 <li>kwin/composite.cpp <span style="color: \
grey">(11958961a74593b23744dfcbff1397da972a551b)</span></li>

 <li>kwin/effects.cpp <span style="color: \
grey">(dabb96aeca162302cc4c6ecea475723ba23b8196)</span></li>

 <li>kwin/effects/blur/blur.cpp <span style="color: \
grey">(2b32b3a49cc9da3a012bb21e1ca6ec19fe14e5d2)</span></li>

 <li>kwin/effects/blur/blurshader.cpp <span style="color: \
grey">(1564c5a7f2a5908bb92523e2e66f48796eb31a44)</span></li>

 <li>kwin/effects/coverswitch/coverswitch.cpp <span style="color: \
grey">(22c9576a69990ec6a419321aa27a44989ccb29af)</span></li>

 <li>kwin/effects/cube/cube.cpp <span style="color: \
grey">(4b1391776d53d91b51f7567417e64f047e90943a)</span></li>

 <li>kwin/effects/cube/cubeslide.cpp <span style="color: \
grey">(1d0f02fc73bacefeddc289e1cab41652491ad67c)</span></li>

 <li>kwin/effects/explosion/explosion.cpp <span style="color: \
grey">(15fdc6e0a1793d74aacbd8be86e83a5fcbbdbfda)</span></li>

 <li>kwin/effects/flipswitch/flipswitch.cpp <span style="color: \
grey">(a88a6d3933e54ca6f9d7cf9d2ccb556b3c939c38)</span></li>

 <li>kwin/effects/glide/glide.cpp <span style="color: \
grey">(540844fece0b15a1be944a4fa1b248f6fd1f1987)</span></li>

 <li>kwin/effects/invert/invert.cpp <span style="color: \
grey">(258e2ecadd01783c082f1dbcaafab24a22e2471e)</span></li>

 <li>kwin/effects/logout/logout.cpp <span style="color: \
grey">(a78460d1bbf0348aa6da884e071d7eb06277192f)</span></li>

 <li>kwin/effects/lookingglass/lookingglass.cpp <span style="color: \
grey">(c532fe1a3d814dfabf43cd57e2e3dba10b3510fe)</span></li>

 <li>kwin/effects/magiclamp/magiclamp.cpp <span style="color: \
grey">(c151ed1175f96d7572ebf40c32e741e4a1ee44a8)</span></li>

 <li>kwin/effects/magnifier/magnifier.cpp <span style="color: \
grey">(0103257fbd104f6b8685f9a4a500ae6447e5b63c)</span></li>

 <li>kwin/effects/mousemark/mousemark.cpp <span style="color: \
grey">(0a3229b3ec0cd256d883bf6d219cff37d772c64f)</span></li>

 <li>kwin/effects/presentwindows/presentwindows.cpp <span style="color: \
grey">(96d9a68a27884a48592183b4735f6a50e5b97e3f)</span></li>

 <li>kwin/effects/resize/resize.cpp <span style="color: \
grey">(410b6748f6b82a0164bc5bed78e4f6785010a400)</span></li>

 <li>kwin/effects/screenshot/screenshot.cpp <span style="color: \
grey">(ea56e80c8b6443a2d09b024396d5f78cf098a0b1)</span></li>

 <li>kwin/effects/sheet/sheet.cpp <span style="color: \
grey">(58af9b4c0d0536de0637a2b741c81450ffb18263)</span></li>

 <li>kwin/effects/showfps/showfps.cpp <span style="color: \
grey">(b3dd9b74943becfb1545f3159bb5aec8d8efb31e)</span></li>

 <li>kwin/effects/showpaint/showpaint.cpp <span style="color: \
grey">(c17178ce54beb4c77c45c2688b2fdc9630c64381)</span></li>

 <li>kwin/effects/snaphelper/snaphelper.cpp <span style="color: \
grey">(7d4be144b4041b61e547b0aaa74c7b80d3c38314)</span></li>

 <li>kwin/effects/startupfeedback/startupfeedback.cpp <span style="color: \
grey">(cab7d43dcfd765bd24006322de3436b88035302e)</span></li>

 <li>kwin/effects/taskbarthumbnail/taskbarthumbnail.cpp <span style="color: \
grey">(0c67a3b84a72b22e9b6d1f6b9e34db8c3445962e)</span></li>

 <li>kwin/effects/trackmouse/trackmouse.cpp <span style="color: \
grey">(cf6575456a03259fb8de10b2792291edcb06ea1a)</span></li>

 <li>kwin/effects/wobblywindows/wobblywindows.cpp <span style="color: \
grey">(dbfbdc35167dfb9301aadd15ae653c031d1868a6)</span></li>

 <li>kwin/effects/zoom/zoom.cpp <span style="color: \
grey">(587de0328d2390b2200e024dbff7f90c24d68308)</span></li>

 <li>kwin/lanczosfilter.cpp <span style="color: \
grey">(654d2cf95ad5da4707bab7fef9a0019702c57d0f)</span></li>

 <li>kwin/libkwineffects/kwineffects.h <span style="color: \
grey">(2c2f7bf5edae3c0f93e255833faab842939f63d1)</span></li>

 <li>kwin/libkwineffects/kwineffects.cpp <span style="color: \
grey">(bae85e79b5b38314e3eb964dd6922bf62528c3e2)</span></li>

 <li>kwin/libkwineffects/kwinglobals.h <span style="color: \
grey">(689fe4da2a1604bee9d2022377ce7f7940105a53)</span></li>

 <li>kwin/libkwineffects/kwinglutils.h <span style="color: \
grey">(14fdd5ba9b5e645eb839cc530e7f6b1d91007589)</span></li>

 <li>kwin/scene_opengl.h <span style="color: \
grey">(8ff0006a650b47f426fb775e7681724f2315e513)</span></li>

 <li>kwin/scene_opengl.cpp <span style="color: \
grey">(3e4290a556e58cf5580278b554d2f199bb2c9a60)</span></li>

 <li>kwin/shadow.cpp <span style="color: \
grey">(de989c7955fa267aa533f40126066e6e0ccb26d9)</span></li>

 <li>kwin/workspace.cpp <span style="color: \
grey">(5b37aee2bc85181ed2b11bd00c21e38fd61a38dd)</span></li>

</ul>

<p><a href="http://git.reviewboard.kde.org/r/106521/diff/" style="margin-left: \
3em;">View Diff</a></p>

  </td>
 </tr>
</table>

  </div>
 </body>
</html>

_______________________________________________
kwin mailing list
kwin@kde.org
https://mail.kde.org/mailman/listinfo/kwin

[prev in list] [next in list] [prev in thread] [next in thread] 