Title tux walks to item label to pick up item, not to item itself
Priority important Status resolved
Assigned To hail Keywords
Linked issues Watchers hail, matthiaskrgr

Submitted on 2011-09-23 11h48 by matthiaskrgr, last changed by matthiaskrgr.

Author: matthiaskrgr Date: 2011-09-23   11h48
When I have item labels shown and several items are laying next to each other,
tux will walk to the position of the item label on the screen in order to pick
up the item.

This makes it impossible to pick up some items when the item label is far away.
It may even occur that the item label is clickable but not in an "physically"
accessible area.

On level 24, I managed to get the game crashed because tux walked to an item
label which was shown beyond the level (out-of map, crash occured.)

Author: ahuillet Date: 2011-09-23   16h39
I've noticed this several times. We should get it fixed before next release.
Author: ahuillet Date: 2011-10-11   15h12
This is strange - I can't seem to reproduce the bug! I've tried time and again
on level 24 and tux always walks to the right destination. 

Is the report still accurate? If so, can you please provide a detailed recipe to

Author: fluzz Date: 2011-10-18   13h17
I also noticed something strange with item selection. It was easier some months
ago to pick up items. I don't know what changed, but it needs to be investigated.
Author: matthiaskrgr Date: 2011-10-24   10h13
I can still reproduce this.

On level 24, find the Barfs Energy Drink (blue bottle), have labels displayed
and click on its label which is not directly under the item itself.
Now Tux will walk to the label but won't pick up the item.
Author: hail Date: 2011-10-26   17h43
I just want to shed some light on the issue. Taken from 
get_floor_item_index_under_mouse_cursor in items.c:

// In the case that X was pressed, we don't use the item positions but rather
// we use the item slot rectangles from the item texts.

[Further down]

// If no X was pressed, we only use the floor position the mouse
// has pointed to and see if we can find an item that has geographically
// that very same (or a similar enough) position.

This would indicate that the behavior is intentional. However, when testing this 
bug while holding 'x' I've found that clicking on the item's label is not always 
sufficient. I did this testing on r5066.
Author: hail Date: 2011-10-26   17h58
Actually, I misread matthias' original description. Please disregard.
Author: hail Date: 2011-10-26   18h31
After some further testing I found the item pick-up system to be generally very 
robust, and correctly (or at least predictably) handled even corner case 
situations (e.g. overlapping item labels). I did notice one irksome idiosyncrasy 
with the pickup system when item labels are displayed.

If you click the item label, a combo action is set up to move to and pick up the 
item. HOWEVER, if you don't release the mouse button immediately, move actions 
are still added, causing tux to move to the cursors current locations (wherever 
that may be) while still keeping the combo action to pickup the item. If you 
release the button in a somewhat timely manner (off of the label but still 
within ITEM_TAKE_DIST Tux will happily grab the item and you'll not even notice 
the error. However, if you hold the mouse button for slightly longer, the 
movement point gets set ever farther away. If this final point is farther than 
ITEM_TAKE_DIST Tux will stupidly walk over the item (really, check it out) and 
to wherever the target point lies.

In this manner it possible (but not guaranteed) that Tux may not pick up an item 
despite the player's click originating on the label.

As a solution I would suggest locking Tux's combo action to an item's position 
if the (currently held) click originated on that item's label and Z or X is 
active. That way just clicking again will cancel the action, but the clearly 
displayed intent of picking up an item is fulfilled without the current 
Author: ahuillet Date: 2011-10-26   18h31
I don't know if you misunderstood the description - it seems that it might make
Author: hail Date: 2011-10-26   19h35
Implementation of the proposed solution. Pretty significant change, though. Not 
sure if you'd want to put it in:
Author: matthiaskrgr Date: 2011-11-05   21h02
Assigning to hail.
Author: matthiaskrgr Date: 2011-11-06   11h00
Fix committed in r5122, thanks!
Date User Action Args
2011-11-06 11:00:47matthiaskrgrsetstatus: open -> resolved
messages: + msg1915
2011-11-05 21:02:08matthiaskrgrsetassignedto: matthiaskrgr -> hail
messages: + msg1902
nosy: + hail
2011-10-26 19:35:41hailsetmessages: + msg1763
2011-10-26 18:31:03ahuilletsetmessages: + msg1762
2011-10-26 18:31:03hailsetmessages: + msg1761
2011-10-26 17:58:27hailsetmessages: + msg1760
2011-10-26 17:43:00hailsetmessages: + msg1759
2011-10-24 10:13:18matthiaskrgrsetmessages: + msg1734
2011-10-18 13:17:18fluzzsetmessages: + msg1718
2011-10-11 15:12:34ahuilletsetassignedto: matthiaskrgr
messages: + msg1717
nosy: + matthiaskrgr
2011-09-23 16:44:39matthiaskrgrsettitle: tux walkt to item label to pick up item, not to item itself -> tux walks to item label to pick up item, not to item itself
2011-09-23 16:39:01ahuilletsetpriority: bug -> important
messages: + msg1655
2011-09-23 11:48:44matthiaskrgrcreate