Re: [kde-artists] oxygen style and kwin client, svn external

On January 4, 2010, Hugo Pereira Da Costa wrote:
> Now last time I did the switch to QPropertyAnimation, it resulted in
> significantly slower animations (at least on my intel graphics card.
> Reason it seems is that QAnimation tries to push more updates per second
> than QTimeLine. (which kdepepo confirmed in the code). Which is why I
> left it as a QTimeLine.

frame limiting may make a nice addition to kinetic, but i don't see how the 
animations would be slower in this case.

if it takes 100ms to generate a frame in the animation, then the maximum rate 
will be 10 frames/s with kinetic. it doesn't enforce some frame rate, 
regardless of how long it takes to achieve that. it is time based, just like 
with QTimeLine. :)

what can happen is that it uses more cpu to achieve an animation since it may 
generates more frames than you strictly want (if the animation frame 
generation is quick), but the timespan itself should be no different. if 
anything, the animation should be smoother but more cpu intensive.

so, what do you mean by "slower", exactly? because perhaps you have some 
specific impact in mind other than what that means to me on first read :)

> > there also appears to be something of  a misunderstanding is the usage of
> > QPointer in the code. data() is never checked (so it's not actually
> > protecting the code against anything!) and QPointer itself results in a
> > global hash being populated and checked *whenever a QObject is deleted*.
> > this is made even worse since the hash is global to the app it requires
> > aquiring an app-global lock every time it is accessed. oy vey! please
> > use QWeakPointer instead of QPointer, which avoids this problem. and in
> > the case of code like this where the value of data() is never checked,
> > don't bother using a shared pointer at all, it just gives the code a
> > false sense of security at the expense of some overhead.
> 
> Same thing here. I planed to switch to QWeakPointer instead of QPointer,
> which is why I used data() all over the place.

using data() only matters if you actually check the object first. e.g. this:

QWeakPointer<QWidget> *wp = foo;
.. later ...
wp.data()->doSomething();

is jut as dangerous as:

QWidget * p = foo;
... later ...
p->doSomething();

what you need to do is:

QWeakPointer<QWidget> *wp = foo;
.. later ...
if (wp) { wp.data()->doSomething(); }

that wasn't happening anywhere in the code that i looked at and so the usage 
of shared pointers was actually not giving any benefit, just overhead. 
fortunately the "shared" pointers weren't really shared at all in any of the 
window decoration code, so it was trivial to just move to plain c++ pointers 
there. i haven't looked much into the style code, but if the pointers really 
are shared around there, please be certain to check that the pointer is valid 
before using data()

> > thoughts?
> 
> QPointer, I'm ready to change. No big deal, and that was the original idea.

great :)

> OxygenAnimation, its ok too (though I would leave it in lib/ cause I
> added some utility functions (a la "isRunning", "restart" and stuff like
> that, and might benefit a tiny abstraction layer for future changes).

we've been around this same path in Plasma, it's really not worth it. 
isRunning and restart are trivial and don't really save much to warrant the 
addition of a whole other class and QAbstractAnimation is about as abstract as 
one is going to get for an animation base (even to the point of being too 
generic/simple, as it doesn't provide any actual animations framework, just a 
bunch of convenience classes by which to change values over time; this is 
useful and needed, but also very basic :)

> However, this might trigger complains about style animations performing
> poorly. (to be confirmed. So far I have been the only one to test this.
> By the way: any smart idea on how to actually allow people to test both ?)

if OxygenAnimation replicated large parts of the API and had a 
QAbstractAnimation or a QTimeline as an internal member. this makes other 
things really difficult though, such as combining animations. i don't think 
it's worth it.

if we're having issues with Kinetic, let's discuss it with the people working 
on Kinetic (some of them are very much involved with KDE development as well) 
and get improvements there.

also note that QTimeLine uses a separate timer per object while Kinetic uses a 
shared timer. in the case of many animations running simultaneously, having a 
shared timer is usually better for coordination, wakeups, etc.

> I'm confident I can commit both before the branching tomorrow (since
> this was the original plan).

great; i've already got the oxygen style done, so i'll commit that then.

-- 
Aaron J. Seigo
humru othro a kohnu se
GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA  EE75 D6B7 2EB1 A7F1 DB43

KDE core developer sponsored by Qt Development Frameworks
______________________________________________________________________________
[email protected] |  https://mail.kde.org/mailman/listinfo/kde-artists