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

[Robert Špalek]

hi Denis,

On Mon, Jul 27, 2009 at 6:18 AM, <dkasak13 at users.sourceforge.net> wrote:

> Revision: 42836
>          http://scummvm.svn.sourceforge.net/scummvm/?rev=42836&view=rev
> Author:   dkasak13
> Date:     2009-07-27 04:18:44 +0000 (Mon, 27 Jul 2009)
>
> Log Message:
> -----------
> * Added AnimationManager::addText() for adding text animations
> * Added AnimationManager::getTopAnimationID(x, y) which returns the ID of
> the top layer animation located on a point
> * Added Animation::getScale{X,Y}()
> * Fixed a few bugs related to animations sometimes having no frames
>
> Modified Paths:
> --------------
>    scummvm/branches/gsoc2009-draci/engines/draci/animation.cpp
>    scummvm/branches/gsoc2009-draci/engines/draci/animation.h
>
> Modified: scummvm/branches/gsoc2009-draci/engines/draci/animation.cpp
> ===================================================================
> --- scummvm/branches/gsoc2009-draci/engines/draci/animation.cpp 2009-07-27
> 04:14:59 UTC (rev 42835)
> +++ scummvm/branches/gsoc2009-draci/engines/draci/animation.cpp 2009-07-27
> 04:18:44 UTC (rev 42836)
> @@ -227,6 +235,11 @@
>
>  Drawable *Animation::getFrame(int frameNum) {
>
> +       // If there are no frames stored, return NULL
> +       if (_frames.size() == 0) {
> +               return NULL;
> +       }
> +


when does this happen, please?  if there is a bug/feature in our game files
that makes this necessary, then OK.  however, logic tells me that animation
with 0 frames should not be produced by the compiler and if it is, then it
definitely shouldn't be playing.

if possible, it would be nice to impose an invariant that getFrame() always
returns non-NULL value so that anybody can dereference it, or even return
const-reference to make it checkable by the compiler


> +Animation *AnimationManager::addText(int id, bool playing) {
> +
> +       Animation *anim = new Animation(_vm, kIgnoreIndex);
> +
> +       anim->setID(id);
> +       anim->setZ(256);
> +       anim->setPlaying(playing);
> +
> +       insertAnimation(anim);
> +
> +       return anim;
> +}


I'm a bit confused that I don't see reading the concrete text anywhere.  it
is stored in the BArchive separately


>
> +int AnimationManager::getTopAnimationID(int x, int y) {
> +
> +       Common::List<Animation *>::iterator it;
> +
> +       // The default return value if no animations were found on these
> coordinates (not even overlays)
> +       // i.e. the black background shows through so treat it as an
> overlay
> +       int retval = kOverlayImage;
> +
> +       // Get transparent colour for the current screen
> +       const int transparent =
> _vm->_screen->getSurface()->getTransparentColour();
> +
> +       for (it = _animations.reverse_begin(); it != _animations.end();
> --it) {
> +
> +               Animation *anim = *it;
> +
> +               // If the animation is not playing, ignore it
> +               if (!anim->isPlaying()) {
> +                       continue;
> +               }
> +
> +               Drawable *frame = anim->getFrame();
> +
> +               if (frame == NULL) {
> +                       continue;
> +               }
>

for clarity, this if would be nice to get rid of if my above comment is
valid


> +
> +               int oldX = frame->getX();
> +               int oldY = frame->getY();
> +
> +               // Take account relative coordinates
> +               int newX = oldX + anim->getRelativeX();
> +               int newY = oldY + anim->getRelativeY();
> +
> +               // Translate the frame to those relative coordinates
> +               frame->setX(newX);
> +               frame->setY(newY);
> +
> +               // Save scaled width and height
> +               int scaledWidth = frame->getScaledWidth();
> +               int scaledHeight = frame->getScaledHeight();
> +
> +               // Take into account per-animation scaling and adjust the
> current frames dimensions
> +               if (anim->getScaleX() != 1.0 || anim->getScaleY() != 1.0)
> +                       frame->setScaled(scaledWidth * anim->getScaleX(),
> scaledHeight * anim->getScaleY());
> +
> +               if (frame->getRect().contains(x, y)) {
> +
> +                       if (frame->getType() == kDrawableText) {
> +
> +                               retval = anim->getID();
> +
> +                       } else if (frame->getType() == kDrawableSprite &&
> +                                          reinterpret_cast<Sprite
> *>(frame)->getPixel(x, y) != transparent) {
> +
> +                               retval = anim->getID();
> +                       }
> +               }
>

why do you need all this?  you have a function returning a pixel from a
sprite, which does all the scaling.  can you not call it and compare the
returned value with the transparent color?


> +
> +               // Revert back to old coordinates
> +               frame->setX(oldX);
> +               frame->setY(oldY);
> +
> +               // Revert back to old dimensions
> +               frame->setScaled(scaledWidth, scaledHeight);
> +
> +               // Found an animation
> +               if (retval != kOverlayImage)
> +                       break;
> +       }
>

even if having the logic duplicated has a good reason, I find this very ugly
and non-maintainable to set some values temporarily and than have to revert
them.  basically, a rule of thumb should be that if a function (or, block of
code) just tests a property, it should be able to receive the input object
as a const reference instead of a writeable pointer.


-- 
Robert Špalek <rspalek at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.scummvm.org/pipermail/scummvm-devel/attachments/20090727/9c4706d1/attachment.html>