Feature - Implemented Proposal: maintain lastAdventure and nextAdventure

Veracity

Developer
Staff member
In my opinion the graphical widget should reflect the state of some internal variable, rather than the other way around. Sounds like some refactoring is in order.
This is on one of several bug reports that exist because, in my opinion, we are trying to make "lastLocation" server two different roles. This note discusses the proposed refactoring.

Seems to me, we have two locations of interest:

lastAdventure - the location where you last spent a turn
nextAdventure - the location where you are about to spend a turn.

You can see lastAdventure in the charpane and in api.php?what=status:

"lastadv":{"id":"365","name":"The Spirit World","link":"adventure.php?snarfblat=365","container":"place.php?whichplace=forestvillage"}
"lastadv":{"id":"186","name":"The Briny Deeps","link":"adventure.php?snarfblat=186","container":"place.php?whichplace=thesea"}
"lastadv":{"id":"190","name":"An Octopus's Garden","link":"adventure.php?snarfblat=190","container":"seafloor.php"},
"lastadv":{"id":"195","name":"The Marinara Trench","link":"adventure.php?snarfblat=195","container":"seafloor.php"}
"lastadv":{"id":"207","name":"Mer-kin Elementary School","link":"adventure.php?snarfblat=207","container":"sea_merkin.php"}
"lastadv":{"id":"339","name":"Dreadsylvanian Village","link":"adventure.php?snarfblat=339","container":"clan_dreadsylvania.php"}
"lastadv":{"id":"316","name":"The Gourd!","link":"adventure.php?snarfblat=316","container":"place.php?whichplace=junggate_2"}

(Note that what KoL calls "zones" and "containers", KoLmafia calls "locations" and "zones". KoL "containers" do not map one-to-one with KoLmafia "zones"; we have a zone called "The Sea", whereas we can see at least three KoL "containers", all of whom contains KoL "zones" which are in "The Sea", as far as item enchantments are concerned.)

Used to be, we had our own location names, and we didn't bother looking at the two places that KoL displayed this, since our name and KoL's name didn't necessarily match. We've gone to a lot of trouble to make them match, and are probably 99% there, but we still don't actually save what we find there. We do look for new locations there and temporarily add them. Instead, we have a setting - lastAdventure - tells us what the last location was that we watched you adventure in.

That setting determines the "zone" and "location" that are applied to modifiers and is used to set the AdventureFrame in the GUI to track where you are currently adventuring.

How do we set lastAdventure?

- KoLAdventure.setNextLocation
-- from GenericRequest.invokeCounterScript
-- from DreadsylvaniaRequest when going straight to a noncombat
-- from KoLAdventure.run()
- ASH set_location
- GenericRequest.checkItemRedirection when using an item which will redirect to a fight
- UseItemRequest when using an item which we know uses adventures (is that redundant?)
- RelayRequest.sendWarnings when we detect you are about to "adventure"
- UseItemRequest when doing the "Welcome Back" adventure
- VioletFogManager when you are diverted by an astral mushroom

Remember that I said that we conceptually have two locations of interest? Unfortunately, we are trying to make do with one. We are missing nextAdventure. I think adding that will rationalize a lot of stuff, but it is an incompatible change: both beforeBattleScripts and counterScripts are invoked when we are about to adventure somewhere and in both cases, we set "lastAdventure" to where we are about to go.

Let's consider how I'd like it to work.

There will be two settings:

lastAdventure - the location where you last spent a turn

- When you log in, we look at api.php and set lastAdventure to what we see there - including registering an unknown adventure location. We could look at the "container" value and decide what the KoLmafia "zone" is.
- When we submit a URL to adventure somewhere - either via automation, via visit_url, or in the Relay Browser - we figure out which "adventure" this is and set lastAdventure to remember that.
- When we detect that we have been diverted - astral mushroom, Teleportitis (can't do this yet, unfortunately) - set lastAdventure to the real place we visited.

*** when we set lastAdventure, we update the Adventure Location in the GUI so you can see where you just went.

nextAdventure - the location where you are about to spend a turn.

- when we set lastAdventure, as a above, set nextAdventure to match.
- when you change the selected location in the GUI, set nextAdventure to new selection
- when we are about to submit a URL to adventure - via automation, visit_url, or the Relay Browser - set nextAdventure to where we are about to go
- When we are about to get into a fight via an item, set nextAdventure to None
- When we complete a fight, if nextAdventure is None, set it to lastAdventure

*** when we set nextAdventure, update modifiers.
*** The loc() and zone() functions in modifiers will now look at nextAdventure, not lastAdventure
*** beforeBattleScripts and counterScripts need to look at nextAdventure, not lastAdventure

Changes to observed behavior as a result of the above:

- when you use set_location() are select something in the GUI, modifiers update immediately.
- when you use an item which redirects to a fight, lastAdventure does not change
- when you complete an item-redirected fight, modifiers get set back to where they were before.
- even if a counterScript redirects you, which will change lastAdventure, when we submit the URL after invoking the counterScript, it will be changed back.

It will be tricky to figure out exactly the places where these setting are changed; I may not have them precisely correct. But I think this will be much more rational and will solve a number of problems.

Unfortunately, it is not backwards compatible.

What do you think?
 

Darzil

Developer
I think it sounds a very good idea.

I'm sure there will be some edge cases and oddities in scripts, but it makes more sense and should be easier to understand.
 

lostcalpolydude

Developer
Staff member
*** when we set lastAdventure, we update the Adventure Location in the GUI so you can see where you just went.
This is supposed to happen with the existing code. I don't understand how splitting things into two settings would make that start working.

- when you use set_location() or select something in the GUI, modifiers update immediately.
If the above functionality wasn't bugged, this would happen currently. Calling set_location() twice in a row with different locations in the same zone gets to the result that should be reached by calling it once.
 
What would my_location() return?

I'd presume it would return nextAdventure. That would preserve the (currently slightly wonky) functionality that using set_location() changes the value returned by my_location().

I don't think there's currently an ash function for returning the last adventure location. That would probably be a good addition.
 

Veracity

Developer
Staff member
Interesting. If scripts use that function, rather than looking directly at the setting, then yes, changing it to return nextAdventure means they will Just Work with this change.
 

Winterbay

Active member
I use my_location since
Code:
if(my_location() == $loccation[])
is a lot easier to remember (for me). and also slightly shorter, than
Code:
if(get_property("lastAdventure") == to_string($location[]))
.
 

roippi

Developer
Just piping up to say that AdventureSelectPanel should implement PreferenceListener and listen to (I think, if I understand the proposal correctly) nextAdventure.

Right now, it looks like we have to both set the preference and update the GUI in separate steps, which is prone to breaking, especially when hypothetical new features are added. Better if things Just Work with a unified interface when nextAdventure is changed.
 

Veracity

Developer
Staff member
I have this working pretty well. I'm going to check it in so we can shake out issues.

One issue is that the adventuring location only shows up the second time you visit there - but that is not new behavior. I have another open bug report that discusses that.
 
If I change nextAdventure (with the mafia GUI or with set_location) then refresh the charpane, it reverts to lastAdventure.

That doesn't seem like desired behaviour, as the charpane may be refreshed for all sorts of reasons after selecting your next adventure location (like casting buffs, eating).
 

Veracity

Developer
Staff member
I think you are right and revision 13145 will move lastAdventure to nextAdventure (hence refreshing the GUI) in two cases:

- We are doing a full session refresh (login, ascension, "Refresh Session" menu item)
- You adventure somewhere (GUI, script, Relay Browser) and we just set lastAdventure to that location.
 

Veracity

Developer
Staff member
I think this is working well. I had a bug (fixed today) in which it did not correctly set location to "None" and recalculate modifiers when you used an item which summons a monster (dolphin whistle, for example), but that's fixed.

I'm marking this Implemented.
 

Bale

Minion
I am somewhat impressed. A multitude of tiny issues have been cleared up by this refactoring.

Incidentally, ChIT now uses both my_location() and get_property("lastAdventure") in different places to suit different needs. (The gear changer is more interested in where you will be, but the rest of the script cares where you were.)
 

Crowther

Active member
I am somewhat impressed. A multitude of tiny issues have been cleared up by this refactoring.

Incidentally, ChIT now uses both my_location() and get_property("lastAdventure") in different places to suit different needs. (The gear changer is more interested in where you will be, but the rest of the script cares where you were.)
Agreed. This is what I needed to clean up the mess that is my flower planting script.
 
Top