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

[Denis Kasak]

Hi Robert,

On Tue, Aug 11, 2009 at 07:52, Robert Špalek<rspalek at gmail.com> wrote:
> hi Denis!
>
> this is a very impressive commit!  I'm really excited as it pushes the game
> much further!

Thanks. It's indeed great to see the dialogues working. :-)

> On Mon, Aug 10, 2009 at 9:03 PM, <dkasak13 at users.sourceforge.net> wrote:
>>
>> Revision: 43253
>>          http://scummvm.svn.sourceforge.net/scummvm/?rev=43253&view=rev
>> Author:   dkasak13
>> Date:     2009-08-11 04:03:22 +0000 (Tue, 11 Aug 2009)
>>
>> Log Message:
>> -----------
>> Added dialogue support.
>
> great job!!!
>
>>
>> Modified: scummvm/branches/gsoc2009-draci/engines/draci/game.cpp
>> ===================================================================
>> --- scummvm/branches/gsoc2009-draci/engines/draci/game.cpp      2009-08-11
>> 02:12:24 UTC (rev 43252)
>> +++ scummvm/branches/gsoc2009-draci/engines/draci/game.cpp      2009-08-11
>> 04:03:22 UTC (rev 43253)
>> @@ -61,21 +63,24 @@
>>        // Close persons file
>>        file->close();
>>
>> -       // Read in dialog offsets
>> +       // Read in dialogue offsets
>
> I see that you like British English :-)

Well, in general, yes; I prefer BrE. I reflexively used "dialog"
initially (for the few such identifiers that existed before dialogue
support) because that is how it's done in the original player's code
and hadn't really noticed it. However, in the subsequent dialogue
implementation, I (again reflexively :) ) used dialogue exclusively.
Since this was the bigger part of the code, I decided to just change
to old identifiers to British for consistency.

>> @@ -227,6 +232,19 @@
>>        Text *speech = new Text("", _vm->_bigFont, kFontColour1, 0, 0);
>>        speechAnim->addFrame(speech);
>>
>> +       for (uint i = 0; i < kDialogueLines; ++i) {
>> +               _dialogueAnims[i] = _vm->_anims->addText(-10 - i, true);
>
> is -10 a magical constant making sure the texts have unique animation ID?
>  maybe you should declare a constant for it and say in the header file how
> many numbers are allocated around it

Yes, it's a magical constant. The code from yesterday isn't very
polished; it was a real ordeal getting it to work and I was really
tired afterwards so I decided to do the polish today. There are many
such aesthetically unappealing quirks in the current implementation
(for instance, everything dialogue related is public now). I'll fix
that today.

>> +               Text *dialogueLine0 = new Text("", _vm->_smallFont,
>> kLineInactiveColour, 0, 0);
>> +               _dialogueAnims[i]->addFrame(dialogueLine0);
>> +
>> +               _dialogueAnims[i]->setZ(254);
>> +               _dialogueAnims[i]->setRelative(1,
>> +                       kScreenHeight - (i + 1) *
>> _vm->_smallFont->getFontHeight());
>
> maybe you wanna increase the font height by 1 pixel before multiplication,
> to make sure there is a little gap between the lines

I initially thought that this will be needed everywhere, however gaps
seem to be incorporated in the fonts already. Are you suggesting this
as an aesthetic improvement? I'm asking because in the original
player, there is no gap explicitly added.

>> @@ -469,7 +501,193 @@
>>
>>        return kObjectNotFound;
>>  }
>> +
>> +void Game::dialogueMenu(int dialogueID) {
>>
>> +       int oldLines, hit;
>> +
>> +       char tmp[5];
>> +       sprintf(tmp, "%d", dialogueID+1);
>>
>> +       Common::String ext(tmp);
>> +       _dialogueArchive = new BArchive(dialoguePath + ext + ".dfw");
>
> more elegant is to do all path forming at once, i.e. sprintf(tmp,
> "%s%d.dfw", ...), but then you need to allocate more space for tmp, of
> course.

Agreed.

>>
>> +       debugC(4, kDraciLogicDebugLevel, "Starting dialogue (ID: %d,
>> Archive: %s)",
>> +               dialogueID, (dialoguePath + ext + ".dfw").c_str());
>
> especially since here you form the path again.  just reuse the same variable
> then
>
>>
>> +       do {
>> +               _dialogueExit = false;
>> +               hit = dialogueDraw();
>> +
>> +               debug(2, "Hit: %d, _lines[hit]: %d", hit, _lines[hit]);
>> +
>> +               if ((!_dialogueExit) && (hit != -1) && (_lines[hit] !=
>> -1)) {
>
> this condition is quite complicated.  I would rather test for the opposite
> and break, and then this true-branch need not be indented.

Copied from the original. I'm not sure I even understand it
completely, particularly because the original engine checks for 0 and
I couldn't deduce what should be 0-indexed and what not, so this is
largely a result of trial-and-error. I'll see what I can do.

>> +void Game::runDialogueProg(GPL2Program prog, int offset) {
>> +
>> +       // Mark last animation
>> +       int lastAnimIndex = _vm->_anims->getLastIndex();
>> +
>> +       // Run gate program
>> +       _vm->_script->run(prog, offset);
>
> fix the comment.  there is not gate here

Oops. That's what I get for copy-pasting code. :D

>> ]+       // Delete all animations loaded after the marked one
>> +       // (from objects and from the AnimationManager)
>> +       for (uint i = 0; i < getNumObjects(); ++i) {
>> +               GameObject *obj = &_objects[i];
>> +
>> +               for (uint j = 0; j < obj->_anims.size(); ++j) {
>> +                       Animation *anim;
>> +
>> +                       anim = _vm->_anims->getAnimation(obj->_anims[j]);
>> +                       if (anim != NULL && anim->getIndex() >
>> lastAnimIndex)
>> +                               obj->_anims.remove_at(j);
>> +               }
>> +       }
>
> I think I have seen a similar loop before and maybe more than once.  perhaps
> they all have something in common and can be refactored into a separate
> method.

Indeed. I have been thinking of doing it but haven't got around it yet.

>> Modified: scummvm/branches/gsoc2009-draci/engines/draci/game.h
>> ===================================================================
>> --- scummvm/branches/gsoc2009-draci/engines/draci/game.h        2009-08-11
>> 02:12:24 UTC (rev 43252)
>> +++ scummvm/branches/gsoc2009-draci/engines/draci/game.h        2009-08-11
>> 04:03:22 UTC (rev 43253)
>> @@ -27,6 +27,7 @@
>>  #define DRACI_GAME_H
>>
>>  #include "common/str.h"
>> +#include "draci/barchive.h"
>>  #include "draci/script.h"
>>  #include "draci/animation.h"
>>  #include "draci/sprite.h"
>> @@ -68,6 +69,10 @@
>>        kDefaultRoomMap = -1
>>  };
>>
>> +enum {
>> +       kNoDialogue = -1, kDialogueLines = 4
>> +};
>
> there is one occurrence of raw constant 4 in dialogueDraw()

Okay, changing it.

>> @@ -146,6 +151,14 @@
>>        byte _fontColour;
>>  };
>>
>> +struct Dialogue {
>> +       int _canLen;
>> +       byte *_canBlock;
>
> since this pair of variables for a mathematical expression is stored in many
> objects in the game, maybe it would be worth wrapping them by a simple class
> or struct, similarly to GPL2Program.

I'll check it out.

>> @@ -286,6 +304,22 @@
>>
>>        int _currentIcon;
>>
>> +// HACK: remove public when tested and add getters instead
>> +public:
>> +       uint *_dialogueOffsets;
>> +       int _currentDialogue;
>> +       int *_dialogueVars;
>> +       BArchive *_dialogueArchive;
>> +       Dialogue *_dialogueBlocks;
>> +       bool _dialogueBegin;
>> +       bool _dialogueExit;
>> +       int _currentBlock;
>> +       int _lastBlock;
>> +       int _dialogueLines;
>> +       int _blockNum;
>> +       int _lines[4];
>
> suggest to rename _lines to something more specific.  this is a very
> specific variable, so it's confusing to have such a general name.

Most of these identifiers can have a better name. For instance,
_dialogueLines should actually be something suggestive of a count and
_lines could be named _dialogueLines instead or something similar. I
only used these names while implementing it to maintain similarity to
the original so I have one less thing to remap mentally.

>> +       Animation *_dialogueAnims[4];
>
> here and above, use kDialogueLines

Yes, thanks.

>> Modified: scummvm/branches/gsoc2009-draci/engines/draci/script.cpp
>> ===================================================================
>> --- scummvm/branches/gsoc2009-draci/engines/draci/script.cpp    2009-08-11
>> 02:12:24 UTC (rev 43252)
>> +++ scummvm/branches/gsoc2009-draci/engines/draci/script.cpp    2009-08-11
>> 04:03:22 UTC (rev 43253)
>> @@ -218,6 +218,27 @@
>> +int Script::funcBlockVar(int blockID) {
>> +       return
>> _vm->_game->_dialogueVars[_vm->_game->_dialogueOffsets[_vm->_game->_currentDialogue]
>> + blockID];
>
> why do you use blockID alone here, whereas you use _lastBlock + 1 in
> game.cpp:549 ?

Most likely an error on my part. Now that everything is at least
partially working, I'll have to re-read my implementation to fix such
errors.

>> +int Script::funcHasBeen(int blockID) {
>> +       return
>> _vm->_game->_dialogueVars[_vm->_game->_dialogueOffsets[_vm->_game->_currentDialogue]
>> + blockID] > 0;
>
> ditto
>
>>
>> @@ -373,9 +394,12 @@
>>                return;
>>        }
>>
>> -       int objID = params.pop() - 1;
>> -       int animID = params.pop() - 1;
>> +       int objID = params.pop();
>> +       int animID = params.pop();
>>
>> +       objID -= 1;
>> +       animID -= 1;
>
> why this?  it doesn't make anything more readable and you still use the
> simpler (and preferred, by me) form at many other places.

This was an intermediate stage before I committed the patch for the
intro crash bug.

>> +void Script::resetBlock(Common::Queue<int> &params) {
>> +       int blockID = params.pop();
>> +
>> +
>> _vm->_game->_dialogueVars[_vm->_game->_dialogueOffsets[_vm->_game->_currentDialogue]+blockID]
>> = 0;
>
> not sure if you should use blockID or subtract 1 from it, due to 1-indexing
> vs. 0-indexing, and the confused addition of +1 above

TBH, I'm not sure either. :-)
I'll revisit that today.

-- 
Denis Kasak