Feature - Implemented [Patch] Implement PirateRealm-only modifiers

zarqon

Well-known member
If your location is set to a specific PirateRealm island, the modifiers remain 0, since "PirateRealm" and "PirateRealm Island" are two different zones.

> ash set_location($location[piraterealm island])

Returned: void

> ash numeric_modifier($item[cursed compass], "Item Drop")

Returned: 100.0

> ash set_location($location[battle island])

Returned: void

> ash numeric_modifier($item[cursed compass], "Item Drop")

Returned: 0.0

We might have to do something like [100*(zone(PirateRealm)+zone(PirateRealm Island))]
 

fronobulax

Developer
Staff member
Possibly my bad since I do recall some changes concerning realm zones and they may have occurred after the patch was first submitted.

I'll gladly take a correction/fix but Zarqon has shown me the way if it's all me :) I'll revisit later today.
 

Saklad5

Member
If your location is set to a specific PirateRealm island, the modifiers remain 0, since "PirateRealm" and "PirateRealm Island" are two different zones.

That’s odd, since I had a very specific conversation with Veracity about this. She said that PirateRealm Island is nested within PirateRealm. If PirateRealm Island is within PirateRealm, and the specific islands are within PirateRealm Island, that should work. Clearly one of those things is incorrect. I recall testing it successfully before submitting the patch, but something may have changed. My memory, for instance.

At any rate, thanks for catching it.
 
Last edited:

zarqon

Well-known member
Zones can totally nest. But the above behavior seems to indicate that the zone() function is non-recursive; it simply checks for a match against the location's immediate parent zone, without going any further up or down the tree.

Changing instances of zone(PirateRealm) to zone(PirateRealm)+zone(PirateRealm Island) ought to yield the desired behavior.

And thank you for the patch Saklad!
 

Saklad5

Member
OK, I’ve looked at the code in Expression.java that actually evaluates the modifiers, and it looks like it is just doing a case-insensitive match on the zone. I could submit a patch that fixes that, since it is supposed to take nested zones into account.

There’s a few ways I could implement this, and given the code in question it is probably worth optimizing carefully.

I’m thinking a bottom-up search would be best, where we check whether the current zone matches the expression, then whether the parent of the current zone matches, then whether the parent ofthat zone matches, until we either get a match or hit a top-level zone.

Alternatively, we could do a breadth-first top-down search, where we check whether the zone in the expression matches the current zone, then whether any of the children of the zone in the expression match the current zone, then whether any of the children of those zones match, until we get a match or exhaust the possibilities. Since most zones don’t match most modifiers, this seems like it would take more time.
 
Last edited:

Saklad5

Member
Zones can totally nest. But the above behavior seems to indicate that the zone() function is non-recursive; it simply checks for a match against the location's immediate parent zone, without going any further up or down the tree.

Changing instances of zone(PirateRealm) to zone(PirateRealm)+zone(PirateRealm Island) ought to yield the desired behavior.

And thank you for the patch Saklad!
I’d rather just make the zone() function recursive, as I said as you posted that. Existing modifiers for zones like Hobopolis and Spookyraven Manor could benefit from that, not to mention future maintainers. Heck, given the current approach of adding up redundant zone expressions, it might actually yield a small performance increase.

And again, thanks for catching the issue.
 
Last edited:

Veracity

Developer
Staff member
I’d rather just make the zone() function recursive, as I said just now. Existing modifiers for zones like Hobopolis and Spookyraven Manor could benefit from that, not to mention future maintainers. Heck, given the current approach of adding up redundant zone expressions, it might actually yield a small performance increase.
That's fine. If you are on Battle Island, you are in PirateRealm Island and are also in PirateRealm.
I'm all for adjusting modifier expressions that currently use multiple subzones to use the super zone instead. There are only a few, actually:

Code:
Item	doll-eye amulet	Single Equip, Item Drop: [40*(zone(Manor0)+zone(Manor1)+zone(Manor2)+zone(Manor3))]
Item	stinky fannypack	Stench Damage: +25, Stench Spell Damage: +25, Item Drop: [40*(zone(Spring Break Beach)+zone(Conspiracy Island)+zone(Dinseylandfill)+zone(That 70s Volcano)+zone(The Glaciest))]
Effect	Expert Vacationer	Experience Percent (Muscle): [25*(zone(Spring Break Beach)+zone(Conspiracy Island)+zone(Dinseylandfill)+zone(That 70s Volcano)+zone(The Glaciest))], Experience Percent (Mysticality): [25*(zone(Spring Break Beach)+zone(Conspiracy Island)+zone(Dinseylandfill)+zone(That 70s Volcano)+zone(The Glaciest))], Experience Percent (Moxie): [25*(zone(Spring Break Beach)+zone(Conspiracy Island)+zone(Dinseylandfill)+zone(That 70s Volcano)+zone(The Glaciest))]
Those could use zone(Manor) and zone(Elemental International Airport).
 

Saklad5

Member
OK, it’s going into an infinite loop. It looks like the parent zone of a top-level zone is itself? I’m probably just misunderstanding the output of
Code:
AdventureDatabase.PARENT_ZONES.get( currentZone );
For now, I’m going to assume that’s intended behavior and adjust the loop accordingly.
 

Saklad5

Member
A lot of modifiers appear to be incorrect, actually. They’re using “zone” instead of “location”, which is breaking now.
Code:
Item    slime-covered shovel    Weapon Damage: [100*zone(The Slime Tube)+60], Initiative: -10
This shouldn’t be difficult to fix as part of the patch, though. Whoever made KoLmafia capable of surviving NullPointerExceptions, thank you.
 
Last edited:

Saklad5

Member
OK, it’s going into an infinite loop. It looks like the parent zone of a top-level zone is itself? I’m probably just misunderstanding the output of
Code:
AdventureDatabase.PARENT_ZONES.get( currentZone );
For now, I’m going to assume that’s intended behavior and adjust the loop accordingly.
It seems like this isn’t always true. For now, I’m going to check for both the parent being null and the parent being the child.
 

Saklad5

Member
It turns out this is somewhat difficult to test during a TCRS run. If anyone wants to test the patch now and see if it fixes the issue, be my guest.
View attachment Recursive Zones.patch
Just to be clear, I have not been able to check if this resolves the issue. Don’t merge it without testing it yourself. When I’m finished with my current run, I can see if it works myself.
 

fronobulax

Developer
Staff member
It turns out this is somewhat difficult to test during a TCRS run. If anyone wants to test the patch now and see if it fixes the issue, be my guest.
View attachment 9515
Just to be clear, I have not been able to check if this resolves the issue. Don’t merge it without testing it yourself. When I’m finished with my current run, I can see if it works myself.

Since I am tracking this, does r19341 mean the patch has been checked and verified? I'm going to assume Yes but this is an opportunity for someone to tell me I am wrong :)
 

Saklad5

Member
Since I am tracking this, does r19341 mean the patch has been checked and verified? I'm going to assume Yes but this is an opportunity for someone to tell me I am wrong :)

I messaged Veracity that it was working, yes. This thread was technically about a different issue, so it was a bit off-topic in the first place.
 

fronobulax

Developer
Staff member
I messaged Veracity that it was working, yes. This thread was technically about a different issue, so it was a bit off-topic in the first place.

I got the off topic :) At a real high level, I committed something, it did not work as expected and my sense of professionalism requires me to revert it or confirm that something else changed so that it did work as expected. I just wanted to make sure I could cross revert r19336 off of my list of things I might have to do.
 
Top