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

[Robert Špalek]

On Tue, Aug 11, 2009 at 12:35 PM, Denis Kasak <denis.kasak at gmail.com> wrote:

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

sure, I'm aware of that.  I just thought it may be useful to have a list of
things to clean.

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

all right, follow the old player then.  I wasn't aware the gap is already a
part of the font.


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


ok, good enough for now, but nice to clean it up a bit.  I think the
original code is pretty horrible at some places and it would a pity to mici
it at all costs.

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.


I was actually thinking similarly that the condition doesn't make much
sense!!  maybe it isn't.  the old contains a few hacks that had been needed
at some point due to crappy libraries and later ceased to be necessary,
however we left them there just to make sure.  there are often comments
saying {this may not be necessary?}.

I don't know what to do to make your life as easy as possible.  what about
importing such fishy code and right away commenting it out in your source
code labeled by some sign so that if things break down you know what kind of
trick MAY fix it?


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

agreed.  just label this e-mail with TODO for later so that once your code
is stable and no correspondence to the old code is needed, you can go
through all suggested renames and clean-ups and perform them.


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

yes, I'm quite confident that you have an error there.  please grep for
"Offset" (there are not too many occurrences) and verify them all at the
same time.

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