hi Denis,<br><br>sorry for a long delay. my vacation is near the end and I have found time to send you some comments.<br><br>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.<br>
<br>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.<br>
<br><div class="gmail_quote">On Mon, Jul 20, 2009 at 7:25 PM, <span dir="ltr"><<a href="mailto:dkasak13@users.sourceforge.net" target="_blank">dkasak13@users.sourceforge.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
Revision: 42627<br>
<a href="http://scummvm.svn.sourceforge.net/scummvm/?rev=42627&view=rev" target="_blank">http://scummvm.svn.sourceforge.net/scummvm/?rev=42627&view=rev</a><br>
Author: dkasak13<br>
Date: 2009-07-20 17:25:57 +0000 (Mon, 20 Jul 2009)<br>
<br>
Log Message:<br>
-----------<br>
* Added scaling support<br>
* Made the dragon scale when it is in different parts of the room<br>
* Added getters for relative coordinates (Animation::getRelativeX() and Animation::getRelativeY())<br>
* Commented Game::loop() and Sprite::draw*() methods in more detail<br>
<br>
Modified: scummvm/branches/gsoc2009-draci/engines/draci/game.cpp<br>
===================================================================<br>
--- scummvm/branches/gsoc2009-draci/engines/draci/game.cpp 2009-07-20 09:19:27 UTC (rev 42626)<br>
+++ scummvm/branches/gsoc2009-draci/engines/draci/game.cpp 2009-07-20 17:25:57 UTC (rev 42627)<br>
@@ -171,16 +171,33 @@<br>
<br>
if (_vm->_mouse->lButtonPressed() && _currentRoom._walkingMap.isWalkable(x, y)) {<br>
<br>
+ // Fetch dragon's animation ID<br>
+ // FIXME: Need to add proper walking (this only warps the dragon to position)<br>
int animID = getObject(kDragonObject)->_anims[0];<br>
<br>
Animation *anim = _vm->_anims->getAnimation(animID);<br>
Drawable *frame = anim->getFrame();<br>
<br>
+ // Calculate scaling factor<br>
+ double scaleX = _currentRoom._pers0 + _currentRoom._persStep * y;<br>
</blockquote><div><br>this should be a method of _currentRoom rather than having the logic here<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
+ // Set the scaling factor<br>
+ anim->setScaling(scaleX, scaleY);<br>
</blockquote><div><br>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);<br>
<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">Modified: scummvm/branches/gsoc2009-draci/engines/draci/game.h<br>
===================================================================<br>
--- scummvm/branches/gsoc2009-draci/engines/draci/game.h 2009-07-20 09:19:27 UTC (rev 42626)<br>
+++ scummvm/branches/gsoc2009-draci/engines/draci/game.h 2009-07-20 17:25:57 UTC (rev 42627)<br>
@@ -134,6 +134,9 @@<br>
<br>
class Game {<br>
<br>
+ // HACK: Remove this before committing; if anyone sees this, remind me :D<br>
+ friend class Animation;<br>
</blockquote><div><br>reminder<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>
Modified: scummvm/branches/gsoc2009-draci/engines/draci/sprite.cpp<br>
===================================================================<br>
--- scummvm/branches/gsoc2009-draci/engines/draci/sprite.cpp 2009-07-20 09:19:27 UTC (rev 42626)<br>
+++ scummvm/branches/gsoc2009-draci/engines/draci/sprite.cpp 2009-07-20 17:25:57 UTC (rev 42627)<br>
@@ -115,6 +117,87 @@<br>
_mirror = false;<br>
}<br>
<br>
+// TODO: Research what kind of sampling the original player uses</blockquote><div><br>nearest neighbor. we don't do any smoothing. just some kind of rounding similar to the one in your code<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
+void Sprite::drawScaled(Surface *surface, double scaleX, double scaleY, bool markDirty) const {<br>
+<br>
+ Common::Rect sourceRect(0, 0, _width, _height);<br>
+ Common::Rect destRect(_x, _y, _x + getScaledWidth(scaleX), _y + getScaledHeight(scaleY));<br>
+ Common::Rect surfaceRect(0, 0, surface->w, surface->h);<br>
</blockquote><div><br>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.<br>
<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">+ // Calculate how many rows and columns we need to draw<br>
+ const int rows = clippedDestRect.bottom - clippedDestRect.top;<br>
+ const int columns = clippedDestRect.right - clippedDestRect.left;<br>
</blockquote><div><br>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.<br>
<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">+ int *rowIndices = new int[rows];<br>
+ int *columnIndices = new int[columns];<br>
+<br>
+ // Precalculate pixel indexes<br>
+ for (int i = 0; i < rows; ++i) {<br>
+ rowIndices[i] = lround(i / scaleY);<br>
+ }<br>
+<br>
+ for (int j = 0; j < columns; ++j) {<br>
+ columnIndices[j] = lround(j / scaleX);<br>
+ }<br>
</blockquote><div><br>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.<br>
<br>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.<br>
<br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">@@ -175,6 +262,18 @@<br>
return Common::Rect(_x, _y, _x + _width, _y + _height);<br>
}<br>
<br>
+uint Sprite::getScaledHeight(double scaleY) const {<br>
+ return lround(scaleY * _height);<br>
+}<br>
+<br>
+uint Sprite::getScaledWidth(double scaleX) const {<br>
+ return lround(scaleX * _width);<br>
+}<br>
</blockquote><div><br>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.<br>
<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">Modified: scummvm/branches/gsoc2009-draci/engines/draci/sprite.h<br>
===================================================================<br>
--- scummvm/branches/gsoc2009-draci/engines/draci/sprite.h 2009-07-20 09:19:27 UTC (rev 42626)<br>
+++ scummvm/branches/gsoc2009-draci/engines/draci/sprite.h 2009-07-20 17:25:57 UTC (rev 42627)<br>
@@ -38,11 +38,17 @@<br>
<br>
public:<br>
virtual void draw(Surface *surface, bool markDirty = true) const = 0;<br>
+ virtual void drawScaled(Surface *surface, double scaleX, double scaleY,<br>
+ bool markDirty = true) const = 0;<br>
</blockquote><div><br>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()<br>
<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">@@ -53,6 +59,7 @@<br>
int getDelay() { return _delay; }<br>
<br>
virtual Common::Rect getRect() const = 0;<br>
+ virtual Common::Rect getScaledRect(double scaleX, double scaleY) const = 0;<br>
</blockquote><div><br>... and getScaledRect()<br> <br></div>best,<br></div>-- <br>Robert Špalek <<a href="mailto:rspalek@gmail.com" target="_blank">rspalek@gmail.com</a>><br>