Bug - Fixed Lucky Gold Ring thinks the Halloweiner Volcoino is a LGR Volcoino Drop

[Tokoeka]

When adventuring in the Bubblin' Caldera whilst wearing the Lucky Gold Ring, if you get the Halloweiner Dog Free Noncombat that gives you a Volcoino before the Lucky Gold Ring has dropped its volcoino for the day, mafia sets the "_luckyGoldRingVolcoino" property to true.

r20810

 

[lostcalpolydude]

That... shouldn't happen. The code to set "_luckyGoldRingVolcoino" checks for it dropping from combat.

If that's not working for some reason, then there's a bigger issue.

 

[Tokoeka]

I've noticed it doing it on multiple occasions and I know someone else who has an aftercombat script set up to check if the last adventure name was the halloweiner noncombat volcoino drop and to reset "_luckyGoldRingVolcoino" in that case.

 

[gausie]

That... shouldn't happen. The code to set "_luckyGoldRingVolcoino" checks for it dropping from combat.

If that's not working for some reason, then there's a bigger issue.

I can see the bigger issue:

kolmafia/src/net/sourceforge/kolmafia/request/GenericRequest.java at main · kolmafia/kolmafia

KoLmafia is a cross-platform desktop tool which interfaces with the online adventure game Kingdom of Loathing 🗡️🍸 - kolmafia/kolmafia

github.com

This code means any request to adventure.php is treated as a combat by the results processor - this is the cause of the issue. I don't understand why its written like this and I worry fixing it could break other things.

I've written a PR with a regression test to validate the issue: https://github.com/kolmafia/kolmafia/pull/466

 

[gausie]

@Veracity care to cast your mind back a short 12 years and give your opinion?

 

[Veracity]

I have no idea what that was originally about. Too bad I slid that change in with the pxel whip changes but the commit message only mentioned the latter.

However, at a guess, adventure.php can give you items directly from an NC - like two olives from "Olive My Love To You, Oh" in the Dark Neck of the Woods and, if we had special actions on olives entering inventory from adventuring vs. pulling or uncloseting them, calling those a "combat" result is how you distinguish.

GenericRequest no longer contains a "containsUpdate" field. However, the equivalent code appears in GenericRequest.parseResults:

    if (urlString.startsWith("adventure.php")) {
      ResultProcessor.processResults(true, this.responseText);
      return;
    }

So, the issue is that there are two ways to get a Volcoino via "adventuring" - as a post-fight drop from the lucky gold ring, and as a NC in the Bubbling Caldera, but there is a property to track the first one which should exclude the second.

I believe that treating adventure.php results as "combat" results is correct. Perhaps it would be more obvious if we called them "adventuring" results?

I'll look at your PR. Writing the regression test first is a good idea. Fixing the issue is not so easy.

 

[Veracity]

Look at FightRequest.processNode:

      if ( // KoL Con 13 Snowglobe
      str.contains("KoL Con")
          || str.contains("You notice some extra Meat")
          ||
          // Mr. Screege's spectacles
          str.contains("You notice something valuable hidden")
          ||
          // Mr. Cheeng's spectacles
          str.contains("You see a weird thing out of the corner of your eye, and you grab it")
          || str.contains("You think you see a weird thing out of the corner of your eye")
          ||
          // lucky gold ring
          str.contains("Your lucky gold ring gets warmer for a moment.")) {
        FightRequest.logText(str, status);
      }

"status" is a TagStatus object. We could add a field to that - luckyGoldRingDrop - and set it to true when we detect the message. Then, later, somehow, manipulate the property from within FightRequest (where we know it was a lucky gold ring drop) rather than ResultProcessor.

Or something like that.

 

[Veracity]

Or, you know, considering that "combatResults" is true for either a fight or an NC, perhaps the simplest is to keep the property setting in ResultProcessor and have the conditional be something like "if ( combatResults && !lastEncounter.equals("the name of the wiener dog encounter") )".

 

[gausie]

I ended up just checking for the drop line at the end of combat and removing the check from ResultProcessor entirely

 

[Veracity]

OK. Although, as coded, isn’t that another String search of the entire responseText? Seems like that could be incorporated into the HTML parsing of the fight response where we look for the lucky gold ring.

if str contains “your lucky gold ring gets warm” {
    log text
    if str contains “Volcoino” {
        set property
    }
}

Whatever.

 

[gausie]

OK. Although, as coded, isn’t that another String search of the entire responseText? Seems like that could be incorporated into the HTML parsing of the fight response where we look for the lucky gold ring.

if str contains “your lucky gold ring gets warm” {
    log text
    if str contains “Volcoino” {
        set property
    }
}

Whatever.

I agree! I noticed that nothing else in that logging function actually set any prefs so I didn't want to. I'll do something with status to make it a little more performant.

 

[Veracity]

Well, yeah. Don’t change the logging function. Change the place where it detects the lucky gold ring message and calls the logging function - and then looks to see if it should set the property.

There are other places in that HTML tree walking where it might call, for example, handleLuckyGoldRing(node), and that method would see if it needed to log, and if so, set a property.

I am all for somebody else also being familiar with how FightRequest walks the HTML tree an being comfortable tweaking it.

 

[gausie]

What do you think of this?

Lucky gold ring volcoino issue by gausie · Pull Request #466 · kolmafia/kolmafia

Here is a failing regression test for https://kolmafia.us/threads/lucky-gold-ring-thinks-the-halloweiner-volcoino-is-a-lgr-volcoino-drop.26245/ to show the issue

github.com

 

[Veracity]

I prefer this:

diff --git a/src/net/sourceforge/kolmafia/request/FightRequest.java b/src/net/sourceforge/kolmafia/request/FightRequest.java
index 3354d5a868..ff4e05a880 100644
--- a/src/net/sourceforge/kolmafia/request/FightRequest.java
+++ b/src/net/sourceforge/kolmafia/request/FightRequest.java
@@ -6346,13 +6346,12 @@ public class FightRequest extends GenericRequest {
           ||
           // Mr. Cheeng's spectacles
           str.contains("You see a weird thing out of the corner of your eye, and you grab it")
-          || str.contains("You think you see a weird thing out of the corner of your eye")
-          ||
-          // lucky gold ring
-          str.contains("Your lucky gold ring gets warmer for a moment.")) {
+          || str.contains("You think you see a weird thing out of the corner of your eye")) {
         FightRequest.logText(str, status);
       }
 
+      FightRequest.handleLuckyGoldRing(str, status);
+
       // Retrospecs
       if (str.contains("notice an item you missed earlier")) {
         FightRequest.logText(str, status);
@@ -7133,6 +7132,16 @@ public class FightRequest extends GenericRequest {
     }
   }
 
+  private static void handleLuckyGoldRing(String str, TagStatus status) {
+    if (!str.contains("Your lucky gold ring gets warmer for a moment.")) {
+      return;
+    }
+    FightRequest.logText(str, status);
+    if (str.contains("You look down and find a Volcoino!")) {
+      Preferences.setBoolean("_luckyGoldRingVolcoino", true);
+    }
+  }
+
   private static boolean handleProselytization(TagNode node, TagStatus status) {
     String str = node.getText().toString();

 

[gausie]

Done! Thanks. I'll merge this

 

[gausie]

Fixed