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

[Robert Špalek]

hi Denis,
this is an absolutely great commit!  really a lot of cool work in just one
commit :-)  I'm also excited with how nicely it work during the game-play.

On Sat, Aug 8, 2009 at 9:09 PM, <dkasak13 at users.sourceforge.net> wrote:

> * Added Game::updateCursor().


I'm a bit suspicious with its code, because I don't understand the logic
with changing the shape of the cursor and highlighting it.  it works so far,
but on the other hand, I only see the cross and arrow cursors, and the
highlighted variant, but no game item cursors, because they are not handled
yet.  when you implement the game items, I will see how the cursors work.

first, I would make the names setCursorType() and loadItemCursor() more
similar, because they do the same things but with a different source object.
 what about loadMouseCursor() instead of setCursorType() ?  also, I would
probably not make the second parameter of loadItemCursor() optional, but let
the caller always explicitly specify it.  not too much work, and makes the
code cleaner, I believe.

second, loadItemCursor() doesn't multiply the number of the object by two.
 this is needed, because the BArchive stores cursors as [item1, highlighted
item1, item2, ...].

the rest of the comments inlined there

* Restructured the main loop to fix many subtle bugs and enable some new
> functionality concerning object scripts (like support for room-global use
> scripts).


really great job!


> Modified: scummvm/branches/gsoc2009-draci/engines/draci/game.cpp
> ===================================================================
> --- scummvm/branches/gsoc2009-draci/engines/draci/game.cpp      2009-08-09
> 03:59:39 UTC (rev 43159)
> +++ scummvm/branches/gsoc2009-draci/engines/draci/game.cpp      2009-08-09
> 04:09:24 UTC (rev 43160)
> +void Game::updateCursor() {
> +
> +       _vm->_mouse->setCursorType(kNormalCursor);
> +
> +       if (_currentIcon != kNoIcon) {
> +               _vm->_mouse->loadItemCursor(_currentIcon);
> +       }
> +
> +       if (_objUnderCursor == kObjectNotFound) {
> +
> +               if (_vm->_script->testExpression(_currentRoom._program,
> _currentRoom._canUse)) {
> +                       if (_currentIcon == kNoIcon) {
> +
> _vm->_mouse->setCursorType(kHighlightedCursor);
> +                       } else {
> +                               _vm->_mouse->loadItemCursor(_currentIcon,
> true);
> +                       }
> +               }
> +       } else {
> +               GameObject *obj = &_objects[_objUnderCursor];
> +
> +               _vm->_mouse->setCursorType((CursorType)obj->_walkDir);


I would move this to the else branch, otherwise you change the cursor twice
in a row.  this doesn't matter much for the speed, but it decreases the
readability of the code.  it would be ideal if you first performed all kinds
of ifs and then set the mouse cursor exactly once, instead of setting and
resetting it.


> +
> +               if (!(obj->_walkDir > 0)) {


please test if (obj->_walkDir == 0) instead of this negation


>
> +                       if (_vm->_script->testExpression(obj->_program,
> obj->_canUse)) {
> +                               if (_currentIcon == kNoIcon) {
> +
> _vm->_mouse->setCursorType(kHighlightedCursor);
> +                               } else {
> +
> _vm->_mouse->loadItemCursor(_currentIcon, true);
> +                               }
> +                       }
> +               }
> +       }


I would also maybe unify both branches of the higher if, by storing the
beginning of the GPL program to be run into a local variable, and also
_walkDir into another one. the logic is the same otherwise, so why to
duplicate it.

also, I'm not sure whether it's correct to display an arrow if you have a
game item in your hand.  what does the click in the original player do when
the mouse is on an exit game from the location?  if it first drops the game
item there and only exits the room at the next click, then the mouse cursor
should by left to be the game item and not the arrow.

 int Game::getObjectWithAnimation(int animID) {
>        for (uint i = 0; i < _info._numObjects; ++i) {
>                GameObject *obj = &_objects[i];
> @@ -398,11 +463,14 @@
>  void Game::walkHero(int x, int y) {
>
>        Surface *surface = _vm->_screen->getSurface();
> +
>        Common::Point p = _currentRoom._walkingMap.findNearestWalkable(x, y,
> surface->getRect());
>
>        x = p.x;
>        y = p.y;
>
> +       debugC(4, kDraciLogicDebugLevel, "Walk to x: %d y: %d", x, y);
> +
>        // Fetch dragon's animation ID
>        // FIXME: Need to add proper walking (this only warps the dragon to
> position)
>        int animID = getObject(kDragonObject)->_anims[0];
> @@ -426,19 +494,18 @@
>        _persons[kDragonObject]._x = x + (lround(scaleX) * width) / 2;
>        _persons[kDragonObject]._y = y - lround(scaleY) * height;
>
> +       // Set the per-animation scaling factor
> +       anim->setScaleFactors(scaleX, scaleY);
> +
>        // We naturally want the dragon to position its feet to the location
> of the
>        // click but sprites are drawn from their top-left corner so we
> subtract
>        // the current height of the dragon's sprite
>        y -= (int)(scaleY * height);
> +       //x -= (int)(scaleX * width) / 2;
>

why is this commented out?  the dragon should have his feet centered at the
click's location, which it currently does not.


>  Common::Point WalkingMap::findNearestWalkable(int startX, int startY,
> Common::Rect searchRect) {
>
> +       // If the starting point is walkable, just return that
> +       if (searchRect.contains(startX, startY) && isWalkable(startX,
> startY)) {
> +               return Common::Point(startX, startY);
> +       }
> +
> +
>        int signs[] = { 1, -1 };
>        const uint kSignsNum = 2;
>
> @@ -945,6 +1026,13 @@
>                                }
>                        }
>
> +                       if (x == y) {
> +                               // If the starting point is walkable, just
> return that
> +                               if (searchRect.contains(finalX, finalY) &&
> isWalkable(finalX, finalY)) {
> +                                       return Common::Point(finalX,
> finalY);
> +                               }
> +                       }


what is the purpose of testing the 45-degree point of an ellipse?  I believe
even the original loop would work well and that there is no need for this
optimization.  however, what really confuses me is the comment about the
starting point and testing the starting point here.  isn't the first test in
this function enough?

best,
-- 
Robert Špalek <rspalek at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.scummvm.org/pipermail/scummvm-git-logs/attachments/20090809/0f619b3f/attachment.html>