[Scummvm-cvs-logs] SF.net SVN: scummvm:[42114] scummvm/branches/gsoc2009-draci/engines/draci

[Robert Špalek]

hi Denis,

On Sat, Jul 4, 2009 at 8:24 PM, <dkasak13 at users.sourceforge.net> wrote:

> Revision: 42114
>          http://scummvm.svn.sourceforge.net/scummvm/?rev=42114&view=rev
> Author:   dkasak13
> Date:     2009-07-05 03:24:46 +0000 (Sun, 05 Jul 2009)
>
> Log Message:
> -----------
> * API change for Animation and AnimObj; AnimObj is now a proper class and
> each instance handles its own animation. Animation handles adding, fetching
> and deleting of AnimObjs (probably needs a namechange).
> * Implemented actual animation (previously only the first frame was
> display)
> * Implemented animation starting, stoping, looping
> * Loaded looping dragon animation as a test
>


>
> Modified: scummvm/branches/gsoc2009-draci/engines/draci/animation.cpp
> ===================================================================
> --- scummvm/branches/gsoc2009-draci/engines/draci/animation.cpp 2009-07-05
> 01:16:53 UTC (rev 42113)
> +++ scummvm/branches/gsoc2009-draci/engines/draci/animation.cpp 2009-07-05
> 03:24:46 UTC (rev 42114)
> +       // If we are at the last frame and not looping, stop the animation
> +       // The animation is also restarted to frame zero
> +       if ((_currentFrame == nextFrameNum() - 1) && !_looping) {
> +               _currentFrame = 0;
> +               _playing = false;
> +               return;
> +       }


to be verified: I'm not sure if the original player deletes the object or if
it sticks to the last image forever.  probably you made it right.

+       if (force || (_tick + _delay <= _vm->_system->getMillis())) {
> +               _vm->_screen->getSurface()->markDirtyRect(frameRect);
> +               _currentFrame = nextFrameNum();
> +               _tick = _vm->_system->getMillis();
> +       }


it probably doesn't matter, but it would be better to assign _tick +=
_delay;  this way you get exact timing even if the player is slow and more
time has elapsed.

+uint AnimObj::nextFrameNum() {
> +
> +       if ((_currentFrame == getFramesNum() - 1) && _looping)
> +               return 0;


I'd remote "&& _looping".  of course, the case !_looping is handled above,
but in case an error happens and you get here, it's nicer to keep the
invariant that _currentFrame is between 0 and getFramesNum() instead of
having a buffer overflow.

+       if (_id == kOverlayImage) {
> +               _frames[_currentFrame]->draw(surface, false);
> +       }
> +       else {
> +               _frames[_currentFrame]->draw(surface, true);
> +       }


it would be more readable to say:

const bool is_overlay = _id == kOverlayImage;
_frames[_currentFrame]->draw(surface, is_overlay);

+void Animation::play(int id) {

+
> +       AnimObj *obj = getAnimation(id);
> +
> +       obj->setPlaying(true);
> +}


in all functions similar to this, wouldn't it be shorter and still quite
comprehensible to say getAnimation(id)->setPlaying(true); ?


> +AnimObj *Animation::getAnimation(int id) {
> +
> +       Common::List<AnimObj *>::iterator it;
> +
>        for (it = _animObjects.begin(); it != _animObjects.end(); ++it) {
> -               if (it->_id == id) {
> -                       return it;
> +               if ((*it)->getID() == id) {
> +                       return *it;
>                }
>        }
>
> -       return _animObjects.end();
> +       return *_animObjects.end();


it may be better to return simply NULL instead of  _animObjects.end();  this
is an internal variable that shouldn't be exposed.  actually I think you
shouldn't dereference such a pointer anyway, because it may not be defined.
 in any case testing against NULL in the caller is easier.


>  }
>
> -void Animation::insertAnimation(AnimObj &animObj) {
> +void Animation::insertAnimation(AnimObj *animObj) {
>
> -       Common::List<AnimObj>::iterator it;
> +       Common::List<AnimObj *>::iterator it;
>
>        for (it = _animObjects.begin(); it != _animObjects.end(); ++it) {
> -               if (animObj._z < it->_z)
> +               if (animObj->getZ() < (*it)->getZ())


just to make sure, without knowing the libraries.  there is no STL-like
implementation of sorted list in ScummVM?  it would be much more readable to
use it rather than using a general list and maintaining sortedness yourself.


>
> Modified: scummvm/branches/gsoc2009-draci/engines/draci/animation.h
> ===================================================================
> --- scummvm/branches/gsoc2009-draci/engines/draci/animation.h   2009-07-05
> 01:16:53 UTC (rev 42113)
> +++ scummvm/branches/gsoc2009-draci/engines/draci/animation.h   2009-07-05
> 03:24:46 UTC (rev 42114)
> @@ -30,44 +30,81 @@
>
> -       Common::List<AnimObj> _animObjects;
> +       Common::List<AnimObj *> _animObjects;


why did you do this change, please?  AFAIK each AnimObj instance is held in
the memory exactly once, and that is in this list.  it therefore would be
most logical to keep it the old way.  the new way just makes you syntax very
cumbersome (you have to use (*it)->method() instead of it->method()) and
what really happens in the memory is that Common::List allocates a memory
for a pointer, which points again to the real instance (i.e., instead of
having direct pointer you have a link list of length 2).

if you ever need to refer to an AnimObj from elsewhere than from this list,
you can pass a pointer, of course, but it should be clearer to have this
list a list of instances instead of pointers.  you will also get memory
management for free.

maybe you have a reason to do this, but I didn't get it from this commit.

a nice commit otherwise!
-- 
Robert Špalek <rspalek at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.scummvm.org/pipermail/scummvm-git-logs/attachments/20090706/0ef39052/attachment.html>