Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix wielding from inventory #28276

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@ifreund
Copy link
Contributor

commented Feb 19, 2019

Summary

SUMMARY: None

Purpose of change

Fix debug message on wielding from inventory caused by #28248
Fix #28281
Only consume moves on success.

Describe the solution

Because player::wield() updates the inventory, the item_location is invalidated, causing a debug message on trying to remove the item. This fixes that by using character::i_rem() instead.

This also fixes errant removal of items if the wield was canceled by checking for success before removal.

Additional context

Guess I should've tested more.

Show resolved Hide resolved src/game.cpp Outdated

@ZhilkinSerg ZhilkinSerg added this to the 0.D milestone Feb 20, 2019

Fix wielding from inventory
Co-Authored-By: ifreund <isaac.a.freund@gmail.com>

@ifreund ifreund force-pushed the ifreund:fix-wield-from-inventory branch from a14d480 to 4b7e8df Feb 20, 2019

@ifreund

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

now fixes #28281 as well, edited to reflect that.

// The item_location gets invalidated if wielding an item from the inventory due to updating of
// the cache, so we have to use u.i_rem() instead to avoid a debug message.
const item *target = loc.get_item();
item to_wield = *target;

This comment has been minimized.

Copy link
@ifreund

ifreund Feb 20, 2019

Author Contributor

It seems like there should be a better way to do this but I wasn't thinking of one

@Tatterdemalian

This comment has been minimized.

Copy link

commented Feb 21, 2019

Not sure if this bugfix has been implemented in build 0.C-37535-g409acf797d, but the error is no longer intermittent. It now happens consistently whenever I attempt to wield anything from my inventory:

DEBUG : item location does not point to valid item

FUNCTION : void item_location::remove_item()
FILE : src/item_location.cpp
LINE : 601

To Reproduce

  1. Open wield menu.
  2. Select item from first column in menu (player inventory).
  3. If an item is already being wielded, select what to do with it from the dialog that appears.

Versions and configuration

  • OS: Linux 4.19.23 Gentoo (new kernel released today, this might also have something to do with the new bug)
  • Game Version: 0.C-37535-g409acf797d
  • Graphics version: Tiles
  • Mods loaded: Defaults only: Dark Days Ahead, Disable NPC Needs, Filthy Clothing, Simplified Nutrition

Additional context

debug.log:
CDDADebug.log

Tested several times with various combinations of wielding. The error seems to occur when an item is moved to or from the player inventory by the wield action. It does NOT occur when:

  • An item is wielded from the floor when the player is not wielding any item.
  • The item selected for the wield command is the item currently being wielded, AND the option to drop the item or wear it is selected from the "Stop wielding {item}" dialog. Selecting the "Store in inventory" option DOES cause the error.
  • An item is wielded from the floor, when another item is being wielded, and the "Drop item" or "Wear item" options are chosen from the "Stop wielding {item}" dialog.

It does occur when an item is wielded directly from the worn inventory, but not when an item being un-wielded is worn, which seems to indicate that there is a step where an item being wielded is moved from the worn inventory to the carried inventory as it's wielded, but moved directly to the worn inventory when it's un-wielded.

@kevingranade

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

This pull request has been mentioned on Cataclysm: Dark Days Ahead. There might be relevant details there:

https://discourse.cataclysmdda.org/t/android-error-android-cdda/18872/3

@kevingranade

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

Merged to 0.D and master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.