[Kde-games-devel] KPat/Freecell broken?

[Andreas Pakulat]

On 14.12.09 20:09:26, Parker Coates wrote:
> On Mon, Dec 14, 2009 at 16:19, Andreas Pakulat wrote:
> > On 14.12.09 21:07:30, Andreas Pakulat wrote:
> >> On 14.12.09 12:50:24, Parker Coates wrote:
> >>> Fortunately there's an even cleaner solution. CardDeck now has a
> >>> cardAnimationsDone() signal, so as soon as I get a chance, I'll modify
> >>> this logic to make use of that instead of manually connecting to the
> >>> animationStopped() signal of individual cards.
> >>
> >> That might actually solve the spider problem. I've tried some debugging
> >> and for some (unfortunately unknown reason) the bookkeeping of
> >> carddeck.cpp m_cardsWaitedFor gets out of sync with reality. Thats why
> >> mousePressEvent then ignores all subsequent mouse events, the
> >> m_cardsWaitedFor is not 0 once the run is moved to the bottom-left
> >> stack.
> >
> > I've tried to dig into this more, but I feel like I'm stuck as I don't
> > really have an idea how the animation of the cards is supposed to be
> > working. It seems this is a bit of a timing issue as with all the debug
> > output I now had one case where the right order of things happened. My
> > last "non-working" debug output is attached along with the diff for
> > that.
> >
> > The output suggests that there's 2 animations started for one of the
> > cards, without a stopAnimation. So the "simplest" fix was to check
> > m_animation inside Card::animate() and call stopAnimation if its != 0:
> >
> > Index: card.cpp
> > ===================================================================
> > --- card.cpp    (revision 1062335)
> > +++ card.cpp    (working copy)
> > @@ -218,6 +218,9 @@
> >
> >  void Card::animate( QPointF pos2, qreal z2, qreal scale2, qreal
> > rotation2, bool faceup2, bool raised, int duration )
> >  {
> > +    if( m_animation ) {
> > +        stopAnimation();
> > +    }
> >     if ( duration <= 0 )
> >     {
> >         setPos( pos2 );
> >
> > I have no idea wether that change is correct, or wether one should try
> > to find why things aren't done in the "proper" order. But I'm not able
> > to do the latter anyway.
> 
> It definitely looks like the right thing to do. At first I was shocked
> that it isn't already there. Then after poking around for a bit, I
> remembered why it wasn't. Calling stopAnimation before starting an
> animation causes a weird looking jump to happen during the
> right-click-to-expose-an-obscured-card animation. In retrospect,
> however, I think that's a small price to pay for giving Card a more
> sane API.

Well, its your code, but I think it might be better to actually find out
why the order of things is messed up there. As reverting the
waitForCards-change at least fixes the games to be playable, maybe this
can wait until you have time again to look into the code yourself.
 
> I'll have to think about whether there's a better way to keep that
> animation smooth. I guess I could always add another bool argument to
> Card::animate(), called forceAnimationToCompleteWhenStoppingIt, but I
> think we can agree that that function has enough arguments already. :)
> 
> Not only was this breaking animation tracking (in some as of yet
> undiscovered corner case), but it also means that Q*Animations were
> leaking. Did you get a chance to test this patch? Please commit if it
> doesn't break anything for you.

See above, I've played a round of freecell and spider with the change, but
am a bit hesitant to put it in as it seems to just hide other bugs in the
animation code. 

If you still think this should go in I'll commit it of course :)

Andreas

-- 
It's all in the mind, ya know.