View Issue Details

IDProjectCategoryView StatusLast Update
0002993The Dark ModDesign/Codingpublic03.12.2020 19:05
Reporterangua Assigned ToSTiFU  
PrioritynormalSeveritynormalReproducibilityhave not tried
Status resolvedResolutionfixed 
Product VersionTDM 1.07 
Target VersionTDM 2.08Fixed in VersionTDM 2.08 
Summary0002993: Removing an inventory item can invalidate cursor, resulting in crash
DescriptionIn Inventory.cpp, the CInventory::RemoveItem() function advances the cursor to the next before removing the item. If the removed item is the second last one, the cursor moves to the last one, and then the numbers of elements in the category is reduced by one, leaving the cursor at an invalid position.
Steps To ReproduceMake a custom category with at least 2 items, remove the second last item
(use on object in world, with script function
$player1.replaceInvItem(item, $null_entity);)
Try removing the other item -> crash
TagsNo tags attached.

Relationships

related to 0004987 feedback Loot icon and item icons overlapping 
related to 0005038 resolvedstgatilov 'M' key (cycle maps) not behaving 
related to 0005302 resolvedstgatilov House of Locked Secrets: crash because inventory cursor points to empty category 

Activities

grayman

grayman

05.05.2012 15:29

viewer   ~0004539

As we attempt to wrap up 1.08, is this fix required, or can it wait?
tels

tels

05.05.2012 17:03

reporter   ~0004543

It is not fixed, and since its a crash, it should probably be fixed. A cursory look didn't show me an easy way to fix it, tho. Once the cursor runs over, it somehow needs to be set back to a valid item.
tels

tels

05.05.2012 18:06

reporter   ~0004544

And I forgot to mention that the crash can occur because the player drops an item from the inventory to the hands or ground, too.
grayman

grayman

05.05.2012 18:18

viewer   ~0004546

I picked up two flashmines, then dropped one after the other on the ground, with no crash.

Guess I have to find out what's going on.
grayman

grayman

05.05.2012 18:19

viewer   ~0004547

And how do I "make a custom category"?
grayman

grayman

05.05.2012 21:46

viewer   ~0004548

This note is for angua, because she filed the issue.

I'm unable to reproduce a crash, and want to know if I'm creating a reasonable test case.

I have a flashmine (Mine1) and a mine (Mine2), both of which belong to the Tools category.

I have two buttons. Button 1 calls a script that does this:

$player1.replaceInvItem($Mine1, $null_entity);
$player1.replaceInvItem($Mine2, $null_entity);

Button 2 calls a script that does this:

$player1.replaceInvItem($Mine2, $null_entity);
$player1.replaceInvItem($Mine1, $null_entity);

In the game, I pick up Mine1, then Mine2.

In the inventory cursor list, Mine2 becomes the first entry, and Mine1 is the second entry.

Regardless of which button I press, I can't get a crash. I assume Button 2 is the one that creates the problem you describe, since Mine2--the second item from the end--is removed first, followed by Mine1.

Have I set this up correctly?

(There is a display problem where the screen isn't being updated properly depending on what's being displayed, but that's a separate issue.)
angua

angua

10.05.2012 18:19

manager   ~0004563

Can't reproduce the crash either, looks like this is not an issue any more.
grayman

grayman

10.05.2012 18:57

viewer   ~0004566

May I close it,then?

Thanks.
tels

tels

11.05.2012 17:40

reporter   ~0004573

I am still not convinced that this bug is really fixed.


Consider what this code does:

void CInventory::RemoveItem(const CInventoryItemPtr& item)
{
        if (item == NULL) return;

        // Update the cursors first
        for (int i = 0; i < m_Cursor.Num(); i++)
        {
                if (m_Cursor[i]->GetCurrentItem() == item)
                {
                        // Advance the cursor, this should be enough
                        m_Cursor[i]->GetNextItem();
                }
        }

        // Now remove the item, the cursors are updated.
        item->Category()->RemoveItem(item);
}


If you have three items in a category, 0, 1, and 2, and the cursor is at item 1 (the second-to-last-one), then

m_Cursor[i]->GetNextItem();

will fetch the ptr to the last item (2) and set the item index for the cursor to "2".

However, when you then remove the second-to-last-item, the ptr to the item inside the cursor is still valid (points to 2), but the itemIndex is invalid, it is still "2", but there are only two items now (0 and 1).

The code item->Category()->RemoveItem(item); does not seem to update the cursor anymore.

E.g. the code seems to be backwards, it first updates the cursor, then removes the item, leaving and invalid cursor.

That it no more crashes is probably pure luck.
CodeMonkey

CodeMonkey

07.10.2012 13:07

reporter   ~0004869

Wouldn't it be better to select the previous item?

This 'could' lead to a case where it tries to select an invalid index, ie, -1, but, that shouldn't be hard to account for.
VanishedOne

VanishedOne

09.02.2019 20:11

reporter   ~0011561

Possibly relevant: when I made the this-message-will-self-destruct-after-you-read-it note in ITB I started out just dropping it in the map with "inv_map_start" "1", and removing it with $player1.replaceInvItem($inv_item_to_remove, $null_entity) caused the item cursor to change to empty. However, after I made a corresponding shop item and added it to the starting inventory via the shop, the inventory cursor would stick around until I cycled to another item. I ended up adding $player1.getPrevInvItem(); to the script.
STiFU

STiFU

14.02.2019 10:37

developer   ~0011593

The GetCurrentItem()-method family do a validity check of the index and return NULL-ptr for invalid indices. The callers do not consistently check for nullptr and I didn't want to mess with the whole codebase, so I have ensured the index is always valid. As a fallback, the TDM_DUMMY_ITEM is selected in error cases, which always exists. This is the same as clearing the inventory ingame.

Rev. 7957

Issue History

Date Modified Username Field Change
24.01.2012 16:24 angua New Issue
05.05.2012 15:29 grayman Note Added: 0004539
05.05.2012 17:03 tels Note Added: 0004543
05.05.2012 18:06 tels Note Added: 0004544
05.05.2012 18:18 grayman Note Added: 0004546
05.05.2012 18:19 grayman Note Added: 0004547
05.05.2012 21:46 grayman Note Added: 0004548
05.05.2012 22:15 grayman Assigned To => grayman
05.05.2012 22:15 grayman Status new => assigned
10.05.2012 18:19 angua Note Added: 0004563
10.05.2012 18:57 grayman Note Added: 0004566
11.05.2012 16:24 grayman Status assigned => resolved
11.05.2012 16:24 grayman Resolution open => unable to reproduce
11.05.2012 16:24 grayman Target Version TDM 1.08 =>
11.05.2012 17:40 tels Note Added: 0004573
11.05.2012 17:40 tels Status resolved => feedback
11.05.2012 17:40 tels Resolution unable to reproduce => reopened
11.05.2012 18:19 grayman Assigned To grayman =>
07.10.2012 13:07 CodeMonkey Note Added: 0004869
05.02.2019 22:49 nbohr1more Relationship added related to 0004987
09.02.2019 20:11 VanishedOne Note Added: 0011561
14.02.2019 10:30 STiFU Assigned To => STiFU
14.02.2019 10:30 STiFU Status feedback => assigned
14.02.2019 10:37 STiFU Note Added: 0011593
14.02.2019 10:38 STiFU Status assigned => resolved
14.02.2019 10:38 STiFU Resolution reopened => fixed
14.02.2019 10:38 STiFU Fixed in Version => TDM 2.08
14.02.2019 10:38 STiFU Target Version => TDM 2.08
06.12.2019 16:34 stgatilov Relationship added related to 0005038
18.07.2020 15:33 stgatilov Relationship added related to 0005302