[Scummvm-devel] [Scummvm-cvs-logs] SF.net SVN: scummvm:[43253] scummvm/branches/gsoc2009-draci/engines/draci
Robert Špalek
rspalek at gmail.com
Tue Aug 11 07:52:02 CEST 2009
hi Denis!
this is a very impressive commit! I'm really excited as it pushes the game
much further!
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 :-)
> @@ -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
>
> + 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
@@ -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.
> + 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.
+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
> ]+ // 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.
> 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()
@@ -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.
@@ -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.
> + Animation *_dialogueAnims[4];
here and above, use kDialogueLines
> 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 ?
> +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.
> +void Script::resetBlock(Common::Queue<int> ¶ms) {
> + 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
--
Robert Špalek <rspalek at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.scummvm.org/pipermail/scummvm-devel/attachments/20090810/ff5d9086/attachment.html>
More information about the Scummvm-devel
mailing list