Feature - Implemented Allow set_location($location[none]) to set your location to none

[WARriorer]

Not sure if this was a recent change or the intended behaviour - I seem to remember that this used to work, but I'm not seeing anything super recent in `git blame` (in `KoLAdventure`) though, so I could very well be mis-remembering things.

 

[MCroft]

It looks like it's been that way since at least 2013:

public static void setNextAdventure(final KoLAdventure adventure) {
if (adventure == null) {
return;
}

 

[WARriorer]

Yeah that's what I saw when poking around as well.

In view of this, should we consider allowing set_location() to set our location to $location[None]? This would be useful to have a general prediction of what our skill/effect bonuses are (e.g. excluding zone/location-specific effects like Speluck and How to Scam Tourists). Choosing a default location like $location[The Overgrown Lot] instead of $location[None] indicates that we might be analysing our effects in an Outdoor area, which may not be what we want.

I've also gone ahead and changed the thread tag from "Bug" to "Feature", but if the understanding is that scripters should know exactly where they are adventuring next (not always true under the effects of Feeling Lost and Teleportitis, but this is an extremely contrived example), then we can go ahead and close this issue.

 

[MCroft]

Hmm. I would be concerned that that change might introduce churn to existing scripts that, sometime in the last decade, had used the existing behavior in some way and would now break.

It might be ok to create a new command like clear_location().

 

[WARriorer]

I think clear_location() seems good; but I just want to muse that set_location($location[none]) is currently a no-op because of the early return, so any existing scripts coding in such a no-op would likely have intended it to be something other than said no-op.

Currently $location[none] is indeed a valid location defined by mafia, but set_location($location[none]) does not in fact set our location to none, which does feel a little misleading about the behaviour of set_location()

 

[Veracity]

Datatypes:

  public static final Value LOCATION_INIT = new Value(DataTypes.LOCATION_TYPE, "none", null);

RuntimeLibrary:

  public static Value set_location(ScriptRuntime controller, final Value location) {
    KoLAdventure adventure = (KoLAdventure) location.rawValue();
    KoLAdventure.setNextAdventure(adventure);
    return DataTypes.VOID_VALUE;
  }

So, yeah - the KoLAdventure is null.

GenericRequest:

  private static void checkItemRedirection(final String location) {
...
    if (nextAdventure == null) {
      KoLAdventure.setLastAdventure("None");
      KoLAdventure.setNextAdventure("None");
    } else {
      KoLAdventure adventure = AdventureDatabase.getAdventure(nextAdventure);
      KoLAdventure.setLastAdventure(adventure);
      KoLAdventure.setNextAdventure(adventure);
      EncounterManager.registerAdventure(adventure);
    }
...
  }

We certainly have the concept of "none" for the next location; almost all items that lead to fights are in that location.
(lynyrd snare, rusty hedge trimmers, and white page are the only exceptions.)

KoLAdventure:

  public static void setNextAdventure(final String adventureName) {
    KoLAdventure adventure = AdventureDatabase.getAdventure(adventureName);
    if (adventure == null) {
      Preferences.setString("nextAdventure", adventureName);
      KoLCharacter.updateSelectedLocation(null);
      return;
    }
    KoLAdventure.setNextAdventure(adventure);
    EncounterManager.registerAdventure(adventureName);
  }

  public static void setNextAdventure(final KoLAdventure adventure) {
    if (adventure == null) {
      return;
    }

    Preferences.setString("nextAdventure", adventure.adventureName);
    KoLCharacter.updateSelectedLocation(adventure);
    NamedListenerRegistry.fireChange("(koladventure)");
  }

You can set the nextAdventure either from an adventureName or a KoLAdventure.
Similar methods exist for setLastAdventure.

GenericRequest.checkItemRedirection uses "lookup by name" - where "name" is usually "None".
RuntimeLibrary sets ONLY setNextAdventure and ONLY via KoLAdventure.

I find it odd that setNextAdventure(KoLAdventure) - with non null KoLAdventure does this:

    Preferences.setString("nextAdventure", adventure.adventureName);
    KoLCharacter.updateSelectedLocation(adventure);
    NamedListenerRegistry.fireChange("(koladventure)");

and setNextAdventure(String adventureName) - with a name that resolves to a valid KoLAdventure does this:

    KoLAdventure.setNextAdventure(adventure);
    EncounterManager.registerAdventure(adventureName);

i.e. those three lines and also registering the adventure in EncounterManager.

Perhaps those are correct for most uses of those methods.
Huh. AdventureSelectPanel.valueChanged:

      KoLAdventure location = AdventureSelectPanel.this.getSelectedAdventure();
      if (location != null) {
        KoLAdventure.setNextAdventure(location);
      }

This is equivalent of what RuntimeLibrary wants to do: set the adventuring location in a way that location-specific modifiers are visible.
Note that doing it that way (correctly?) does not register the adventure in EncounterManager.

Something funny:

setNextAdventure(KoLAdventure):

NamedListenerRegistry.fireChange("(koladventure)")

KoLCharacter.updateSelectedLocation(KoLAdventure):

PreferenceListenerRegistry.firePreferenceChanged("(location)");

The first one is used only by the AdventureFrame and the second is for scripts, I guess; it is not used internally.

I think RuntimeLibrary, as opposed to AdventureSelectPanel has a valid use for setting nextLocation to "none".

Perhaps it wants sort of a hybrid approach which does exactly the minimum?

  public static Value set_location(ScriptRuntime controller, final Value location) {
    KoLAdventure adventure = (KoLAdventure) location.rawValue();
    if (adventure == null) {
      Preferences.setString("nextAdventure", "None");
      KoLCharacter.updateSelectedLocation(null);
    } else {
      Preferences.setString("nextAdventure", adventure.getAdventureName());
      KoLCharacter.updateSelectedLocation(adventure);
    }
    return DataTypes.VOID_VALUE;
  }

This will not update the AdventureFrame (GUI) - which seems correct, for speculation.

 

[Veracity]

I don't see an issue with changing this, so PR https://github.com/kolmafia/kolmafia/pull/1679 is out for review.
Let's see what my code reviewrs have to say about it.

 

[Veracity]

r27331