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

[Denis Kasak]

Hi Robert,

On Wed, Jul 29, 2009 at 22:01, Robert Špalek<rspalek at gmail.com> wrote:
> hi Denis,
> good job!  I still have a few questions and suggestions though.

Thank you. :-)

>> 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

I agree. I'm going to take notes of every rename request and once the
game is playable to some degree, I'll rename it. For now, I'm
postponing it so there is a better correspondence with the original
code so I can find my way around.

> - 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

This is copied 1-to-1 from the original engine so, at this point, I
cannot give you a better answer. I'll make a note of this so I
remember to check it if there are any problems, but for now, I think
it's safer to leave it the way the original player does it.

In fact, for all your comments here, the answer is the same. They are
all modeled according to the original player's commands. :-)

>> +       // 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.

Same as above. Also, I think I remember seeing some objects having
smaller animations started on top of the bigger ones, so the answer
may even be "yes". Not sure, though.

>> @@ -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

Same as above.

>> +       anim->registerCallback(&Animation::doNothing);
>
> restore the original callback if it could ever be different than Nothing

The original engine does this and has a comment above it that says:

    {???to nasledujici tady snad nemusi byt:???}

I think that means even the author was unsure if that should be there.
Correct? :-D

>> +       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...

This one was a mistake. I forgot to remove the old code once I coded
StartPlay properly.

>> @@ -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.

Apparently, I didn't check the diff closely enough before I made the
commit. That was an accident. :-)

>> 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.)

Agreed.

-- 
Denis Kasak