Re: Review Request 116024: Port IconItem to native QSGTexture
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/#review57990 --- Ship it! Ship It! - Aleix Pol Gonzalez On May 13, 2014, 3:25 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/ --- (Updated May 13, 2014, 3:25 p.m.) Review request for Plasma. Repository: plasma-framework Description --- Port IconItem to native QSGTexture including the animation. This will save constantly uploading a new texture to OpenGL throughout the animation. Diffs - .reviewboardrc 804529c src/declarativeimports/core/CMakeLists.txt 9b3313d src/declarativeimports/core/fadingnode.cpp PRE-CREATION src/declarativeimports/core/fadingnode_p.h PRE-CREATION src/declarativeimports/core/iconitem.h 92a5233 src/declarativeimports/core/iconitem.cpp 9cb487c Diff: https://git.reviewboard.kde.org/r/116024/diff/ Testing --- Test app: http://paste.kde.org/pl5pwdnel Test app shows for lots of icons a decrease from 40 - 27Mb apitrace has a lot fewer calls to glTexImage2D Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 116024: Port IconItem to native QSGTexture
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/#review57991 --- This review has been submitted with commit 096c6d247b6cf688dd3554202f0386ab1558934c by David Edmundson to branch master. - Commit Hook On May 13, 2014, 3:25 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/ --- (Updated May 13, 2014, 3:25 p.m.) Review request for Plasma. Repository: plasma-framework Description --- Port IconItem to native QSGTexture including the animation. This will save constantly uploading a new texture to OpenGL throughout the animation. Diffs - .reviewboardrc 804529c src/declarativeimports/core/CMakeLists.txt 9b3313d src/declarativeimports/core/fadingnode.cpp PRE-CREATION src/declarativeimports/core/fadingnode_p.h PRE-CREATION src/declarativeimports/core/iconitem.h 92a5233 src/declarativeimports/core/iconitem.cpp 9cb487c Diff: https://git.reviewboard.kde.org/r/116024/diff/ Testing --- Test app: http://paste.kde.org/pl5pwdnel Test app shows for lots of icons a decrease from 40 - 27Mb apitrace has a lot fewer calls to glTexImage2D Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 116024: Port IconItem to native QSGTexture
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/ --- (Updated May 15, 2014, 2:19 p.m.) Status -- This change has been marked as submitted. Review request for Plasma. Repository: plasma-framework Description --- Port IconItem to native QSGTexture including the animation. This will save constantly uploading a new texture to OpenGL throughout the animation. Diffs - .reviewboardrc 804529c src/declarativeimports/core/CMakeLists.txt 9b3313d src/declarativeimports/core/fadingnode.cpp PRE-CREATION src/declarativeimports/core/fadingnode_p.h PRE-CREATION src/declarativeimports/core/iconitem.h 92a5233 src/declarativeimports/core/iconitem.cpp 9cb487c Diff: https://git.reviewboard.kde.org/r/116024/diff/ Testing --- Test app: http://paste.kde.org/pl5pwdnel Test app shows for lots of icons a decrease from 40 - 27Mb apitrace has a lot fewer calls to glTexImage2D Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 116024: Port IconItem to native QSGTexture
On May 13, 2014, 2:47 p.m., Martin Gräßlin wrote: src/declarativeimports/core/CMakeLists.txt, line 39 https://git.reviewboard.kde.org/r/116024/diff/4/?file=272717#file272717line39 why do you link against Qt5::OpenGL and find it? OpenGL is part of Gui David Edmundson wrote: For anyone else who gets this problem: QGlContext needs porting to QOpenGLContext which is in Gui. Then you don't need to link OpenGL directly the check, and link for Qt5OpenGL is indeed needed, otherwise: /home/abuild/rpmbuild/BUILD/plasma-framework-4.99.0git~20140515~09453cf/src/declarativeimports/core/fadingnode.cpp:23:31: fatal error: QtOpenGL/QGLContext: No such file or directory - Hrvoje --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/#review57872 --- On May 15, 2014, 4:19 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/ --- (Updated May 15, 2014, 4:19 p.m.) Review request for Plasma. Repository: plasma-framework Description --- Port IconItem to native QSGTexture including the animation. This will save constantly uploading a new texture to OpenGL throughout the animation. Diffs - .reviewboardrc 804529c src/declarativeimports/core/CMakeLists.txt 9b3313d src/declarativeimports/core/fadingnode.cpp PRE-CREATION src/declarativeimports/core/fadingnode_p.h PRE-CREATION src/declarativeimports/core/iconitem.h 92a5233 src/declarativeimports/core/iconitem.cpp 9cb487c Diff: https://git.reviewboard.kde.org/r/116024/diff/ Testing --- Test app: http://paste.kde.org/pl5pwdnel Test app shows for lots of icons a decrease from 40 - 27Mb apitrace has a lot fewer calls to glTexImage2D Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 116024: Port IconItem to native QSGTexture
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/#review57914 --- just random thought: should we introuce a shaders source directory and not have the shader source in the code? - Martin Gräßlin On May 13, 2014, 5:25 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/ --- (Updated May 13, 2014, 5:25 p.m.) Review request for Plasma. Repository: plasma-framework Description --- Port IconItem to native QSGTexture including the animation. This will save constantly uploading a new texture to OpenGL throughout the animation. Diffs - .reviewboardrc 804529c src/declarativeimports/core/CMakeLists.txt 9b3313d src/declarativeimports/core/fadingnode.cpp PRE-CREATION src/declarativeimports/core/fadingnode_p.h PRE-CREATION src/declarativeimports/core/iconitem.h 92a5233 src/declarativeimports/core/iconitem.cpp 9cb487c Diff: https://git.reviewboard.kde.org/r/116024/diff/ Testing --- Test app: http://paste.kde.org/pl5pwdnel Test app shows for lots of icons a decrease from 40 - 27Mb apitrace has a lot fewer calls to glTexImage2D Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 116024: Port IconItem to native QSGTexture
On May 14, 2014, 11:48 a.m., Martin Gräßlin wrote: just random thought: should we introuce a shaders source directory and not have the shader source in the code? Qt does that. It certainly makes sense if you are going to have lots and lots. We have 2. Certainly not a bad thought, but I'd say we should move later if we need it. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/#review57914 --- On May 13, 2014, 3:25 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/ --- (Updated May 13, 2014, 3:25 p.m.) Review request for Plasma. Repository: plasma-framework Description --- Port IconItem to native QSGTexture including the animation. This will save constantly uploading a new texture to OpenGL throughout the animation. Diffs - .reviewboardrc 804529c src/declarativeimports/core/CMakeLists.txt 9b3313d src/declarativeimports/core/fadingnode.cpp PRE-CREATION src/declarativeimports/core/fadingnode_p.h PRE-CREATION src/declarativeimports/core/iconitem.h 92a5233 src/declarativeimports/core/iconitem.cpp 9cb487c Diff: https://git.reviewboard.kde.org/r/116024/diff/ Testing --- Test app: http://paste.kde.org/pl5pwdnel Test app shows for lots of icons a decrease from 40 - 27Mb apitrace has a lot fewer calls to glTexImage2D Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 116024: Port IconItem to native QSGTexture
On May 13, 2014, 4:59 p.m., Martin Gräßlin wrote: src/declarativeimports/core/fadingnode.cpp, line 104 https://git.reviewboard.kde.org/r/116024/diff/4-5/?file=272718#file272718line104 double ;; Fixed. I'm not going to re-upload a diff to prove it though. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/#review57887 --- On May 13, 2014, 3:25 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/ --- (Updated May 13, 2014, 3:25 p.m.) Review request for Plasma. Repository: plasma-framework Description --- Port IconItem to native QSGTexture including the animation. This will save constantly uploading a new texture to OpenGL throughout the animation. Diffs - .reviewboardrc 804529c src/declarativeimports/core/CMakeLists.txt 9b3313d src/declarativeimports/core/fadingnode.cpp PRE-CREATION src/declarativeimports/core/fadingnode_p.h PRE-CREATION src/declarativeimports/core/iconitem.h 92a5233 src/declarativeimports/core/iconitem.cpp 9cb487c Diff: https://git.reviewboard.kde.org/r/116024/diff/ Testing --- Test app: http://paste.kde.org/pl5pwdnel Test app shows for lots of icons a decrease from 40 - 27Mb apitrace has a lot fewer calls to glTexImage2D Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 116024: Port IconItem to native QSGTexture
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/ --- (Updated May 13, 2014, 10:50 a.m.) Review request for Plasma. Changes --- Updated Repository: plasma-framework Description --- Port IconItem to native QSGTexture including the animation. This will save constantly uploading a new texture to OpenGL throughout the animation. Diffs (updated) - .reviewboardrc 804529c CMakeLists.txt a726c75 src/declarativeimports/core/CMakeLists.txt 9b3313d src/declarativeimports/core/fadingnode.cpp PRE-CREATION src/declarativeimports/core/fadingnode_p.h PRE-CREATION src/declarativeimports/core/iconitem.h 92a5233 src/declarativeimports/core/iconitem.cpp 9cb487c Diff: https://git.reviewboard.kde.org/r/116024/diff/ Testing --- Test app: http://paste.kde.org/pl5pwdnel Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 116024: Port IconItem to native QSGTexture
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/ --- (Updated May 13, 2014, 11:09 a.m.) Review request for Plasma. Repository: plasma-framework Description --- Port IconItem to native QSGTexture including the animation. This will save constantly uploading a new texture to OpenGL throughout the animation. Diffs - .reviewboardrc 804529c CMakeLists.txt a726c75 src/declarativeimports/core/CMakeLists.txt 9b3313d src/declarativeimports/core/fadingnode.cpp PRE-CREATION src/declarativeimports/core/fadingnode_p.h PRE-CREATION src/declarativeimports/core/iconitem.h 92a5233 src/declarativeimports/core/iconitem.cpp 9cb487c Diff: https://git.reviewboard.kde.org/r/116024/diff/ Testing (updated) --- Test app: http://paste.kde.org/pl5pwdnel Test app shows for lots of icons a decrease from 40 - 27Mb apitrace has a lot fewer calls to glTexImage2D Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 116024: Port IconItem to native QSGTexture
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/#review57872 --- I really like the approach! src/declarativeimports/core/CMakeLists.txt https://git.reviewboard.kde.org/r/116024/#comment40270 why do you link against Qt5::OpenGL and find it? OpenGL is part of Gui src/declarativeimports/core/fadingnode.cpp https://git.reviewboard.kde.org/r/116024/#comment40275 I normally prefix the varying with v_ e.g. varying highp vec2 v_coord src/declarativeimports/core/fadingnode.cpp https://git.reviewboard.kde.org/r/116024/#comment40276 similarily I prefix uniforms with u_ e.g. uniform sampler2D u_src src/declarativeimports/core/fadingnode.cpp https://git.reviewboard.kde.org/r/116024/#comment40271 I'd always activate the GL_TEXTURE0 texture target after setting the texture. You never know what will be called afterwards and it might expect it's GL_TEXTURE0. src/declarativeimports/core/fadingnode.cpp https://git.reviewboard.kde.org/r/116024/#comment40273 any particular reason to explicitly set the true? src/declarativeimports/core/fadingnode.cpp https://git.reviewboard.kde.org/r/116024/#comment40272 QScopedPointer? src/declarativeimports/core/fadingnode.cpp https://git.reviewboard.kde.org/r/116024/#comment40274 empty newlines src/declarativeimports/core/iconitem.h https://git.reviewboard.kde.org/r/116024/#comment40281 why additional new line? src/declarativeimports/core/iconitem.h https://git.reviewboard.kde.org/r/116024/#comment40280 why is it moved around? src/declarativeimports/core/iconitem.cpp https://git.reviewboard.kde.org/r/116024/#comment40282 pedantic: move all the * one to right src/declarativeimports/core/iconitem.cpp https://git.reviewboard.kde.org/r/116024/#comment40277 coding style if( - if ( src/declarativeimports/core/iconitem.cpp https://git.reviewboard.kde.org/r/116024/#comment40279 ? src/declarativeimports/core/iconitem.cpp https://git.reviewboard.kde.org/r/116024/#comment40278 ? - Martin Gräßlin On May 13, 2014, 1:09 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/ --- (Updated May 13, 2014, 1:09 p.m.) Review request for Plasma. Repository: plasma-framework Description --- Port IconItem to native QSGTexture including the animation. This will save constantly uploading a new texture to OpenGL throughout the animation. Diffs - .reviewboardrc 804529c CMakeLists.txt a726c75 src/declarativeimports/core/CMakeLists.txt 9b3313d src/declarativeimports/core/fadingnode.cpp PRE-CREATION src/declarativeimports/core/fadingnode_p.h PRE-CREATION src/declarativeimports/core/iconitem.h 92a5233 src/declarativeimports/core/iconitem.cpp 9cb487c Diff: https://git.reviewboard.kde.org/r/116024/diff/ Testing --- Test app: http://paste.kde.org/pl5pwdnel Test app shows for lots of icons a decrease from 40 - 27Mb apitrace has a lot fewer calls to glTexImage2D Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 116024: Port IconItem to native QSGTexture
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/ --- (Updated May 13, 2014, 3:25 p.m.) Review request for Plasma. Repository: plasma-framework Description --- Port IconItem to native QSGTexture including the animation. This will save constantly uploading a new texture to OpenGL throughout the animation. Diffs (updated) - .reviewboardrc 804529c src/declarativeimports/core/CMakeLists.txt 9b3313d src/declarativeimports/core/fadingnode.cpp PRE-CREATION src/declarativeimports/core/fadingnode_p.h PRE-CREATION src/declarativeimports/core/iconitem.h 92a5233 src/declarativeimports/core/iconitem.cpp 9cb487c Diff: https://git.reviewboard.kde.org/r/116024/diff/ Testing --- Test app: http://paste.kde.org/pl5pwdnel Test app shows for lots of icons a decrease from 40 - 27Mb apitrace has a lot fewer calls to glTexImage2D Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 116024: Port IconItem to native QSGTexture
On May 13, 2014, 12:47 p.m., Martin Gräßlin wrote: src/declarativeimports/core/CMakeLists.txt, line 39 https://git.reviewboard.kde.org/r/116024/diff/4/?file=272717#file272717line39 why do you link against Qt5::OpenGL and find it? OpenGL is part of Gui For anyone else who gets this problem: QGlContext needs porting to QOpenGLContext which is in Gui. Then you don't need to link OpenGL directly On May 13, 2014, 12:47 p.m., Martin Gräßlin wrote: src/declarativeimports/core/fadingnode.cpp, lines 84-92 https://git.reviewboard.kde.org/r/116024/diff/4/?file=272718#file272718line84 I'd always activate the GL_TEXTURE0 texture target after setting the texture. You never know what will be called afterwards and it might expect it's GL_TEXTURE0. I'm confused. I need to set the active texture /before/ I bind to it. Otherwise everything breaks. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/#review57872 --- On May 13, 2014, 3:25 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/ --- (Updated May 13, 2014, 3:25 p.m.) Review request for Plasma. Repository: plasma-framework Description --- Port IconItem to native QSGTexture including the animation. This will save constantly uploading a new texture to OpenGL throughout the animation. Diffs - .reviewboardrc 804529c src/declarativeimports/core/CMakeLists.txt 9b3313d src/declarativeimports/core/fadingnode.cpp PRE-CREATION src/declarativeimports/core/fadingnode_p.h PRE-CREATION src/declarativeimports/core/iconitem.h 92a5233 src/declarativeimports/core/iconitem.cpp 9cb487c Diff: https://git.reviewboard.kde.org/r/116024/diff/ Testing --- Test app: http://paste.kde.org/pl5pwdnel Test app shows for lots of icons a decrease from 40 - 27Mb apitrace has a lot fewer calls to glTexImage2D Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 116024: Port IconItem to native QSGTexture
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/#review57887 --- looks good. I'll give it a run tomorrow in a real setup. src/declarativeimports/core/fadingnode.cpp https://git.reviewboard.kde.org/r/116024/#comment40286 double ;; - Martin Gräßlin On May 13, 2014, 5:25 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/ --- (Updated May 13, 2014, 5:25 p.m.) Review request for Plasma. Repository: plasma-framework Description --- Port IconItem to native QSGTexture including the animation. This will save constantly uploading a new texture to OpenGL throughout the animation. Diffs - .reviewboardrc 804529c src/declarativeimports/core/CMakeLists.txt 9b3313d src/declarativeimports/core/fadingnode.cpp PRE-CREATION src/declarativeimports/core/fadingnode_p.h PRE-CREATION src/declarativeimports/core/iconitem.h 92a5233 src/declarativeimports/core/iconitem.cpp 9cb487c Diff: https://git.reviewboard.kde.org/r/116024/diff/ Testing --- Test app: http://paste.kde.org/pl5pwdnel Test app shows for lots of icons a decrease from 40 - 27Mb apitrace has a lot fewer calls to glTexImage2D Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 116024: Port IconItem to native QSGTexture
On March 21, 2014, 9:24 a.m., Marco Martin wrote: what's the status of this? It wasn't really a priority before the beta, so I moved away for a bit. Code as far as I'm concerned is fine, but I'd like to modify this so that we switch back away from a fading node when we're not animating. Otherwise we're keeping two QSGTexture objects when we only need one. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/#review53630 --- On Feb. 27, 2014, 1:44 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/ --- (Updated Feb. 27, 2014, 1:44 p.m.) Review request for Plasma. Repository: plasma-framework Description --- Port IconItem to native QSGTexture including the animation. This will save constantly uploading a new texture to OpenGL throughout the animation. Diffs - CMakeLists.txt 4bc47bf src/declarativeimports/core/CMakeLists.txt 97e9283 src/declarativeimports/core/fadingnode.cpp PRE-CREATION src/declarativeimports/core/fadingnode_p.h PRE-CREATION src/declarativeimports/core/iconitem.h 26ee410 src/declarativeimports/core/iconitem.cpp fed2f9b Diff: https://git.reviewboard.kde.org/r/116024/diff/ Testing --- Test app: http://paste.kde.org/pl5pwdnel Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 116024: Port IconItem to native QSGTexture
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/#review53630 --- what's the status of this? - Marco Martin On Feb. 27, 2014, 1:44 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/ --- (Updated Feb. 27, 2014, 1:44 p.m.) Review request for Plasma. Repository: plasma-framework Description --- Port IconItem to native QSGTexture including the animation. This will save constantly uploading a new texture to OpenGL throughout the animation. Diffs - CMakeLists.txt 4bc47bf src/declarativeimports/core/CMakeLists.txt 97e9283 src/declarativeimports/core/fadingnode.cpp PRE-CREATION src/declarativeimports/core/fadingnode_p.h PRE-CREATION src/declarativeimports/core/iconitem.h 26ee410 src/declarativeimports/core/iconitem.cpp fed2f9b Diff: https://git.reviewboard.kde.org/r/116024/diff/ Testing --- Test app: http://paste.kde.org/pl5pwdnel Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 116024: Port IconItem to native QSGTexture
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/ --- (Updated Feb. 27, 2014, 12:19 p.m.) Review request for Plasma. Changes --- Perform the animation as a fragment shader. I've done it as a separate class because I know Eike wants to fade between prefixes in FrameSVG. Repository: plasma-framework Description --- Port IconItem to native QSGTexture including the animation. This will save constantly uploading a new texture to OpenGL throughout the animation. Diffs (updated) - CMakeLists.txt 4bc47bf src/declarativeimports/core/CMakeLists.txt 97e9283 src/declarativeimports/core/fadingnode.cpp PRE-CREATION src/declarativeimports/core/fadingnode_p.h PRE-CREATION src/declarativeimports/core/iconitem.h 26ee410 src/declarativeimports/core/iconitem.cpp fed2f9b Diff: https://git.reviewboard.kde.org/r/116024/diff/ Testing --- Test app: http://paste.kde.org/pl5pwdnel Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 116024: Port IconItem to native QSGTexture
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/ --- (Updated Feb. 27, 2014, 1:44 p.m.) Review request for Plasma. Repository: plasma-framework Description --- Port IconItem to native QSGTexture including the animation. This will save constantly uploading a new texture to OpenGL throughout the animation. Diffs (updated) - CMakeLists.txt 4bc47bf src/declarativeimports/core/CMakeLists.txt 97e9283 src/declarativeimports/core/fadingnode.cpp PRE-CREATION src/declarativeimports/core/fadingnode_p.h PRE-CREATION src/declarativeimports/core/iconitem.h 26ee410 src/declarativeimports/core/iconitem.cpp fed2f9b Diff: https://git.reviewboard.kde.org/r/116024/diff/ Testing --- Test app: http://paste.kde.org/pl5pwdnel Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 116024: Port IconItem to native QSGTexture
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/#review51129 --- src/declarativeimports/core/fadingnode.cpp https://git.reviewboard.kde.org/r/116024/#comment35847 Despite my code being clearly right this doesn't work. (according to apitrace and then qDebug) oldState is always 0 This means we are binding textures every frame, which is what we wanted to avoid. I'm quite confused as to why. - David Edmundson On Feb. 27, 2014, 1:44 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/ --- (Updated Feb. 27, 2014, 1:44 p.m.) Review request for Plasma. Repository: plasma-framework Description --- Port IconItem to native QSGTexture including the animation. This will save constantly uploading a new texture to OpenGL throughout the animation. Diffs - CMakeLists.txt 4bc47bf src/declarativeimports/core/CMakeLists.txt 97e9283 src/declarativeimports/core/fadingnode.cpp PRE-CREATION src/declarativeimports/core/fadingnode_p.h PRE-CREATION src/declarativeimports/core/iconitem.h 26ee410 src/declarativeimports/core/iconitem.cpp fed2f9b Diff: https://git.reviewboard.kde.org/r/116024/diff/ Testing --- Test app: http://paste.kde.org/pl5pwdnel Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 116024: Port IconItem to native QSGTexture
On Feb. 27, 2014, 8:17 p.m., David Edmundson wrote: src/declarativeimports/core/fadingnode.cpp, line 85 https://git.reviewboard.kde.org/r/116024/diff/3/?file=246683#file246683line85 Despite my code being clearly right this doesn't work. (according to apitrace and then qDebug) oldState is always 0 This means we are binding textures every frame, which is what we wanted to avoid. I'm quite confused as to why. I understand it now (I think) If I have 2 icons, I will have 2 materials, but still only 1 shader. oldState is used so that when SG batches operations I don't have to update all my uniforms if properties are the same between different materials being rendered in different nodes. It is not the previous state of the current material being updated. Sorry for the noise. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/#review51129 --- On Feb. 27, 2014, 1:44 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/ --- (Updated Feb. 27, 2014, 1:44 p.m.) Review request for Plasma. Repository: plasma-framework Description --- Port IconItem to native QSGTexture including the animation. This will save constantly uploading a new texture to OpenGL throughout the animation. Diffs - CMakeLists.txt 4bc47bf src/declarativeimports/core/CMakeLists.txt 97e9283 src/declarativeimports/core/fadingnode.cpp PRE-CREATION src/declarativeimports/core/fadingnode_p.h PRE-CREATION src/declarativeimports/core/iconitem.h 26ee410 src/declarativeimports/core/iconitem.cpp fed2f9b Diff: https://git.reviewboard.kde.org/r/116024/diff/ Testing --- Test app: http://paste.kde.org/pl5pwdnel Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 116024: Port IconItem to native QSGTexture
On Feb. 24, 2014, 4:55 p.m., Marco Martin wrote: not sure how much will be visible in the end result. tough if i understood correctly how it works, it does the animation by animating the opacity of the old image from 1 to 0, and the opacity of the new one from 0 to 1. This is not the same thing as a crossfade, that's what it was doing. I fear it will be visible the opacity of the whole result go down and then up again, especially when animating with an highlight effect (that is the same icon) David Edmundson wrote: I think it should be possible to keep that behaviour, with something like this patch. I would need to create a QSGMaterialShader then copy and paste the relevant fragmentShader from qtgraphicseffects. I can then put the shader directly on my texture, rather than needing the extra node. Marco can you confirm this fading looks right. http://paste.kde.org/p17wtna35 . It should run as a standalone in qmlscene. For comparison the old IconItem is on the right. The animations look slightly different because my hacked example of the fragment shader doesn't have the delay waiting for the pixmap to load. It uses a fragmentShader to do the transition. If it's OK, I'll merge this shader into the C++ code I have here as a custom material on the root node. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/#review50720 --- On Feb. 24, 2014, 4:09 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/ --- (Updated Feb. 24, 2014, 4:09 p.m.) Review request for Plasma. Repository: plasma-framework Description --- Port IconItem to native QSGTexture including the animation. This will save constantly uploading a new texture to OpenGL throughout the animation. Diffs - src/declarativeimports/core/iconitem.h 26ee410 src/declarativeimports/core/iconitem.cpp fed2f9b Diff: https://git.reviewboard.kde.org/r/116024/diff/ Testing --- Test app: http://paste.kde.org/pl5pwdnel Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Review Request 116024: Port IconItem to native QSGTexture
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/ --- Review request for Plasma. Summary (updated) - Port IconItem to native QSGTexture Repository: plasma-framework Description (updated) --- Port IconItem to native QSGTexture including the animation. This will save constantly uploading a new texture to OpenGL throughout the animation. Diffs (updated) - src/declarativeimports/core/iconitem.h 26ee410 src/declarativeimports/core/iconitem.cpp fed2f9b Diff: https://git.reviewboard.kde.org/r/116024/diff/ Testing (updated) --- Test app: http://paste.kde.org/pl5pwdnel Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 116024: Port IconItem to native QSGTexture
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/#review50716 --- I haven't runtime-tested it yet, but this is something I was hoping we'd get around to doing, and from what I can see the chosen approach seems sound. Yay! - Eike Hein On Feb. 24, 2014, 4:09 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/ --- (Updated Feb. 24, 2014, 4:09 p.m.) Review request for Plasma. Repository: plasma-framework Description --- Port IconItem to native QSGTexture including the animation. This will save constantly uploading a new texture to OpenGL throughout the animation. Diffs - src/declarativeimports/core/iconitem.h 26ee410 src/declarativeimports/core/iconitem.cpp fed2f9b Diff: https://git.reviewboard.kde.org/r/116024/diff/ Testing --- Test app: http://paste.kde.org/pl5pwdnel Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 116024: Port IconItem to native QSGTexture
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/#review50720 --- not sure how much will be visible in the end result. tough if i understood correctly how it works, it does the animation by animating the opacity of the old image from 1 to 0, and the opacity of the new one from 0 to 1. This is not the same thing as a crossfade, that's what it was doing. I fear it will be visible the opacity of the whole result go down and then up again, especially when animating with an highlight effect (that is the same icon) - Marco Martin On Feb. 24, 2014, 4:09 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/ --- (Updated Feb. 24, 2014, 4:09 p.m.) Review request for Plasma. Repository: plasma-framework Description --- Port IconItem to native QSGTexture including the animation. This will save constantly uploading a new texture to OpenGL throughout the animation. Diffs - src/declarativeimports/core/iconitem.h 26ee410 src/declarativeimports/core/iconitem.cpp fed2f9b Diff: https://git.reviewboard.kde.org/r/116024/diff/ Testing --- Test app: http://paste.kde.org/pl5pwdnel Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 116024: Port IconItem to native QSGTexture
On Feb. 24, 2014, 4:55 p.m., Marco Martin wrote: not sure how much will be visible in the end result. tough if i understood correctly how it works, it does the animation by animating the opacity of the old image from 1 to 0, and the opacity of the new one from 0 to 1. This is not the same thing as a crossfade, that's what it was doing. I fear it will be visible the opacity of the whole result go down and then up again, especially when animating with an highlight effect (that is the same icon) I think it should be possible to keep that behaviour, with something like this patch. I would need to create a QSGMaterialShader then copy and paste the relevant fragmentShader from qtgraphicseffects. I can then put the shader directly on my texture, rather than needing the extra node. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/#review50720 --- On Feb. 24, 2014, 4:09 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116024/ --- (Updated Feb. 24, 2014, 4:09 p.m.) Review request for Plasma. Repository: plasma-framework Description --- Port IconItem to native QSGTexture including the animation. This will save constantly uploading a new texture to OpenGL throughout the animation. Diffs - src/declarativeimports/core/iconitem.h 26ee410 src/declarativeimports/core/iconitem.cpp fed2f9b Diff: https://git.reviewboard.kde.org/r/116024/diff/ Testing --- Test app: http://paste.kde.org/pl5pwdnel Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
