Transfer inhaler to NPC inventory on mission end

[wapcaplet]

Summary

SUMMARY: Bugfixes "Let asthmatic NPC keep inhaler after mission complete"

Purpose of change

Attempted fix for #31773

Describe the solution

In my early testing I learned that for missions with MGOAL_FIND_ITEM type, the found item appears to be simply deleted when the mission is complete:

Cataclysm-DDA/src/mission.cpp

Lines 327 to 329 in bfee67a

	case MGOAL_FIND_ITEM:
	comps.push_back( item_comp( type->item_id, item_count ) );
	u.consume_items( comps );

To solve this, I changed the mission goal to MGOAL_CONDITION, with a condition of "u_has_item": "inhaler". Effectively the same criteria, but prevents the wrap_up function from deleting it.

Then, I added an ending effect of "u_sell_item": "inhaler" to transfer it to the NPC's inventory. Sounds good, right?

Well, what happened was that my inhaler loses (1) of its possibly (100) charges, and the NPC gains a new fully-charged inhaler. Complete the quest again for another NPC, and they get a full inhaler, while mine drops to (98/100). This seems like a bug for which I'll open a separate ticket when I have time to test it more (if someone else doesn't do it first!)

My crude workaround to this bug is, after quest completion, to remove the player's inhaler from inventory with "u_remove_item_with": "inhaler". Unfortunately this will remove all inhalers, since there appears to be no way to remove only 1, unless u_remove_item_with accepts a count that isn't documented in NPCs.md, so hopefully the player isn't hoarding them to distribute to a bunch of NPCs. The upside is, a depleted inhaler will be magically refilled on transfer due to the above bug, so if you have an inhaler with (1/100) charges, I think it will become (100/100) in the NPC's inventory.

Workaround turned into a working solution with the help of other developers - see discussion below.

Describe alternatives you've considered

Considered looking at a more general way of allowing NPCs to keep completed quest items for MGOAL_FIND_ITEM, but didn't want to run into volume concerns (for example with Draco's guitar quest) of NPC being unable to hold the thing. Inhaler is small enough to avoid this problem, I hope.

Testing

Using an invincible player in a test world, I spawned many NPCs until I got asthmatic ones, and spawned inhalers as needed to complete their quests. Then I asked the NPCs to trade and made sure they had an inhaler in their inventory.

Additional context

I had a personal interest in fixing this, because my latest character is in a world with maximum (100.0) NPC spawns, and the first friendly recruit she found was asthmatic.

[I-am-Erk]

Hmm. I don't think this is an adequate solution to the problem, it just creates a new bug - that you give the npc all your inhalers. I would suggest you keep the u_sell_item entry, because that's the correct entry to use, and we should expand the original bug report to include the new problem.

At least for verissimilitude, the NPC appears to be using a puff of the inhaler you brought

[wapcaplet]

Agreed, I don’t like the current solution either. Fixing u_sell_item to transfer the inhaler item instead of just 1 charge seems like the right thing to do. I’ll take a stab at it and add to the PR.

[wapcaplet]

It's a bit complicated. This appears to be the relevant section of set_u_sell_item:

Cataclysm-DDA/src/npctalk.cpp

Lines 1971 to 1990 in ff90cbb

	npc &p = *d.beta;
	player &u = *d.alpha;
	item old_item = item( item_name, calendar::turn );
	if( u.has_charges( item_name, count ) ) {
	u.use_charges( item_name, count );
	} else if( u.has_amount( item_name, count ) ) {
	u.use_amount( item_name, count );
	} else {
	//~ %1$s is a translated item name
	popup( _( "You don't have a %1$s!" ), old_item.tname() );
	return;
	}
	if( old_item.count_by_charges() ) {
	old_item.mod_charges( count - 1 );
	p.i_add( old_item );
	} else {
	for( int i_cnt = 0; i_cnt < count; i_cnt++ ) {
	p.i_add( old_item );
	}
	}

I think what's happening here is, after quest completion:

• L1973: The old_item is initialized to a fully-charged item_name="inhaler" item
• L1974-1975: My inhaler matches the has_charges condition, so count=1 charge is depleted from my inhaler
• L1983-1985: The old_item has its charges reduced by count - 1 = 0 (??) before being given to the NPC

My guess is the has_charges and mod_charges pieces are intended to handle some other item that trades in charges (cigarettes maybe?).

I've done some testing using the normal NPC "let's trade items" UI, and when I do it that way, the entire inhaler item (with its remaining charges) is transferred as expected. Seems like it's only when u_sell_item is a mission-complete effect that this weird charge-transferral thing happens. I'm guessing that's because the "let's trade" UI is handled by completely different functions in npctrade.cpp

[BevapDin]

L1983-1985 are not used. The inhaler is not counted by charges, old_item.count_by_charges() will yield false.

The problem is the u.has_charges above. It is "correct" (as in: it works) for things that are actually counted by charges. This check should be the same as below when it's checked which function to use to give items to the NPC. It should check for count_by_charges and if so, call use_charges and otherwise call use_amount.

Final note: use_charges and use_amount return the items they are consuming. Those items cane be given to the NPC directly without constructing a new one. This will give the very same inhaler to the NPC (including modifications like charges and damage).

[wapcaplet]

Thanks @BevapDin for the explanation - this helped me make some progress.

I think my last commits are pretty close to a final fix, but I'd appreciate further review and suggestions from devs. Here is the current behavior I see in my testing:

• Have 1 inhaler, complete mission:
  • 1 inhaler is removed from my inventory
  • NPC gets 1 fully-charged inhaler (100/100)
• Have 3 inhalers, complete mission:
  • 1 inhaler is removed from my inventory (leaving 2)
  • NPC gets 1 fully-charged inhaler (100/100)
• Have 1 inhaler with (97/100) charge, complete mission:
  • 1 inhaler (97/100) is removed from my inventory
  • NPC gets 1 fully-charged inhaler (100/100)

If the inhaler is really not to be treated as having charges, I don't see a way around the magical-recharge-on-transfer issue (at least not without further rewrites to this function, which I don't feel too confident attempting).

[wapcaplet]

The layers of onion around this bug are many. My latest commit is very close to working exactly as expected, except I have to save and reload my game in order for the inhaler to appear in the NPC's inventory.

[BevapDin]

, except I have to save and reload my game in order for the inhaler to appear in the NPC's inventory.

I just tested your branch and it works fine for me: the NPC has my inhaler - it shows up when trading with the NPC.

How do you check for the appearance of the inhaler in the NPC inventory?

[wapcaplet]

This is how I've been setting up the inhaler quest for testing:

• Debug spawn NPC and hope they're not hostile
• Chat with NPC and see if they have a mission
• If not the inhaler mission, use debug to add the inhaler mission to the NPC
• Use debug to complete their current mission (edit NPC directly or spawn item they want)
• Chat with NPC and complete curent mission, get inhaler quest
• Debug spawn an inhaler, maybe take a couple hits to see if charges are preserved

Here's the "You give X a inhaler" (sic) popup displaying as expected:

After this, if I choose "How about some items as payment?" or quit, chat again, and ask "Care to trade?", their inventory does not include the inhaler:

But after a quick save and reload, it is there, with the correct number of charges:

• OS: Linux
  • OS Version: LSB Version: core-9.20170808ubuntu1-noarch:security-9.20170808ubuntu1-noarch; Distributor ID: Ubuntu; Description: Ubuntu 18.04.3 LTS; Release: 18.04; Codename: bionic;
• Game Version: 0.D-11826-ge4b3f3d168 [64-bit]
• Graphics Version: Tiles
• Mods loaded: [
  Dark Days Ahead [dda],
  Disable NPC Needs [no_npc_food]
  ]

[wapcaplet]

I tested this again with a recent pull from master, and got the same results. For the first NPC, it actually worked immediately - after quest completion, they had the inhaler in their inventory right away. But for the next two NPCs I spawned for testing, neither would show the inhaler in their inventory until after I saved/loaded the game.

Considering the failed testing, I'm gonna leave this as WIP until I can figure out what's going on.

[wapcaplet]

Alright, I have now tested this some more, and found that the inhaler does appear in the NPC's inventory (visible in the "let's trade items" screen) if I successfully persuade the NPC to travel with me (two successes so far). I didn't do extended testing to see if they were able to treat their asthma attacks.

Having other conversations with the NPC or waiting a while does not seem to trigger the appearance of the inhaler in the trading screen. So maybe the notion is they aren't ready to immediately trade away the very item they wanted for their quest. In any case, saving and loading has always allowed me to see the inhaler in the trade UI, so the item transfer is certainly happening as expected.

I spent some time looking into maybe adding some test cases for this edge case to tests/npc_talk_test.cpp, but couldn't quite get my head around it, and I see there is already quite thorough coverage there.

Long story short, I am satisfied that this is ready to merge now, so I am removing the WIP tag for what I hope is the last time.