sorry, I forgot to change the Cc: address again<br><br><div class="gmail_quote">---------- Forwarded message ----------<br>From: <b class="gmail_sendername">Robert Špalek</b> <span dir="ltr"><<a href="mailto:rspalek@gmail.com">rspalek@gmail.com</a>></span><br>
Date: Sun, Aug 9, 2009 at 12:32 PM<br>Subject: Re: [Scummvm-cvs-logs] SF.net SVN: scummvm:[43160] scummvm/branches/gsoc2009-draci/engines/draci<br>To: <a href="mailto:dkasak13@users.sourceforge.net">dkasak13@users.sourceforge.net</a><br>
Cc: <a href="mailto:scummvm-cvs-logs@lists.sourceforge.net">scummvm-cvs-logs@lists.sourceforge.net</a><br><br><br>hi Denis,<div><br></div><div>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.<br>
<br><div class="gmail_quote">On Sat, Aug 8, 2009 at 9:09 PM, <span dir="ltr"><<a href="mailto:dkasak13@users.sourceforge.net" target="_blank">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">* Added Game::updateCursor().</blockquote><div><br></div><div>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.</div>
<div><br></div><div>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.</div>
<div><br></div><div>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, ...].</div><div><br></div><div>
the rest of the comments inlined there</div><div class="im">
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">* Restructured the main loop to fix many subtle bugs and enable some new functionality concerning object scripts (like support for room-global use scripts).</blockquote>
<div><br></div></div><div>really great job!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">Modified: scummvm/branches/gsoc2009-draci/engines/draci/game.cpp<br>
===================================================================<br>
--- scummvm/branches/gsoc2009-draci/engines/draci/game.cpp 2009-08-09 03:59:39 UTC (rev 43159)<br>
+++ scummvm/branches/gsoc2009-draci/engines/draci/game.cpp 2009-08-09 04:09:24 UTC (rev 43160)<br></div><div class="im">+void Game::updateCursor() {<br>
+<br>
+ _vm->_mouse->setCursorType(kNormalCursor);<br>
+<br>
+ if (_currentIcon != kNoIcon) {<br>
+ _vm->_mouse->loadItemCursor(_currentIcon);<br>
+ }<br>
+<br>
+ if (_objUnderCursor == kObjectNotFound) {<br>
+<br>
+ if (_vm->_script->testExpression(_currentRoom._program, _currentRoom._canUse)) {<br>
+ if (_currentIcon == kNoIcon) {<br>
+ _vm->_mouse->setCursorType(kHighlightedCursor);<br>
+ } else {<br>
+ _vm->_mouse->loadItemCursor(_currentIcon, true);<br>
+ }<br>
+ }<br>
+ } else {<br>
+ GameObject *obj = &_objects[_objUnderCursor];<br>
+<br>
+ _vm->_mouse->setCursorType((CursorType)obj->_walkDir);</div></blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">+<br>
+ if (!(obj->_walkDir > 0)) {</blockquote><div><br></div><div>please test if (obj->_walkDir == 0) instead of this negation</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
+ if (_vm->_script->testExpression(obj->_program, obj->_canUse)) {<div class="im"><br>
+ if (_currentIcon == kNoIcon) {<br>
+ _vm->_mouse->setCursorType(kHighlightedCursor);<br>
+ } else {<br>
+ _vm->_mouse->loadItemCursor(_currentIcon, true);<br>
+ }<br>
+ }<br>
+ }<br>
+ }</div></blockquote><div><br></div><div>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.</div>
<div><br></div><div>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.</div>
<div class="im">
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> int Game::getObjectWithAnimation(int animID) {<br>
for (uint i = 0; i < _info._numObjects; ++i) {<br>
GameObject *obj = &_objects[i];<br>
@@ -398,11 +463,14 @@<br>
void Game::walkHero(int x, int y) {<br>
<br>
Surface *surface = _vm->_screen->getSurface();<br>
+<br>
Common::Point p = _currentRoom._walkingMap.findNearestWalkable(x, y, surface->getRect());<br>
<br>
x = p.x;<br>
y = p.y;<br>
<br>
+ debugC(4, kDraciLogicDebugLevel, "Walk to x: %d y: %d", x, y);<br>
+<br>
// Fetch dragon's animation ID<br>
// FIXME: Need to add proper walking (this only warps the dragon to position)<br>
int animID = getObject(kDragonObject)->_anims[0];<br>
@@ -426,19 +494,18 @@<br>
_persons[kDragonObject]._x = x + (lround(scaleX) * width) / 2;<br>
_persons[kDragonObject]._y = y - lround(scaleY) * height;<br>
<br>
+ // Set the per-animation scaling factor<br>
+ anim->setScaleFactors(scaleX, scaleY);<br>
+<br>
// We naturally want the dragon to position its feet to the location of the<br>
// click but sprites are drawn from their top-left corner so we subtract<br>
// the current height of the dragon's sprite<br>
y -= (int)(scaleY * height);<br>
+ //x -= (int)(scaleX * width) / 2;<br>
</blockquote><div><br></div></div><div>why is this commented out? the dragon should have his feet centered at the click's location, which it currently does not.</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Common::Point WalkingMap::findNearestWalkable(int startX, int startY, Common::Rect searchRect) {<br>
<br>
+ // If the starting point is walkable, just return that<br>
+ if (searchRect.contains(startX, startY) && isWalkable(startX, startY)) {<br>
+ return Common::Point(startX, startY);<br>
+ }<br>
+<br>
+<br>
int signs[] = { 1, -1 };<br>
const uint kSignsNum = 2;<br>
<br>
@@ -945,6 +1026,13 @@<br>
}<br>
}<br>
<br>
+ if (x == y) {<br>
+ // If the starting point is walkable, just return that<br>
+ if (searchRect.contains(finalX, finalY) && isWalkable(finalX, finalY)) {<br>
+ return Common::Point(finalX, finalY);<br>
+ }<br>
+ }</blockquote><div><br></div></div><div>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?</div>
<div><br></div><div>best,</div></div>-- <br>Robert Špalek <<a href="mailto:rspalek@gmail.com" target="_blank">rspalek@gmail.com</a>><br>
</div>
</div><br><br clear="all"><br>-- <br>Robert Špalek <<a href="mailto:rspalek@gmail.com">rspalek@gmail.com</a>><br>