Bug - Fixed assigning the buddy bjorn a bonus while using -buddy-bjorn results in failure

[phreddrickk]

A fix for a related (but not identical) issue was implemented here (thanks, Fronobulax and Midgley). A similar issue persists when dealing with the bonus keyword, instead of the equip keyword.


When I maximize with -buddy-bjorn, the bjorn slot empty, and a bonus attached to the bjorn, the maximizer wants to both actively vacate my back slot (at a negative cost!) and, interestingly, not fill the hat slot.

Weirdly, pulling the crown of thrones from my clan stash changes this behavior. Now, it wants to equip my bjorn, but it also wants to bjornify something (illegally, sort of)

Disabling the crown-of-thrones slot returns to the original behavior of actively unequipping the bjorn.

Weird!

Enthroning a familiar does not seem to change this behavior at all. Adding `-equip crown of thrones` to my maximizer string returns to the original behaviour as well.

So it seems like considering the crown of thrones as an item has an impact on the behavior of the maximizer with the bjorn.

I suspect that this may have something to do with trying not to bjornify and encromulate the same familiar, that familiar being $familiar[none]. I'll dig into this further, but I just wanted to share how weird this is! It's all very neat.

I've found myself unable to write a failing test associated with this, which to me implies that either I'm doing something wrong writing the tests (honestly, a very plausible thing) or that there's some additional confounding factor that's true of every character-state I've tested, but is not true in the very simple, idealized scenarios under which maximizer tests are run.

 

[heeheehee]

A similar issue persists when dealing with the bonus keyword, instead of the equip keyword.

That's because item.requiredFlag isn't true in that case.

When I maximize with -buddy-bjorn, the bjorn slot empty, and a bonus attached to the bjorn, the maximizer wants to both actively vacate my back slot (at a negative cost!) and, interestingly, not fill the hat slot.

This sounds familiar. I think the hat issue is new. Do you happen to have autoSatisfyWithClanStash = true by chance?

I suspect that this may have something to do with trying not to bjornify and encromulate the same familiar, that familiar being $familiar[none]. I'll dig into this further, but I just wanted to share how weird this is! It's all very neat.

Indeed.

(I most likely don't have time to look into this.)

Linking related (?) threads in case they're helpful to whoever ends up fixing this:

Bug - Fixed - -buddy-bjorn and -crownofthrones maximizer keywords failing when bjorn or crown isn't equipped

The issue is more or less in the title here. the `-buddy-bjorn` keyword should prevent the bjornified familiar from being changed by the maximizer. If you run the maximizer while the bjorn is equipped, it functions as intended. If you run the maximizer without the bjorn equipped, however, it...

kolmafia.us

Bug - using the -buddy-bjorn maximizer command unequips the buddy bjorn item

Hi again! This issue is linked to the one resolved/discussed in this thread. Running the maximizer with a string like Familiar Weight, -buddy-bjorn, -crown-of-thrones, 100 bonus pantogram pants, 145.5 bonus Buddy Bjorn, 180 bonus Mr. Screege's spectacles, 250 bonus Mr. Cheeng's spectacles...

kolmafia.us

 

[phreddrickk]

That's because item.requiredFlag isn't true in that case.

Yep, which means that whatever fix we end up drumming up for this will probably(?) obviate item.requiredFlag

This sounds familiar. I think the hat issue is new. Do you happen to have autoSatisfyWithClanStash = true by chance?

I do not; if I did, I feel like that would be even weirder. I honestly wasn't sure if this was still part of the scope of the same thread as before, given that that thread started with something slightly different still, so I figured I'd start anew, like a phoenix from the flames. I'm sorry if that's gauche.

(I most likely don't have time to look into this.)

I plan on continuing to investigate for at least a little while.

Linking related (?) threads in case they're helpful to whoever ends up fixing this:

Bug - Fixed - -buddy-bjorn and -crownofthrones maximizer keywords failing when bjorn or crown isn't equipped

The issue is more or less in the title here. the `-buddy-bjorn` keyword should prevent the bjornified familiar from being changed by the maximizer. If you run the maximizer while the bjorn is equipped, it functions as intended. If you run the maximizer without the bjorn equipped, however, it...

kolmafia.us

Bug - using the -buddy-bjorn maximizer command unequips the buddy bjorn item

Hi again! This issue is linked to the one resolved/discussed in this thread. Running the maximizer with a string like Familiar Weight, -buddy-bjorn, -crown-of-thrones, 100 bonus pantogram pants, 145.5 bonus Buddy Bjorn, 180 bonus Mr. Screege's spectacles, 250 bonus Mr. Cheeng's spectacles...

kolmafia.us

Thanks!

UPDATE:

for (FamiliarData f : enthronedFamiliars) {
  this.setBjorned(f);
  this.tryAccessories(enthronedFamiliars, possibles, 0, bestCard, useCrownFamiliar);
  any = true;
  this.restore(mark);
}

I believe the issue is here; both times we use this loop in this particular circumstance, enthronedFamiliars is empty.
I'm yet to successfully write a failing test for this, which is weird and frustrating, but adding a shitty little

enthronedFamiliars.add(FamiliarData.NO_FAMILIAR)

above each of those loops, and excising the item.requiredFlag parts, continues to let the tests not fail, which feels promising.

It is probably ultimately wiser to go to Evaluator.java and make carriedFamiliars start with FamiliarData.NO_FAMILIAR in it.

 

[phreddrickk]

Update: I figured it out!

The issue lies in the fact that right now we're using a value of FamiliarData.NO_FAMILIAR to communicate two very different ideas: either a carrier-familiar slot that is yet to be assigned a value, or one that has actively been assigned a value of none. I've built a mafia for myself that uses `null` for the former case, and it seems to be working swimmingly

 

[heeheehee]

Good find! I agree that it's nominally correct to consider FamiliarData.NO_FAMILIAR as a valid option.

(I would be happy if we moved away from sentinel values such as NO_FAMILIAR or null and instead used Optional<> with isPresent() to communicate that more explicitly. I don't think that needs to block a fix, though.)

 

[phreddrickk]

To continue our conversation from the PR:

It looks like the failure isn't coming from maximize, it is in fact coming from the `recommendedSlotIs(EquipmentManager.CONTAINER, "Buddy Bjorn")`. What's weird as hell is that this error persists when I remove the `-buddy-bjorn` from the maximizer string.

 

[heeheehee]

Hm. If recommendedSlotIs doesn't actually work for already-equipped items, then I'm not sure what the best choice is for observability. I suppose empty Crown of Thrones still provides 100 damage absorption, and I assume the same core issue affects CoT as much as it affects Bjorn.

 

[phreddrickk]

I'm switching over to modFor, but it definitely feels like this is either unintended behavior or I'm misunderstanding what's happening re: recommendedSlotIs.

 

[heeheehee]

I think this is an implementation detail that's leaking -- we're not recommending a change to the slot. It seems like recommendedSlotIsEmpty() should do the correct thing, although that's confusingly named. I'd personally rename that to recommendedSlotIsUnchanged(), although probably ought to be a separate refactoring change.

Further: recommendedSlotIs() is insufficiently powerful, as it cannot differentiate between "bjornify Fuzzy Dice" and "equip Buddy Bjorn". While we've been using modFor() as an indirect measurement of the effect, we probably should check the actual recommendation directly.

With recommendedSlotIsEmpty(), the first "reproducing" test (keepFilledBjornEquippedWithBonus) now passes (i.e. is insufficient to reproduce the issue), while the second one (keepEmptyBjornEquippedAndUnchangedWithCrownAndBonus()) still fails. With a more powerful recommendedSlotTextIs() helper, I can confirm that the underlying recommendation is "bjornify Fuzzy Dice".

  public static void recommendedSlotTextIs(int slot, String text) {
    Optional<Boost> boost =
        net.sourceforge.kolmafia.maximizer.Maximizer.boosts.stream()
            .filter(Boost::isEquipment)
            .filter(b -> b.getSlot() == slot)
            .findAny();
    assertTrue(boost.isPresent(), "Expected recommendation '" + text + " for slot " + slot + "', but slot was unchanged");
    assertEquals(text, boost.get().toString());
  }

 

[heeheehee]

Okay. I fixed the first test case by providing an alternative that helps, and simplified the second case slightly.

  public static Optional<Boost> getSlot(int slot) {
    return net.sourceforge.kolmafia.maximizer.Maximizer.boosts.stream()
        .filter(Boost::isEquipment)
        .filter(b -> b.getSlot() == slot)
        .findAny();
  }

  public static void recommendedSlotTextIs(int slot, String text) {
    Optional<Boost> boost = getSlot(slot);
    assertTrue(
        boost.isPresent(),
        "Expected recommendation '" + text + " for slot " + slot + "', but found nothing");
    assertEquals(text, boost.get().toString());
  }

  public static void recommendedSlotIs(int slot, String item) {
    Optional<Boost> equipment = getSlot(slot);
    assertTrue(equipment.isPresent(), "Expected " + item + " to be recommended, but it was not");
    assertEquals(AdventureResult.parseResult(item), equipment.get().getItem());
  }

  public static void recommendedSlotIsUnchanged(int slot) {
    Optional<Boost> equipment = getSlot(slot);
    assertTrue(
        equipment.isEmpty(),
        () ->
            "Expected unchanged "
                + EquipmentRequest.slotNames[slot]
                + " slot, but it was "
                + equipment.map(Boost::toString).orElse(""));
  }

...

  @Test
  public void keepBjornEquippedWithBonus() {
    // Provide a helpful alternative to the bjorn.
    canUse("makeshift cape");
    equip(EquipmentManager.CONTAINER, "Buddy Bjorn");

    assertTrue(maximize("-item, -buddy-bjorn, +25 bonus Buddy Bjorn"));
    recommendedSlotIsUnchanged(EquipmentManager.CONTAINER);
  }

  @Test
  public void keepBjornEquippedAndUnchangedWithCrownAndBonus() {
    canUse("Crown of Thrones");
    canUse("time helmet");

    KoLCharacter.addFamiliar(new FamiliarData(FamiliarPool.DICE));
    equip(EquipmentManager.CONTAINER, "Buddy Bjorn");

    assertTrue(maximize("adventures, -buddy-bjorn, +25 bonus Buddy Bjorn"));

    recommendedSlotIs(EquipmentManager.HAT, "time helmet");
    recommendedSlotIsUnchanged(EquipmentManager.CONTAINER);
  }

for context, improved error messages:

keepEmptyBjornEquippedAndUnchangedWithCrownAndBonus()
org.opentest4j.AssertionFailedError: Expected unchanged back slot, but it was bjornify Fuzzy Dice (+0) ==> expected: <true> but was: <false>
...
keepFilledBjornEquippedWithBonus()
org.opentest4j.AssertionFailedError: Expected unchanged back slot, but it was unequip back (Buddy Bjorn, -25) ==> expected: <true> but was: <false>

 

[phreddrickk]

I've partially implemented this, but when I ran the full gamut, basically every test that uses recommendedSlotIsEmpty started failing with messages like

org.opentest4j.AssertionFailedError: Expected unchanged hat slot, but it was <html><font color=gray>(nothing useful found)</font></html> ==> expected: <true> but was: <false>

I suspect this has to do with changing getSlot to use a Boost rather than an AdventureResult.

But it looks like just renaming recommendedSlotIsEmpty and using that in our tests is enough to make the tests fail for current mafia, and succeed for this version. So cheers!