View Issue Details
|ID||Project||Category||View Status||Date Submitted||Last Update|
|0002993||The Dark Mod||Design/Coding||public||24.01.2012 16:24||03.12.2020 19:05|
|Priority||normal||Severity||normal||Reproducibility||have not tried|
|Product Version||TDM 1.07|
|Target Version||TDM 2.08||Fixed in Version||TDM 2.08|
|Summary||0002993: Removing an inventory item can invalidate cursor, resulting in crash|
|Description||In 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 Reproduce||Make a custom category with at least 2 items, remove the second last item |
(use on object in world, with script function
Try removing the other item -> crash
|Tags||No tags attached.|
|As we attempt to wrap up 1.08, is this fix required, or can it wait?|
|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.|
|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.|
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.
|And how do I "make a custom category"?|
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:
Button 2 calls a script that does this:
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.)
|Can't reproduce the crash either, looks like this is not an issue any more.|
May I close it,then?
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
// Now remove the item, the cursors are updated.
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
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.
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.
|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.|
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.
|24.01.2012 16:24||angua||New Issue|
||Note Added: 0004539|
|05.05.2012 17:03||tels||Note Added: 0004543|
|05.05.2012 18:06||tels||Note Added: 0004544|
||Note Added: 0004546|
||Note Added: 0004547|
||Note Added: 0004548|
||Assigned To||=> grayman|
||Status||new => assigned|
|10.05.2012 18:19||angua||Note Added: 0004563|
||Note Added: 0004566|
||Status||assigned => resolved|
||Resolution||open => unable to reproduce|
||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|
||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|