hi Denis,<br><br><div class="gmail_quote">On Mon, Jul 27, 2009 at 6:18 AM, <span dir="ltr"><<a href="mailto:dkasak13@users.sourceforge.net">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: 42836<br>
<a href="http://scummvm.svn.sourceforge.net/scummvm/?rev=42836&view=rev" target="_blank">http://scummvm.svn.sourceforge.net/scummvm/?rev=42836&view=rev</a><br>
Author: dkasak13<br>
Date: 2009-07-27 04:18:44 +0000 (Mon, 27 Jul 2009)<br>
<br>
Log Message:<br>
-----------<br>
* Added AnimationManager::addText() for adding text animations<br>
* Added AnimationManager::getTopAnimationID(x, y) which returns the ID of the top layer animation located on a point<br>
* Added Animation::getScale{X,Y}()<br>
* Fixed a few bugs related to animations sometimes having no frames<br>
<br>
Modified Paths:<br>
--------------<br>
scummvm/branches/gsoc2009-draci/engines/draci/animation.cpp<br>
scummvm/branches/gsoc2009-draci/engines/draci/animation.h<br>
<br>
Modified: scummvm/branches/gsoc2009-draci/engines/draci/animation.cpp<br>
===================================================================<br>
--- scummvm/branches/gsoc2009-draci/engines/draci/animation.cpp 2009-07-27 04:14:59 UTC (rev 42835)<br>
+++ scummvm/branches/gsoc2009-draci/engines/draci/animation.cpp 2009-07-27 04:18:44 UTC (rev 42836)<br>
@@ -227,6 +235,11 @@<br>
<br>
Drawable *Animation::getFrame(int frameNum) {<br>
<br>
+ // If there are no frames stored, return NULL<br>
+ if (_frames.size() == 0) {<br>
+ return NULL;<br>
+ }<br>
+</blockquote><div><br>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.<br>
<br>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<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;">+Animation *AnimationManager::addText(int id, bool playing) {<br>
+<br>
+ Animation *anim = new Animation(_vm, kIgnoreIndex);<br>
+<br>
+ anim->setID(id);<br>
+ anim->setZ(256);<br>
+ anim->setPlaying(playing);<br>
+<br>
+ insertAnimation(anim);<br>
+<br>
+ return anim;<br>
+}</blockquote><div><br>I'm a bit confused that I don't see reading the concrete text anywhere. it is stored in the BArchive separately<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>
+int AnimationManager::getTopAnimationID(int x, int y) {<br>
+<br>
+ Common::List<Animation *>::iterator it;<br>
+<br>
+ // The default return value if no animations were found on these coordinates (not even overlays)<br>
+ // i.e. the black background shows through so treat it as an overlay<br>
+ int retval = kOverlayImage;<br>
+<br>
+ // Get transparent colour for the current screen<br>
+ const int transparent = _vm->_screen->getSurface()->getTransparentColour();<br>
+<br>
+ for (it = _animations.reverse_begin(); it != _animations.end(); --it) {<br>
+<br>
+ Animation *anim = *it;<br>
+<br>
+ // If the animation is not playing, ignore it<br>
+ if (!anim->isPlaying()) {<br>
+ continue;<br>
+ }<br>
+<br>
+ Drawable *frame = anim->getFrame();<br>
+<br>
+ if (frame == NULL) {<br>
+ continue;<br>
+ }<br>
</blockquote><div><br>for clarity, this if would be nice to get rid of if my above comment is valid<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>
+ int oldX = frame->getX();<br>
+ int oldY = frame->getY();<br>
+<br>
+ // Take account relative coordinates<br>
+ int newX = oldX + anim->getRelativeX();<br>
+ int newY = oldY + anim->getRelativeY();<br>
+<br>
+ // Translate the frame to those relative coordinates<br>
+ frame->setX(newX);<br>
+ frame->setY(newY);<br>
+<br>
+ // Save scaled width and height<br>
+ int scaledWidth = frame->getScaledWidth();<br>
+ int scaledHeight = frame->getScaledHeight();<br>
+<br>
+ // Take into account per-animation scaling and adjust the current frames dimensions<br>
+ if (anim->getScaleX() != 1.0 || anim->getScaleY() != 1.0)<br>
+ frame->setScaled(scaledWidth * anim->getScaleX(), scaledHeight * anim->getScaleY());<br>
+<br>
+ if (frame->getRect().contains(x, y)) {<br>
+<br>
+ if (frame->getType() == kDrawableText) {<br>
+<br>
+ retval = anim->getID();<br>
+<br>
+ } else if (frame->getType() == kDrawableSprite &&<br>
+ reinterpret_cast<Sprite *>(frame)->getPixel(x, y) != transparent) {<br>
+<br>
+ retval = anim->getID();<br>
+ }<br>
+ }<br>
</blockquote><div><br>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?<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>
+ // Revert back to old coordinates<br>
+ frame->setX(oldX);<br>
+ frame->setY(oldY);<br>
+<br>
+ // Revert back to old dimensions<br>
+ frame->setScaled(scaledWidth, scaledHeight);<br>
+<br>
+ // Found an animation<br>
+ if (retval != kOverlayImage)<br>
+ break;<br>
+ }<br>
</blockquote><div><br>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.<br>
<br></div></div><br>-- <br>Robert Špalek <<a href="mailto:rspalek@gmail.com">rspalek@gmail.com</a>><br>