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

[Robert Špalek]

hi Denis,
good job!  I still have a few questions and suggestions though.

On Wed, Jul 29, 2009 at 12:41 PM, <dkasak13 at users.sourceforge.net> wrote:

> Revision: 42902
>          http://scummvm.svn.sourceforge.net/scummvm/?rev=42902&view=rev
> Author:   dkasak13
> Date:     2009-07-29 19:41:30 +0000 (Wed, 29 Jul 2009)
>
> Log Message:
> -----------
> * Implemented the StartPlay and Play GPL commands properly
> * Changed Script::load() to use the new animation callbacks
>
> Modified Paths:
> --------------
>    scummvm/branches/gsoc2009-draci/engines/draci/script.cpp
>    scummvm/branches/gsoc2009-draci/engines/draci/script.h
>
> Modified: scummvm/branches/gsoc2009-draci/engines/draci/script.cpp
> ===================================================================
> --- scummvm/branches/gsoc2009-draci/engines/draci/script.cpp    2009-07-29
> 19:39:10 UTC (rev 42901)
> +++ scummvm/branches/gsoc2009-draci/engines/draci/script.cpp    2009-07-29
> 19:41:30 UTC (rev 42902)
> @@ -289,7 +289,15 @@
>  /* GPL commands */
>
>  void Script::play(Common::Queue<int> &params) {
> +       if (_vm->_game->getLoopStatus() == kStatusInventory) {
> +               return;
> +       }
> +
> +       _vm->_game->setLoopStatus(kStatusStrange);
> +       _vm->_game->setExitLoop(true);
>        _vm->_game->loop();
> +       _vm->_game->setExitLoop(false);
> +       _vm->_game->setLoopStatus(kStatusOrdinary);


- when you finish the code, rename Strange into Inner or something like that
- not sure if the original LoopStatus could be different than Ordinary.  I
know that you check for Inventory and exit, but maybe there is yet another
one.  if this can happen, it would be safer to remember the original status
and restore it rather than forcing Ordinary


> +       // Stop all animation that the object owns
> +
> +       for (uint i = 0; i < obj->_anims.size(); ++i) {
> +               _vm->_anims->stop(obj->_anims[i]);
> +       }


can it happen that there is more than 1 currently playing?  the player would
handle that, but it seems to be weird to me.


> @@ -345,6 +362,41 @@
>        }
>  }
>
> +void Script::startPlay(Common::Queue<int> &params) {
> +       if (_vm->_game->getLoopStatus() == kStatusInventory) {
> +               return;
> +       }
> +
> +       int objID = params.pop() - 1;
> +       int animID = params.pop() - 1;
> +
> +       GameObject *obj = _vm->_game->getObject(objID);
> +
> +       // Stop all animation that the object owns
> +
> +       for (uint i = 0; i < obj->_anims.size(); ++i) {
> +               _vm->_anims->stop(obj->_anims[i]);
> +       }


ditto


> +       Animation *anim = _vm->_anims->getAnimation(animID);
> +       anim->registerCallback(&Animation::exitGameLoop);
> +
> +       _vm->_game->setLoopStatus(kStatusStrange);
> +       _vm->_anims->play(animID);
> +       _vm->_game->loop();
> +       _vm->_game->setExitLoop(false);
> +       _vm->_anims->stop(animID);
> +       _vm->_game->setLoopStatus(kStatusOrdinary);


restoring original status instead of forcing Ordinary, like above


> +       anim->registerCallback(&Animation::doNothing);


restore the original callback if it could ever be different than Nothing


> +       bool visible = (objID == kDragonObject || obj->_visible);
> +
> +       if (visible && (obj->_location == _vm->_game->getRoomNum())) {
> +               _vm->_anims->play(animID);
> +       }


does the original player do this, too?  it seems weird.  you have just
player animation animID and waited for it to finish.  if you should play any
animation on that object, it should probably be exactly the one playing
before, but not the one just played.  this is just my intuition and I know
that the original player is a mess...


> @@ -567,7 +619,12 @@
>  /**
>  * @brief Find the current command in the internal table
>  *
> - * @param num          Command number
> + * @param G_LoopStatus=Inventory then Exit;
> +  G_LoopSubStatus:= Strange;
> +  G_QuitLoop:= True;
> +  Loop;
> +  G_QuitLoop:= False;
> +  G_LoopSubStatus:= Ordinary; num              Command number
>  * @param subnum       Command subnumer
>  *
>  * @return NULL if command is not found. Otherwise, a pointer to a
> GPL2Command
>

is it wise to copy & paste more or less a complete code into the comment?
 I'd rather say something like it runs an inner loop.


> Modified: scummvm/branches/gsoc2009-draci/engines/draci/script.h
> ===================================================================
> --- scummvm/branches/gsoc2009-draci/engines/draci/script.h      2009-07-29
> 19:39:10 UTC (rev 42901)
> +++ scummvm/branches/gsoc2009-draci/engines/draci/script.h      2009-07-29
> 19:41:30 UTC (rev 42902)
> @@ -113,6 +113,7 @@
>        void execUse(Common::Queue<int> &params);
>        void walkOn(Common::Queue<int> &params);
>        void play(Common::Queue<int> &params);
> +       void startPlay(Common::Queue<int> &params);
>

also, one day, we should rename these methods.  play/startPlay/start sound
like really bad, nothing revealing, names.  (I know they come from the
original player.)

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