hi Denis,<br><br><div>awesome! I tried to pick the apply, bring it to the hedgehog, leave the location and come back, and then pick the trapped hedgehog, and it worked! superb.</div><div><br></div><div>let me say a few desultory comments at first:</div>
<div>- the interactivity of the inventory could be improved. according to the code, right click outside exits it, but it seems not to work for me. also, I cannot open the inventory by going up with the mouse, which was the original behavior. you may decide to not support it though as it may be confusing.</div>
<div><br></div><div>- a possible HUGE future change: when I'm reading the loop handlers, I start thinking whether and how much easier it would be to implement callbacks on objects rather than a big set of nested ifs in the loop handler. for example, each animation object would have optional callbacks onMouseEnter, onLeftClick, onRightClick, etc. then you would in the main loop retrieve a pointer to the animation object under the cursor, test whether it has a registered callback for the kind of event that is currently going on, and call its callback if yes. for example, a registered animation would in onMouseEnter test whether the current item can be used and possibly change the mouse cursor, and update the title. an overlay would in onMouseEnter delete the current title. an animation object corresponding to a displayed item in an opened inventory would do its actions. etc. etc., you get the point.</div>
<div><br></div><div>I'm aware that this would be a huge and initially confusing change, but in the long term this would be The Right Thing To Do (TM). you have a nice object model (unlike the old game player) and object-oriented user interfaces are best programmed using event handlers rather than having an infinitely complex main loop.</div>
<div><br></div><div>please don't worry about this now and keep it in your mind as a possible final clean-up once everything is in place. I know that doing so now would confuse everything a lot during the import of the code from the old player.</div>
<div><br></div><div><br><div class="gmail_quote">On Mon, Aug 17, 2009 at 11:50 AM, <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="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
Revision: 43487<br>
<a href="http://scummvm.svn.sourceforge.net/scummvm/?rev=43487&view=rev" target="_blank">http://scummvm.svn.sourceforge.net/scummvm/?rev=43487&view=rev</a><br>
Author: dkasak13<br>
Date: 2009-08-17 18:50:38 +0000 (Mon, 17 Aug 2009)<br>
<br>
Log Message:<br>
-----------<br>
Added inventory and item handling support (monster patch, sorry). Items were previously called "icons" as in the original player. This commit also renamed every such instance to the proper "item".<br>
<br>
Modified Paths:<br>
--------------<br>
scummvm/branches/gsoc2009-draci/engines/draci/game.cpp<br>
scummvm/branches/gsoc2009-draci/engines/draci/game.h<br>
scummvm/branches/gsoc2009-draci/engines/draci/script.cpp<br>
scummvm/branches/gsoc2009-draci/engines/draci/script.h<br>
<br>
Modified: scummvm/branches/gsoc2009-draci/engines/draci/game.cpp<br>
===================================================================<br>
--- scummvm/branches/gsoc2009-draci/engines/draci/game.cpp 2009-08-17 18:47:17 UTC (rev 43486)<br>
+++ scummvm/branches/gsoc2009-draci/engines/draci/game.cpp 2009-08-17 18:50:38 UTC (rev 43487)<br>@@ -353,52 +361,129 @@<br>+ // If we are in inventory mode, all the animations except game items'<br>
+ // images will necessarily be paused so we can safely assume that any<br>
+ // animation under the cursor (a value returned by<br>
+ // AnimationManager::getTopAnimationID()) will be an item animation or.<br>
+ // an overlay, for which we check. Item animations have their IDs<br>
+ // calculated by offseting their itemID from the ID of the last "special"<br>
+ // animation ID. In this way, we obtain its itemID.</blockquote><div><br></div><div>please add a comment to the header file ensuring that enough ID's are allocated around kInventoryItemsID</div>
<div><br></div><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">+ // Otherwise, if we are holding an item, try to place it inside the<br>
+ // inventory<br>
+ } else if (_currentItem != kNoItem) {<br>
+ // FIXME: This should place the item in the nearest inventory slot,</blockquote><div><br>don't worry<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
+ // If we right-clicked outside the inventory, close it<br>
+ if (!inventoryAnim->getFrame()->getRect().contains(x, y)) {<br>
+ inventoryDone();</blockquote><div><br>doesn't seem to work for me. maybe it's just caused by the usual right-click issues on my MacBook<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
@@ -440,40 +523,106 @@<br>
+ // If we are in inventory mode, we do a different kind of updating that handles<br>
+ // inventory items and return early<br>
+ if (_loopStatus == kStatusInventory && _loopSubstatus == kSubstatusOrdinary) {<br>
+<br>
+ if (_currentItem == kNoItem) {<br>
+ _vm->_mouse->setCursorType(kNormalCursor);<br>
+ } else {<br>
+ _vm->_mouse->loadItemCursor(_currentItem);</blockquote><div><br>I'd rather not make the 2nd parameter optional and instead explicitly specify false for not highlighting<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
+ if (_itemUnderCursor != kNoItem) {<br>
+ const GPL2Program &program = _items[_itemUnderCursor]._program;<br>
+ const int canUseOffset = _items[_itemUnderCursor]._canUse;<br>
+<br>
+ if (_vm->_script->testExpression(program, canUseOffset)) {<br>
+ if (_currentItem == kNoItem) {<br>
+ _vm->_mouse->setCursorType(kHighlightedCursor);<br>
</blockquote><div><br>this probably cannot happen (if the game is logically correct), but it's good that you handle it anyway<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
+ // Find the game object under the cursor<br>
+ // (to be more precise, one that corresponds to the animation under the cursor)<br>
+ int curObject = getObjectWithAnimation(_animUnderCursor);<br>
+<br>
+ // Update the game object under the cursor<br>
+ _objUnderCursor = curObject;<br>
+ if (_objUnderCursor != _oldObjUnderCursor) {<br>
+ _oldObjUnderCursor = _objUnderCursor;</blockquote><div><br>is this correct? do you use _oldObjUnderCursor anywhere else? because if you don't, then you also don't need to track it, since you just update its value, but don't call any special action on its old value<br>
<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">@@ -521,6 +674,115 @@<br>
return kObjectNotFound;<br>
}<br>
<br>
+void Game::removeItem(int itemID) {<br>
+<br>
+ for (uint i = 0; i < kInventorySlots; ++i) {<br>
+ if (_inventory[i] == itemID) {<br>
+ _inventory[i] = kNoItem;<br>
+ _vm->_anims->stop(kInventoryItemsID - itemID);<br>
+ break;<br>
+ }<br>
+ }</blockquote><div><br>maybe you should raise an assert if the object has not been found. if the game is inconsistent and relies on that this method is error-resilient, then leave it as is<br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
+void Game::putItem(int itemID, int position) {<br>
+<br>
+ if (itemID == kNoItem)<br>
+ return;</blockquote><div><br>could this ever happen? if the game is consistent, raise an assert instead<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
+ const int line = i / kInventoryColumns + 1;<br>
+ const int column = i % kInventoryColumns + 1;</blockquote><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>
+<br>
+ Animation *anim = _vm->_anims->getAnimation(kInventoryItemsID - itemID);<br>
+ Drawable *frame = anim->getFrame();<br>
+<br>
+ const int x = kInventoryX +<br>
+ (column * kInventoryItemWidth) -<br>
+ (kInventoryItemWidth / 2) -<br>
+ (frame->getWidth() / 2);<br>
+<br>
+ const int y = kInventoryY +<br>
+ (line * kInventoryItemHeight) -<br>
+ (kInventoryItemHeight / 2) -<br>
+ (frame->getHeight() / 2);</blockquote><div><div><br>
why do you add +1 to line and column if you then subtract it in the expressions for x and
y? rather remove it from both and leave line and column 0-indexed.<br>
<br>
</div> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">+void Game::inventoryInit() {<br>
+<br>
+ // Pause all "background" animations<br>
+ _vm->_anims->pauseAnimations();<br>
+<br>
+ // Draw the inventory and the current items<br>
+ inventoryDraw();<br>
+<br>
+ // Turn cursor on if it is off<br>
+ _vm->_mouse->cursorOn();</blockquote><div><br>could it ever happen that the inventory is initialized in a location without a mouse? maybe raise an assert instead.<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
+void Game::inventoryDone() {<br>
+ _vm->_mouse->cursorOn();</blockquote><div><br>if the mouse could have been off when entering the inventory above, then you should restore its previous state rather than leaving it on<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
+ _vm->_anims->unpauseAnimations();</blockquote><div><br>could it happen that some animation was loaded into memory, but stopped (e.g., by a GPL program)? if yes, then you will start playing it here instead of restoring the previous state of all animations<br>
<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">@@ -757,6 +1018,33 @@<br>
_vm->_anims->play(animID);<br>
}<br>
<br>
+void Game::loadItem(int itemID) {<br>
+<br>
+ BAFile *f = _vm->_itemsArchive->getFile(itemID * 3);<br>
+ Common::MemoryReadStream itemReader(f->_data, f->_length);<br>
+<br>
+ GameItem *item = _items + itemID;<br>
+<br>
+ item->_init = itemReader.readSint16LE();<br>
+ item->_look = itemReader.readSint16LE();<br>
+ item->_use = itemReader.readSint16LE();<br>
+ item->_canUse = itemReader.readSint16LE();<br>
+ item->_imInit = itemReader.readByte();<br>
+ item->_imLook = itemReader.readByte();<br>
+ item->_imUse = itemReader.readByte();<br>
+<br>
+ f = _vm->_itemsArchive->getFile(itemID * 3 + 1);<br>
+<br>
+ // The first byte is the length of the string<br>
+ item->_title = Common::String((const char *)f->_data + 1, f->_length - 1); <br>
+ assert(f->_data[0] == item->_title.size());</blockquote><div><br>since you repeat these two lines so often, what about deriving a sub-class of string with an additional constructor taking BAFile *f? since it's a sub-class, you can use it anywhere where a Common::String is expected, you could pass it f directly, and you would get consistent error checking everywhere for free<br>
<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">+ f = _vm->_itemsArchive->getFile(itemID * 3 + 2);<br>
+<br>
+ item->_program._bytecode = f->_data;<br>
+ item->_program._length = f->_length;</blockquote><div><br>ditto here. the GPLProgram could have a constructor taking BAFile *f as a parameter. this one is trivial though, so you wouldn't save anything substantial, except for that it would be nicer that the callers need not be aware of how a GPLProgram is loaded<br>
<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>
Modified: scummvm/branches/gsoc2009-draci/engines/draci/game.h<br>
===================================================================<br>
--- scummvm/branches/gsoc2009-draci/engines/draci/game.h 2009-08-17 18:47:17 UTC (rev 43486)<br>
+++ scummvm/branches/gsoc2009-draci/engines/draci/game.h 2009-08-17 18:50:38 UTC (rev 43487)<br>
@@ -57,10 +57,10 @@<br>
kNoEscRoom = -1<br>
};<br>
<br>
-// Used as a value to Game::_currentIcon and means there is no icon (game item) selected<br>
+// Used as a value to Game::_currentIcon and means there is no item selected</blockquote><div><br>Icon -> Item, and grep for the remaining occurrences, too<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
@@ -82,6 +82,17 @@<br>
kSpeechTimeUnit = 400<br>
};<br>
<br>
+/** Inventory related magical constants */<br>
+enum InventoryConstants {<br>
+ kInventoryItemWidth = 25,<br>
+ kInventoryItemHeight = 25,<br>
+ kInventoryColumns = 7,<br>
+ kInventoryLines = 5,<br>
+ kInventoryX = 70, //!< Used for positioning of the inventory sprite on the X axis<br>
+ kInventoryY = 30, //!< Used for positioning of the inventory sprite on the Y axis</blockquote><div><br>interesting you should need these. isn't the sprite simply centered on the screen?<br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
@@ -271,8 +283,13 @@<br>
int getGateNum();<br>
void setGateNum(int gate);<br>
<br>
- int getIconStatus(int iconID);<br>
- int getCurrentIcon();<br>
+ int getItemStatus(int itemID);<br>
+ void setItemStatus(int itemID, int status);<br>
+ int getCurrentItem();<br>
+ void setCurrentItem(int itemID);<br>
+ void removeItem(int itemID);<br>
+ void putItem(int itemID, int position);<br>
+ void addItem(int itemID);<br>
</blockquote><div><br>please label all getters as const, and also in the remaining classes in the whole project. it's nice, because the compiler checks for you that they really don't modify the state of the object, and you will be able to call the getter on a const pointer, should you use it somewhere (which would be nice for the same reasons).<br>
<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">Modified: scummvm/branches/gsoc2009-draci/engines/draci/script.cpp<br>
===================================================================<br>
--- scummvm/branches/gsoc2009-draci/engines/draci/script.cpp 2009-08-17 18:47:17 UTC (rev 43486)<br>
+++ scummvm/branches/gsoc2009-draci/engines/draci/script.cpp 2009-08-17 18:50:38 UTC (rev 43487)<br>
@@ -500,6 +500,53 @@<br>
_vm->_anims->deleteAfterIndex(markedIndex);<br>
}<br>
<br>
+void Script::icoStat(Common::Queue<int> ¶ms) {<br>
+ int status = params.pop();<br>
+ int itemID = params.pop() - 1;<br>
+<br>
+ _vm->_game->setItemStatus(itemID, status == 1);<br>
+<br>
+ if (_vm->_game->getItemStatus(itemID) == 0) {</blockquote><div><br>you could test the value of status directly and also move the code below into the else branch instead of testing it again<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
+<br>
+ if (itemID != kNoItem) {<br>
+ _vm->_anims->deleteAnimation(kInventoryItemsID - itemID);<br>
+ }<br>
+<br>
+ _vm->_game->removeItem(itemID);</blockquote><div><br>why isn't this inside the if above? is the behavior on kNoItem even defined?<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
+ if (_vm->_mouse->getCursorType() == kNormalCursor) {<br>
+ if (_vm->_game->getLoopStatus() == kStatusInventory) {<br>
+ _vm->_mouse->cursorOff();</blockquote><div><br>what is this here for? why would you wanna turn the mouse off in the inventory?<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
+ if (_vm->_game->getItemStatus(itemID) == 1) {<br>
+<br>
+ if (itemID != kNoItem) {<br>
+ Animation *itemAnim = _vm->_anims->addItem(kInventoryItemsID - itemID);</blockquote><div><br>I'm a bit confused. doesn't the animation ID correspond to the position in the inventory grid instead of to the ID of the underlying game item? if yes, then you should search for the first available position here.<br>
<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>
+ BAFile *f = _vm->_itemImagesArchive->getFile(2 * itemID);<br>
+ Sprite *sp = new Sprite(f->_data, f->_length, 0, 0, true);<br>
+ itemAnim->addFrame(sp);<br>
+ }<br>
+<br>
+ _vm->_game->setCurrentItem(itemID);<br>
+<br>
+ _vm->_mouse->loadItemCursor(itemID);</blockquote><div><br>oh, so based on a call from a GPL script, the dragon gets a game item into his hands, and then dropping it into the inventory is done by the user by a left mouse click? neat! I didn't remember how things were done and I thought it goes directly there. that would be strange, because the player wouldn't be properly notified that he got a new game item.<br>
<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">+ // TODO: This is probably not needed but I'm leaving it to be sure for now<br>
+ // The original engine needed to turn off the mouse temporarily when changing<br>
+ // the cursor image. I'm just setting it to the final state of that transition.<br>
+ if (_vm->_game->getLoopStatus() == kStatusInventory) {<br>
+ _vm->_mouse->cursorOn();<br>
+ }</blockquote><div><br>if the engine works without it, I would comment the code out and much later also completely delete it.<br> <br></div>good job,<br></div>-- <br>Robert Špalek <<a href="mailto:rspalek@gmail.com" target="_blank">rspalek@gmail.com</a>><br>
</div>