hi Denis!<br><br><div>this is a very impressive commit!  I'm really excited as it pushes the game much further!</div><div><br><div class="gmail_quote">On Mon, Aug 10, 2009 at 9:03 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: 43253<br>
          <a href="http://scummvm.svn.sourceforge.net/scummvm/?rev=43253&view=rev" target="_blank">http://scummvm.svn.sourceforge.net/scummvm/?rev=43253&view=rev</a><br>
Author:   dkasak13<br>
Date:     2009-08-11 04:03:22 +0000 (Tue, 11 Aug 2009)<br>
<br>
Log Message:<br>
-----------<br>
Added dialogue support.</blockquote><div><br></div><div>great job!!!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">Modified: scummvm/branches/gsoc2009-draci/engines/draci/game.cpp<br>


===================================================================<br>
--- scummvm/branches/gsoc2009-draci/engines/draci/game.cpp      2009-08-11 02:12:24 UTC (rev 43252)<br>
+++ scummvm/branches/gsoc2009-draci/engines/draci/game.cpp      2009-08-11 04:03:22 UTC (rev 43253)<br>@@ -61,21 +63,24 @@<br>
        // Close persons file<br>
        file->close();<br>
<br>
-       // Read in dialog offsets<br>
+       // Read in dialogue offsets<br></blockquote><div><br></div><div>I see that you like British English :-)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">

@@ -227,6 +232,19 @@<br>
        Text *speech = new Text("", _vm->_bigFont, kFontColour1, 0, 0);<br>
        speechAnim->addFrame(speech);<br>
<br>
+       for (uint i = 0; i < kDialogueLines; ++i) {<br>
+               _dialogueAnims[i] = _vm->_anims->addText(-10 - i, true);</blockquote><div><br></div><div>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</div>

<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><br>
+               Text *dialogueLine0 = new Text("", _vm->_smallFont, kLineInactiveColour, 0, 0);<br>
+               _dialogueAnims[i]->addFrame(dialogueLine0);<br>
+<br>
+               _dialogueAnims[i]->setZ(254);<br>
+               _dialogueAnims[i]->setRelative(1,<br>
+                       kScreenHeight - (i + 1) * _vm->_smallFont->getFontHeight());</blockquote><div><br></div><div>maybe you wanna increase the font height by 1 pixel before multiplication, to make sure there is a little gap between the lines</div>

<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">@@ -469,7 +501,193 @@<br>
<br>
        return kObjectNotFound;<br>
 }<br>
+<br>
+void Game::dialogueMenu(int dialogueID) {<br>
<br>
+       int oldLines, hit;<br>
+<br>
+       char tmp[5];<br>
+       sprintf(tmp, "%d", dialogueID+1);</blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
+       Common::String ext(tmp);<br>
+       _dialogueArchive = new BArchive(dialoguePath + ext + ".dfw");<br></blockquote><div><br></div><div>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.</div>

<div> </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">+       debugC(4, kDraciLogicDebugLevel, "Starting dialogue (ID: %d, Archive: %s)",<br>


+               dialogueID, (dialoguePath + ext + ".dfw").c_str());</blockquote><div><br></div><div>especially since here you form the path again.  just reuse the same variable then</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">

+       do {<br>
+               _dialogueExit = false;<br>
+               hit = dialogueDraw();<br>
+<br>
+               debug(2, "Hit: %d, _lines[hit]: %d", hit, _lines[hit]);<br>
+<br>
+               if ((!_dialogueExit) && (hit != -1) && (_lines[hit] != -1)) {</blockquote><div><br></div><div>this condition is quite complicated.  I would rather test for the opposite and break, and then this true-branch need not be indented.</div>

<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">+void Game::runDialogueProg(GPL2Program prog, int offset) {<br>
+<br>
+       // Mark last animation<br>
+       int lastAnimIndex = _vm->_anims->getLastIndex();<br>
+<br>
+       // Run gate program<br>
+       _vm->_script->run(prog, offset);</blockquote><div><br></div><div>fix the comment.  there is not gate here</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">

]+       // Delete all animations loaded after the marked one<br>
+       // (from objects and from the AnimationManager)<br>
+       for (uint i = 0; i < getNumObjects(); ++i) {<br>
+               GameObject *obj = &_objects[i];<br>
+<br>
+               for (uint j = 0; j < obj->_anims.size(); ++j) {<br>
+                       Animation *anim;<br>
+<br>
+                       anim = _vm->_anims->getAnimation(obj->_anims[j]);<br>
+                       if (anim != NULL && anim->getIndex() > lastAnimIndex)<br>
+                               obj->_anims.remove_at(j);<br>
+               }<br>
+       }</blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">

Modified: scummvm/branches/gsoc2009-draci/engines/draci/game.h<br>
===================================================================<br>
--- scummvm/branches/gsoc2009-draci/engines/draci/game.h        2009-08-11 02:12:24 UTC (rev 43252)<br>
+++ scummvm/branches/gsoc2009-draci/engines/draci/game.h        2009-08-11 04:03:22 UTC (rev 43253)<br>
@@ -27,6 +27,7 @@<br>
 #define DRACI_GAME_H<br>
<br>
 #include "common/str.h"<br>
+#include "draci/barchive.h"<br>
 #include "draci/script.h"<br>
 #include "draci/animation.h"<br>
 #include "draci/sprite.h"<br>
@@ -68,6 +69,10 @@<br>
        kDefaultRoomMap = -1<br>
 };<br>
<br>
+enum {<br>
+       kNoDialogue = -1, kDialogueLines = 4<br>
+};</blockquote><div><br></div><div>there is one occurrence of raw constant 4 in dialogueDraw() </div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">

@@ -146,6 +151,14 @@<br>
        byte _fontColour;<br>
 };<br>
<br>
+struct Dialogue {<br>
+       int _canLen;<br>
+       byte *_canBlock;</blockquote><div><br></div><div>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.</div>

<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">@@ -286,6 +304,22 @@<br>
<br>
        int _currentIcon;<br>
<br>
+// HACK: remove public when tested and add getters instead<br>
+public:<br>
+       uint *_dialogueOffsets;<br>
+       int _currentDialogue;<br>
+       int *_dialogueVars;<br>
+       BArchive *_dialogueArchive;<br>
+       Dialogue *_dialogueBlocks;<br>
+       bool _dialogueBegin;<br>
+       bool _dialogueExit;<br>
+       int _currentBlock;<br>
+       int _lastBlock;<br>
+       int _dialogueLines;<br>
+       int _blockNum;<br>
+       int _lines[4];</blockquote><div><br></div><div>suggest to rename _lines to something more specific.  this is a very specific variable, so it's confusing to have such a general name.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">


+       Animation *_dialogueAnims[4];</blockquote><div><br></div><div>here and above, use kDialogueLines</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">

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

<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">+int Script::funcHasBeen(int blockID) {<br>
+       return _vm->_game->_dialogueVars[_vm->_game->_dialogueOffsets[_vm->_game->_currentDialogue] + blockID] > 0;</blockquote><div><br></div><div>ditto</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">

@@ -373,9 +394,12 @@<br>
                return;<br>
        }<br>
<br>
-       int objID = params.pop() - 1;<br>
-       int animID = params.pop() - 1;<br>
+       int objID = params.pop();<br>
+       int animID = params.pop();<br>
<br>
+       objID -= 1;<br>
+       animID -= 1;</blockquote><div><br></div><div>why this?  it doesn't make anything more readable and you still use the simpler (and preferred, by me) form at many other places.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">

+void Script::resetBlock(Common::Queue<int> &params) {<br>
+       int blockID = params.pop();<br>
+<br>
+       _vm->_game->_dialogueVars[_vm->_game->_dialogueOffsets[_vm->_game->_currentDialogue]+blockID] = 0;</blockquote><div><br></div><div>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</div>

<div> </div></div>-- <br>Robert Špalek <<a href="mailto:rspalek@gmail.com">rspalek@gmail.com</a>><br>
</div>