Bug - Fixed goblin water causes questL05Goblin to be marked as finished in casual

B

Baden

Member
On r26239, questL05Goblin changed from started to finished after adventuring in An Overgrown Shrine (Northwest) during my latest casual run. I think this was caused by the goblin water dropping from Mr. Cheeng's spectacles.

Code: 
[158] An Overgrown Shrine (Northwest)
Preference lastEncounter changed from Water You Dune to dense liana
Encounter: dense liana
Round 0: Baden wins initiative!
Round 1: Baden wins the fight!
After Battle: Humphry kicks you in the thigh, in just the right place to be very effective chiropracty. If that's a word.
After Battle: You gain 18 hit points
After Battle: You gain 25 Muscularity Points
After Battle: Shifty Crystal suddenly appears next to you and drops some meat in your sack.
You gain 12 Meat.
After Battle: Humphry is starting to make audible sloshing noises as he walks around.
After Battle: You see a weird thing out of the corner of your eye, and you grab it. Far out, man!
You acquire an item: goblin water
Preference questL05Goblin changed from started to finished
This combat did not cost a turn
 
MCroft

MCroft

Developer
Staff member
So that used to be the quest result for Heavy Rains

ResultProcessor.java line 2064 has this:
Java: 
   case ItemPool.GOBLIN_WATER:
        if (adventureResults) {
          QuestDatabase.setQuestProgress(Quest.GOBLIN, QuestDatabase.FINISHED);
        }
        break;

We could put something in there to require you to be in HR for this to count, but it still might not be correct if you didn't just fight the Aquagoblin.
 
Veracity

Veracity

Developer
Staff member
I’m going to start a Heavy Rains run, equip Cheeng’s Spectacles, and open a new Bug Report, as necessary.

(You are not really going to ignore the real issue on this, are you?)

Write a test which shows the issue - both out of and in Heavy Rains- and fix the real problem.
 
Veracity

Veracity

Developer
Staff member
To be less snide: if you were not fighting the Aquagoblin, you did not advance the quest.
If you were, you did.
Which is to say, quest advancement should depend on defeating the boss, not an item drop.
 
MCroft

MCroft

Developer
Staff member
Having touched that code recently, I'd take that bug, and you're right, it will be there. Also, I've already started looking at it, so a good bug report from a character with Cheeng's would be great. If we're really trying to find all the edge cases, we could fight the Aquagoblin and have the Spectacles *also* drop Goblin Water. (Should be fine, but I can't code to avoid just based on Spectacles text).

I could've left the fix until it covered every case, but as Chris said, it was a useful interim patch.

On to the real issue: there are 105 instances of "received item/set quest progress" in gainItem, which could have the same issue. The pattern of "look for item acquisition to determine quest progress" is in one case known to have a false positive. The other 104 are potentially also subject to failure and are not future proofed.

So, to fix the real problem I want to implement a tighter check as you mentioned, but any other potion in that 105 might also be summoned by Cheeng's Spectacles. There also other ways to receive stuff in a fight without really fighting the fight needed to advance the quest.

From the FightRequest thread from two months ago:
KoL Con 13 Snowglobe, Mr. Screege's spectacles, Mr. Cheeng's spectacles, lucky gold ring
We also may need to consider Candyblast (Crimbo Carol Ch 3 skill) and Retrospecs, but I'm not sure if those can cause out-of-sequence quest drops.

We might be fine on Scrooge and lucky gold ring, since those are mostly coinage/cash drop source. The snow globe is also a potion (and food and booze) drop source, and the Talk page on the wiki from 2016 mentions "A Boss Drop from Heavy Rains", so it can probably drop the Goblin Water.

So, there's something to dig through line by line and see which are problems and then how to detect the difference between a quest-ending drop and a random drop, and then test cases and implentation.

I'm gonna be a while. :/
 
MCroft

MCroft

Developer
Staff member
... and most of the 105 aren't potions, drinks, or food, so my concern was mostly misplaced. I hope to have a better for that one and maybe a few others in a day or so.

So if I scope it only to consumables, it gets smaller.
 
MCroft

MCroft

Developer
Staff member
VladYvhuce said:
Does the Can of Mixed Everything count as a potential offender, also? It drops random potions, food, and boozes as well.
Click to expand...
Well, I'm going through things in ResultProcessor.java that setQuestProgress on item acquisition.

It shouldn't matter if Can of Mixed Everything summons the same things as other potion/food/booze summoners. I'm thinking this is a much smaller hole than I was worried about, but I am worried about things like wishes that might summon quest items.
 
Veracity

Veracity

Developer
Staff member
I agree that only quest items coming into inventory should be candidates for advancing a quest via item acquisition.
I'd be shocked if KoL provided a way for quest items to appear that does NOT advance the quest.
 
MCroft

MCroft

Developer
Staff member
Finally got back to this. Stopped chasing my tail trying to figure out why it wasn't showing the throneroom as the last URL visited and just looked at lastMonster. Also added tests, as requested.

PR#689
Veracity said:
I’m going to start a Heavy Rains run, equip Cheeng’s Spectacles, and open a new Bug Report, as necessary.

(You are not really going to ignore the real issue on this, are you?)

Write a test which shows the issue - both out of and in Heavy Rains- and fix the real problem.
Click to expand...
 
You must log in or register to reply here.
Top