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

[Robert Špalek]

hi Denis,

sorry for a long delay.  my vacation is near the end and I have found time
to send you some comments.

the biggest comment is your "object model".  without studying the context of
your new code into too much detail, it seems to me that each caller is
testing whether a sprite is scaled and then calls either draw() or
drawScaled(), resp. getRect() and getScaledRect().  this would be
cumbersome.  being scaled should be an internal property of a sprite and
when the caller calls draw(), optionally cropped to some screen rectangle,
the sprite should draw itself according to its settings.

I would either implement it by moving the if-command for scaling/not scaling
inside the object, or even leaving the object as is and implementing a new
inherited object which redefines drawing.  since all objects except for the
dragon are unscaled and only the dragon is scaled, using the plain old
objects for almost everything might be best, and only the dragon would be
handled in a more complicated way.  either way you choose, it will be better
than handling the if in all callers.

On Mon, Jul 20, 2009 at 7:25 PM, <dkasak13 at users.sourceforge.net> wrote:

> Revision: 42627
>          http://scummvm.svn.sourceforge.net/scummvm/?rev=42627&view=rev
> Author:   dkasak13
> Date:     2009-07-20 17:25:57 +0000 (Mon, 20 Jul 2009)
>
> Log Message:
> -----------
> * Added scaling support
> * Made the dragon scale when it is in different parts of the room
> * Added getters for relative coordinates (Animation::getRelativeX() and
> Animation::getRelativeY())
> * Commented Game::loop() and Sprite::draw*() methods in more detail
>
> Modified: scummvm/branches/gsoc2009-draci/engines/draci/game.cpp
> ===================================================================
> --- scummvm/branches/gsoc2009-draci/engines/draci/game.cpp      2009-07-20
> 09:19:27 UTC (rev 42626)
> +++ scummvm/branches/gsoc2009-draci/engines/draci/game.cpp      2009-07-20
> 17:25:57 UTC (rev 42627)
> @@ -171,16 +171,33 @@
>
>                if (_vm->_mouse->lButtonPressed() &&
> _currentRoom._walkingMap.isWalkable(x, y)) {
>
> +                       // Fetch dragon's animation ID
> +                       // FIXME: Need to add proper walking (this only
> warps the dragon to position)
>                        int animID = getObject(kDragonObject)->_anims[0];
>
>                        Animation *anim = _vm->_anims->getAnimation(animID);
>                        Drawable *frame = anim->getFrame();
>
> +                       // Calculate scaling factor
> +                       double scaleX = _currentRoom._pers0 +
> _currentRoom._persStep * y;
>

this should be a method of _currentRoom rather than having the logic here


> +                       // Set the scaling factor
> +                       anim->setScaling(scaleX, scaleY);
>

it's good that setScaling() accepts both scaleX and scaleY, for generality.
however, locally here, there is no reason to have two such variables defined
if you use the same value for them.  just define scale and call
setScaling(scale, scale);

Modified: scummvm/branches/gsoc2009-draci/engines/draci/game.h
> ===================================================================
> --- scummvm/branches/gsoc2009-draci/engines/draci/game.h        2009-07-20
> 09:19:27 UTC (rev 42626)
> +++ scummvm/branches/gsoc2009-draci/engines/draci/game.h        2009-07-20
> 17:25:57 UTC (rev 42627)
> @@ -134,6 +134,9 @@
>
>  class Game {
>
> +       // HACK: Remove this before committing; if anyone sees this, remind
> me :D
> +       friend class Animation;
>

reminder


>
> Modified: scummvm/branches/gsoc2009-draci/engines/draci/sprite.cpp
> ===================================================================
> --- scummvm/branches/gsoc2009-draci/engines/draci/sprite.cpp    2009-07-20
> 09:19:27 UTC (rev 42626)
> +++ scummvm/branches/gsoc2009-draci/engines/draci/sprite.cpp    2009-07-20
> 17:25:57 UTC (rev 42627)
> @@ -115,6 +117,87 @@
>        _mirror = false;
>  }
>
> +// TODO: Research what kind of sampling the original player uses


nearest neighbor.  we don't do any smoothing.  just some kind of rounding
similar to the one in your code


>
> +void Sprite::drawScaled(Surface *surface, double scaleX, double scaleY,
> bool markDirty) const {
> +
> +       Common::Rect sourceRect(0, 0, _width, _height);
> +       Common::Rect destRect(_x, _y, _x + getScaledWidth(scaleX), _y +
> getScaledHeight(scaleY));
> +       Common::Rect surfaceRect(0, 0, surface->w, surface->h);
>

you may need in the future an alternative procedure where the cropping
rectangle is specified by the caller.  you may not need it if your drawer is
built significantly differently from our old one.


> +       // Calculate how many rows and columns we need to draw
> +       const int rows = clippedDestRect.bottom - clippedDestRect.top;
> +       const int columns = clippedDestRect.right - clippedDestRect.left;
>

generally, it seems to me that from the top to here you do way too many
calculations and that they could possibly be simplified, but don't worry
about it, since this is just an initialization code and not something
time-critical called inside a loop.


> +       int *rowIndices = new int[rows];
> +       int *columnIndices = new int[columns];
> +
> +       // Precalculate pixel indexes
> +       for (int i = 0; i < rows; ++i) {
> +               rowIndices[i] = lround(i / scaleY);
> +       }
> +
> +       for (int j = 0; j < columns; ++j) {
> +               columnIndices[j] = lround(j / scaleX);
> +       }
>

I don't like this too much, because precomputing and storing the indices is
not necessary at all.  we used to use a simple adaptive linear algorithm,
working only on integers.  you use floats and store the positions.  I
believe that code is simpler and faster.

however, since you've probably already debugged the algorithm and have much
more more urgent and important work to do, it would be best to just add a
TODO, and one day someone may replace this black-box algorithm by something
more elegant.


@@ -175,6 +262,18 @@
>        return Common::Rect(_x, _y, _x + _width, _y + _height);
>  }
>
> +uint Sprite::getScaledHeight(double scaleY) const {
> +       return lround(scaleY * _height);
> +}
> +
> +uint Sprite::getScaledWidth(double scaleX) const {
> +       return lround(scaleX * _width);
> +}
>

should you ever decide to get rid of doubles and only have integers in the
interface (for simplicity and determinism, because floats are often
implementation-dependent), you may use targetWidth and targetHeight instead
of the scale factors.  I believe this is what we used in the old player.
this is not essential, again.


> Modified: scummvm/branches/gsoc2009-draci/engines/draci/sprite.h
> ===================================================================
> --- scummvm/branches/gsoc2009-draci/engines/draci/sprite.h      2009-07-20
> 09:19:27 UTC (rev 42626)
> +++ scummvm/branches/gsoc2009-draci/engines/draci/sprite.h      2009-07-20
> 17:25:57 UTC (rev 42627)
> @@ -38,11 +38,17 @@
>
>  public:
>        virtual void draw(Surface *surface, bool markDirty = true) const =
> 0;
> +       virtual void drawScaled(Surface *surface, double scaleX, double
> scaleY,
> +               bool markDirty = true) const = 0;
>

as I said, let the caller not worry about scaling and instead treat scaled
sprites as another kind of sprite, which just needs to be drawn via the
usual sprite interface.  so remove drawScaled()


> @@ -53,6 +59,7 @@
>        int getDelay() { return _delay; }
>
>        virtual Common::Rect getRect() const = 0;
> +       virtual Common::Rect getScaledRect(double scaleX, double scaleY)
> const = 0;
>

... and getScaledRect()

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