This patch implements modifiers for items that only affect PirateRealm, such as the bloody harpoon. It does not add any new modifiers, however.
View attachment PirateRealm Properties.patch
View attachment PirateRealm Properties.patch
Feature - Implemented [Patch] Implement PirateRealm-only modifiers
- Thread starter Saklad5
- Start date
r19336
Untested
Untested
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.
I'll gladly take a correction/fix but Zarqon has shown me the way if it's all me
I'll revisit later today.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!
Changing instances of zone(PirateRealm) to zone(PirateRealm)+zone(PirateRealm Island) ought to yield the desired behavior.
And thank you for the patch Saklad!
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.
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:
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:
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))]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
For now, I’m going to assume that’s intended behavior and adjust the loop accordingly.
Code:
AdventureDatabase.PARENT_ZONES.get( currentZone );A lot of modifiers appear to be incorrect, actually. They’re using “zone” instead of “location”, which is breaking now.
This shouldn’t be difficult to fix as part of the patch, though. Whoever made KoLmafia capable of surviving NullPointerExceptions, thank you.
Code:
Item slime-covered shovel Weapon Damage: [100*zone(The Slime Tube)+60], Initiative: -10
Last edited:
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.
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.
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.
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
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.
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.