hi Denis,<br><br><div class="gmail_quote">On Sat, Jul 4, 2009 at 8:24 PM, <span dir="ltr"><<a href="mailto:dkasak13@users.sourceforge.net">dkasak13@users.sourceforge.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
Revision: 42114<br>
<a href="http://scummvm.svn.sourceforge.net/scummvm/?rev=42114&view=rev" target="_blank">http://scummvm.svn.sourceforge.net/scummvm/?rev=42114&view=rev</a><br>
Author: dkasak13<br>
Date: 2009-07-05 03:24:46 +0000 (Sun, 05 Jul 2009)<br>
<br>
Log Message:<br>
-----------<br>
* 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).<br>
* Implemented actual animation (previously only the first frame was display)<br>
* Implemented animation starting, stoping, looping<br>
* Loaded looping dragon animation as a test<br></blockquote><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><br>
Modified: scummvm/branches/gsoc2009-draci/engines/draci/animation.cpp<br>
===================================================================<br>
--- scummvm/branches/gsoc2009-draci/engines/draci/animation.cpp 2009-07-05 01:16:53 UTC (rev 42113)<br>
+++ scummvm/branches/gsoc2009-draci/engines/draci/animation.cpp 2009-07-05 03:24:46 UTC (rev 42114)<br>+ // If we are at the last frame and not looping, stop the animation<br>
+ // The animation is also restarted to frame zero<br>
+ if ((_currentFrame == nextFrameNum() - 1) && !_looping) {<br>
+ _currentFrame = 0;<br>
+ _playing = false;<br>
+ return;<br>
+ }</blockquote><div><br></div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
+ if (force || (_tick + _delay <= _vm->_system->getMillis())) {<br>
+ _vm->_screen->getSurface()->markDirtyRect(frameRect);<br>
+ _currentFrame = nextFrameNum();<br>
+ _tick = _vm->_system->getMillis();<br>
+ }</blockquote><div><br></div><div>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.</div><div><br>
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">+uint AnimObj::nextFrameNum() {<br>
+<br>
+ if ((_currentFrame == getFramesNum() - 1) && _looping)<br>
+ return 0;</blockquote><div><br></div><div>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.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">+ if (_id == kOverlayImage) {<br>
+ _frames[_currentFrame]->draw(surface, false);<br>
+ }<br>
+ else {<br>
+ _frames[_currentFrame]->draw(surface, true);<br>
+ }</blockquote><div><br></div><div>it would be more readable to say:</div><div><br></div><div>const bool is_overlay = _id == kOverlayImage;</div><div>_frames[_currentFrame]->draw(surface, is_overlay);</div><div>
<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">+void Animation::play(int id) {</blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
+<br>
+ AnimObj *obj = getAnimation(id);<br>
+<br>
+ obj->setPlaying(true);<br>
+}</blockquote><div><br></div><div>in all functions similar to this, wouldn't it be shorter and still quite comprehensible to say getAnimation(id)->setPlaying(true); ?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
+AnimObj *Animation::getAnimation(int id) {<br>
+<br>
+ Common::List<AnimObj *>::iterator it;<br>
+<br>
for (it = _animObjects.begin(); it != _animObjects.end(); ++it) {<br>
- if (it->_id == id) {<br>
- return it;<br>
+ if ((*it)->getID() == id) {<br>
+ return *it;<br>
}<br>
}<br>
<br>
- return _animObjects.end();<br>
+ return *_animObjects.end();</blockquote><div><br></div><div>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.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><br>
}<br>
<br>
-void Animation::insertAnimation(AnimObj &animObj) {<br>
+void Animation::insertAnimation(AnimObj *animObj) {<br>
<br>
- Common::List<AnimObj>::iterator it;<br>
+ Common::List<AnimObj *>::iterator it;<br>
<br>
for (it = _animObjects.begin(); it != _animObjects.end(); ++it) {<br>
- if (animObj._z < it->_z)<br>
+ if (animObj->getZ() < (*it)->getZ())</blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><br>Modified: scummvm/branches/gsoc2009-draci/engines/draci/animation.h<br>
===================================================================<br>
--- scummvm/branches/gsoc2009-draci/engines/draci/animation.h 2009-07-05 01:16:53 UTC (rev 42113)<br>
+++ scummvm/branches/gsoc2009-draci/engines/draci/animation.h 2009-07-05 03:24:46 UTC (rev 42114)<br>
@@ -30,44 +30,81 @@<br><br>- Common::List<AnimObj> _animObjects;<br>
+ Common::List<AnimObj *> _animObjects;</blockquote><div><br></div><div>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).</div>
<div><br></div><div>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.</div>
<div><br></div><div>maybe you have a reason to do this, but I didn't get it from this commit.</div><div><br></div><div>a nice commit otherwise!</div></div>-- <br>Robert Špalek <<a href="mailto:rspalek@gmail.com">rspalek@gmail.com</a>><br>