[Scummvm-devel] [Scummvm-cvs-logs] SF.net SVN: scummvm:[43487] scummvm/branches/gsoc2009-draci/engines/draci
Robert Špalek
rspalek at gmail.com
Mon Aug 17 23:05:53 CEST 2009
hi Denis,
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.
let me say a few desultory comments at first:
- 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.
- 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.
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.
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.
On Mon, Aug 17, 2009 at 11:50 AM, <dkasak13 at users.sourceforge.net> wrote:
> Revision: 43487
> http://scummvm.svn.sourceforge.net/scummvm/?rev=43487&view=rev
> Author: dkasak13
> Date: 2009-08-17 18:50:38 +0000 (Mon, 17 Aug 2009)
>
> Log Message:
> -----------
> 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".
>
> Modified Paths:
> --------------
> scummvm/branches/gsoc2009-draci/engines/draci/game.cpp
> scummvm/branches/gsoc2009-draci/engines/draci/game.h
> scummvm/branches/gsoc2009-draci/engines/draci/script.cpp
> scummvm/branches/gsoc2009-draci/engines/draci/script.h
>
> Modified: scummvm/branches/gsoc2009-draci/engines/draci/game.cpp
> ===================================================================
> --- scummvm/branches/gsoc2009-draci/engines/draci/game.cpp 2009-08-17
> 18:47:17 UTC (rev 43486)
> +++ scummvm/branches/gsoc2009-draci/engines/draci/game.cpp 2009-08-17
> 18:50:38 UTC (rev 43487)
> @@ -353,52 +361,129 @@
> + // If we are in inventory mode, all the
> animations except game items'
> + // images will necessarily be paused so we
> can safely assume that any
> + // animation under the cursor (a value
> returned by
> + // AnimationManager::getTopAnimationID())
> will be an item animation or.
> + // an overlay, for which we check. Item
> animations have their IDs
> + // calculated by offseting their itemID
> from the ID of the last "special"
> + // animation ID. In this way, we obtain its
> itemID.
please add a comment to the header file ensuring that enough ID's are
allocated around kInventoryItemsID
+ // Otherwise, if we are holding an
> item, try to place it inside the
> + // inventory
> + } else if (_currentItem != kNoItem)
> {
> + // FIXME: This should place
> the item in the nearest inventory slot,
don't worry
> + // If we right-clicked outside the
> inventory, close it
> + if
> (!inventoryAnim->getFrame()->getRect().contains(x, y)) {
> + inventoryDone();
doesn't seem to work for me. maybe it's just caused by the usual
right-click issues on my MacBook
> @@ -440,40 +523,106 @@
> + // If we are in inventory mode, we do a different kind of updating
> that handles
> + // inventory items and return early
> + if (_loopStatus == kStatusInventory && _loopSubstatus ==
> kSubstatusOrdinary) {
> +
> + if (_currentItem == kNoItem) {
> + _vm->_mouse->setCursorType(kNormalCursor);
> + } else {
> + _vm->_mouse->loadItemCursor(_currentItem);
I'd rather not make the 2nd parameter optional and instead explicitly
specify false for not highlighting
> + if (_itemUnderCursor != kNoItem) {
> + const GPL2Program &program =
> _items[_itemUnderCursor]._program;
> + const int canUseOffset =
> _items[_itemUnderCursor]._canUse;
> +
> + if (_vm->_script->testExpression(program,
> canUseOffset)) {
> + if (_currentItem == kNoItem) {
> +
> _vm->_mouse->setCursorType(kHighlightedCursor);
>
this probably cannot happen (if the game is logically correct), but it's
good that you handle it anyway
> + // Find the game object under the cursor
> + // (to be more precise, one that corresponds to the animation under
> the cursor)
> + int curObject = getObjectWithAnimation(_animUnderCursor);
> +
> + // Update the game object under the cursor
> + _objUnderCursor = curObject;
> + if (_objUnderCursor != _oldObjUnderCursor) {
> + _oldObjUnderCursor = _objUnderCursor;
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
@@ -521,6 +674,115 @@
> return kObjectNotFound;
> }
>
> +void Game::removeItem(int itemID) {
> +
> + for (uint i = 0; i < kInventorySlots; ++i) {
> + if (_inventory[i] == itemID) {
> + _inventory[i] = kNoItem;
> + _vm->_anims->stop(kInventoryItemsID - itemID);
> + break;
> + }
> + }
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
+void Game::putItem(int itemID, int position) {
> +
> + if (itemID == kNoItem)
> + return;
could this ever happen? if the game is consistent, raise an assert instead
> + const int line = i / kInventoryColumns + 1;
> + const int column = i % kInventoryColumns + 1;
> +
> + Animation *anim = _vm->_anims->getAnimation(kInventoryItemsID -
> itemID);
> + Drawable *frame = anim->getFrame();
> +
> + const int x = kInventoryX +
> + (column * kInventoryItemWidth) -
> + (kInventoryItemWidth / 2) -
> + (frame->getWidth() / 2);
> +
> + const int y = kInventoryY +
> + (line * kInventoryItemHeight) -
> + (kInventoryItemHeight / 2) -
> + (frame->getHeight() / 2);
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.
> +void Game::inventoryInit() {
> +
> + // Pause all "background" animations
> + _vm->_anims->pauseAnimations();
> +
> + // Draw the inventory and the current items
> + inventoryDraw();
> +
> + // Turn cursor on if it is off
> + _vm->_mouse->cursorOn();
could it ever happen that the inventory is initialized in a location without
a mouse? maybe raise an assert instead.
> +void Game::inventoryDone() {
> + _vm->_mouse->cursorOn();
if the mouse could have been off when entering the inventory above, then you
should restore its previous state rather than leaving it on
> + _vm->_anims->unpauseAnimations();
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
@@ -757,6 +1018,33 @@
> _vm->_anims->play(animID);
> }
>
> +void Game::loadItem(int itemID) {
> +
> + BAFile *f = _vm->_itemsArchive->getFile(itemID * 3);
> + Common::MemoryReadStream itemReader(f->_data, f->_length);
> +
> + GameItem *item = _items + itemID;
> +
> + item->_init = itemReader.readSint16LE();
> + item->_look = itemReader.readSint16LE();
> + item->_use = itemReader.readSint16LE();
> + item->_canUse = itemReader.readSint16LE();
> + item->_imInit = itemReader.readByte();
> + item->_imLook = itemReader.readByte();
> + item->_imUse = itemReader.readByte();
> +
> + f = _vm->_itemsArchive->getFile(itemID * 3 + 1);
> +
> + // The first byte is the length of the string
> + item->_title = Common::String((const char *)f->_data + 1,
> f->_length - 1);
> + assert(f->_data[0] == item->_title.size());
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
+ f = _vm->_itemsArchive->getFile(itemID * 3 + 2);
> +
> + item->_program._bytecode = f->_data;
> + item->_program._length = f->_length;
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
>
> Modified: scummvm/branches/gsoc2009-draci/engines/draci/game.h
> ===================================================================
> --- scummvm/branches/gsoc2009-draci/engines/draci/game.h 2009-08-17
> 18:47:17 UTC (rev 43486)
> +++ scummvm/branches/gsoc2009-draci/engines/draci/game.h 2009-08-17
> 18:50:38 UTC (rev 43487)
> @@ -57,10 +57,10 @@
> kNoEscRoom = -1
> };
>
> -// Used as a value to Game::_currentIcon and means there is no icon (game
> item) selected
> +// Used as a value to Game::_currentIcon and means there is no item
> selected
Icon -> Item, and grep for the remaining occurrences, too
> @@ -82,6 +82,17 @@
> kSpeechTimeUnit = 400
> };
>
> +/** Inventory related magical constants */
> +enum InventoryConstants {
> + kInventoryItemWidth = 25,
> + kInventoryItemHeight = 25,
> + kInventoryColumns = 7,
> + kInventoryLines = 5,
> + kInventoryX = 70, //!< Used for positioning of the inventory sprite on
> the X axis
> + kInventoryY = 30, //!< Used for positioning of the inventory sprite on
> the Y axis
interesting you should need these. isn't the sprite simply centered on the
screen?
@@ -271,8 +283,13 @@
> int getGateNum();
> void setGateNum(int gate);
>
> - int getIconStatus(int iconID);
> - int getCurrentIcon();
> + int getItemStatus(int itemID);
> + void setItemStatus(int itemID, int status);
> + int getCurrentItem();
> + void setCurrentItem(int itemID);
> + void removeItem(int itemID);
> + void putItem(int itemID, int position);
> + void addItem(int itemID);
>
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).
Modified: scummvm/branches/gsoc2009-draci/engines/draci/script.cpp
> ===================================================================
> --- scummvm/branches/gsoc2009-draci/engines/draci/script.cpp 2009-08-17
> 18:47:17 UTC (rev 43486)
> +++ scummvm/branches/gsoc2009-draci/engines/draci/script.cpp 2009-08-17
> 18:50:38 UTC (rev 43487)
> @@ -500,6 +500,53 @@
> _vm->_anims->deleteAfterIndex(markedIndex);
> }
>
> +void Script::icoStat(Common::Queue<int> ¶ms) {
> + int status = params.pop();
> + int itemID = params.pop() - 1;
> +
> + _vm->_game->setItemStatus(itemID, status == 1);
> +
> + if (_vm->_game->getItemStatus(itemID) == 0) {
you could test the value of status directly and also move the code below
into the else branch instead of testing it again
>
> +
> + if (itemID != kNoItem) {
> + _vm->_anims->deleteAnimation(kInventoryItemsID -
> itemID);
> + }
> +
> + _vm->_game->removeItem(itemID);
why isn't this inside the if above? is the behavior on kNoItem even
defined?
> + if (_vm->_mouse->getCursorType() == kNormalCursor) {
> + if (_vm->_game->getLoopStatus() ==
> kStatusInventory) {
> + _vm->_mouse->cursorOff();
what is this here for? why would you wanna turn the mouse off in the
inventory?
> + if (_vm->_game->getItemStatus(itemID) == 1) {
> +
> + if (itemID != kNoItem) {
> + Animation *itemAnim =
> _vm->_anims->addItem(kInventoryItemsID - itemID);
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.
> + BAFile *f = _vm->_itemImagesArchive->getFile(2 *
> itemID);
> + Sprite *sp = new Sprite(f->_data, f->_length, 0, 0,
> true);
> + itemAnim->addFrame(sp);
> + }
> +
> + _vm->_game->setCurrentItem(itemID);
> +
> + _vm->_mouse->loadItemCursor(itemID);
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.
> + // TODO: This is probably not needed but I'm leaving it to
> be sure for now
> + // The original engine needed to turn off the mouse
> temporarily when changing
> + // the cursor image. I'm just setting it to the final state
> of that transition.
> + if (_vm->_game->getLoopStatus() == kStatusInventory) {
> + _vm->_mouse->cursorOn();
> + }
if the engine works without it, I would comment the code out and much later
also completely delete it.
good job,
--
Robert Špalek <rspalek at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.scummvm.org/pipermail/scummvm-devel/attachments/20090817/6dcbae8e/attachment.html>
More information about the Scummvm-devel
mailing list