OSRS: Fixes to Logic errors related to Max Skill Level determining when Regions are accessible #4188

digiholic · 2024-11-14T19:02:06Z

What is this fixing or adding?

Addresses issues found in RC testing - Regions that required a skill level to access (ex. Crafting Guild, Cooking Guild, and Canoe tasks) were not properly checking to make sure their requirements were within the Max Skill Level set in the options. This would mean that a player might need to reach 32 Cooking to access an Apple from the Cooking Guild for a Level 30 Cooking Task, even if 30 were the chosen max Cooking Level required.

Additionally, some general cleanup to make rules logic easier to work with, and to fix a few edge cases:

Finally switched to explicit_indirect_conditions = False. If OSRS doesn't qualify for it, then nothing does
Moved skill and region access logic into a separate file, and broke apart the skill checks into individual methods, rather than one massive hydra of a method
Skill Rules now all check for the Max Skill Level set in the options before doing anything else. Why bother checking if you have access to a reasonable training method for a level you shouldn't be training to anyway?

How was this tested?

The easiest situation to force is the Canoe logic. By setting max woodcutting level to 1, we essentially remove the ability to use Canoes. 10 seeds were rolled, and all spoiler logs were checked for any logical paths that took any Canoe route, and none were found. 10 more seeds with a Woodcutting Level of 99 were rolled, and all of them contained at least one Canoe route.

Things like Crafting Guild and Cooking Guild access are more complicated to test since they require more than just a skill level to access, so those will need to be tested with actual runs.

Automated tests were also run multiple times to ensure the Indirect Condition removal did not cause any failures to generate. Indirect Condition related failures were quite rare though, so I cannot be 100% sure they're gone.

…it unconditionally

…whole time

…the skill training rules now

…idge to south varrock without farms. This has been fixed

worlds/osrs/Rules.py

worlds/osrs/__init__.py

Co-authored-by: Exempt-Medic <[email protected]>

NewSoupVi · 2024-11-18T01:25:38Z

worlds/osrs/__init__.py

-                    entrance.access_rule = lambda state, item_name=item_name: state.has(item_name, self.player)
-                    continue
-
+                entrance.access_rule = lambda state, item_name=item_name.replace("*",""): state.has(item_name.replace("*",""), self.player)


Wondering why this wasn't changed to set_rule or add_rule as well when everything else was

(Ftr: You can add_rule when there is no rule yet afaik)

Well, that's the only issue I found, and it's not actually an issue, so I'll just merge this after doing some test gens so we can make RC3 as soon as possible

If you want to change it, it can be a followup PR ofc

Exempt-Medic · 2024-11-18T01:33:42Z

worlds/osrs/Rules.py

+        if skill.lower() == "fishing":
+            return get_fishing_skill_rule(level, player, options)
+        if skill.lower() == "mining":
+            return get_mining_skill_rule(level, player, options)
+        if skill.lower() == "woodcutting":
+            return get_woodcutting_skill_rule(level, player, options)
+        if skill.lower() == "smithing":
+            return get_smithing_skill_rule(level, player, options)
+        if skill.lower() == "crafting":
+            return get_crafting_skill_rule(level, player, options)
+        if skill.lower() == "cooking":
+            return get_cooking_skill_rule(level, player, options)
+        if skill.lower() == "runecraft":
+            return get_runecraft_skill_rule(level, player, options)
+        if skill.lower() == "magic":
+            return get_magic_skill_rule(level, player, options)
+        if skill.lower() == "firemaking":
+            return get_firemaking_skill_rule(level, player, options)
+
+        return lambda state: True


Spacing is off

Suggested change

if skill.lower() == "fishing":

return get_fishing_skill_rule(level, player, options)

if skill.lower() == "mining":

return get_mining_skill_rule(level, player, options)

if skill.lower() == "woodcutting":

return get_woodcutting_skill_rule(level, player, options)

if skill.lower() == "smithing":

return get_smithing_skill_rule(level, player, options)

if skill.lower() == "crafting":

return get_crafting_skill_rule(level, player, options)

if skill.lower() == "cooking":

return get_cooking_skill_rule(level, player, options)

if skill.lower() == "runecraft":

return get_runecraft_skill_rule(level, player, options)

if skill.lower() == "magic":

return get_magic_skill_rule(level, player, options)

if skill.lower() == "firemaking":

return get_firemaking_skill_rule(level, player, options)

return lambda state: True

if skill.lower() == "fishing":

return get_fishing_skill_rule(level, player, options)

if skill.lower() == "mining":

return get_mining_skill_rule(level, player, options)

if skill.lower() == "woodcutting":

return get_woodcutting_skill_rule(level, player, options)

if skill.lower() == "smithing":

return get_smithing_skill_rule(level, player, options)

if skill.lower() == "crafting":

return get_crafting_skill_rule(level, player, options)

if skill.lower() == "cooking":

return get_cooking_skill_rule(level, player, options)

if skill.lower() == "runecraft":

return get_runecraft_skill_rule(level, player, options)

if skill.lower() == "magic":

return get_magic_skill_rule(level, player, options)

if skill.lower() == "firemaking":

return get_firemaking_skill_rule(level, player, options)

return lambda state: True

Exempt-Medic · 2024-11-18T01:34:42Z

worlds/osrs/Rules.py

+
+        if region_row.name == RegionNames.Lumbridge:
+            # Canoe Tree access for the Location
+            if outbound_region_name == RegionNames.Canoe_Tree:


I believe from here on you can start using if/elif again instead of if/if. I commented this earlier but I think it was missed

NewSoupVi · 2024-11-18T01:46:10Z

Getting loads of errors on a fully random yaml - Idk if this was always the case because random settings don't work well with this game because they end up being unsolvable / restrictive? I don't feel like it was ever this much though, like every 3rd or 4th fully random seed fails. Well, here are some seeds

Ftr, these hit Fill errors, not one of your designated Exceptions (I hit that too a couple of times, but that's fine)

Old School Runescape.json
59826788302066781595
77746130863581927747
89898869177983739745
68734120406112450527

…multiple skills how actually check all skills

…ipelago into osrs-entrance-fix

Exempt-Medic · 2024-11-19T01:38:26Z

Mentioned it in DM's but just putting it here as well: Cook Some Stew has accessibility problems from Fishing

NewSoupVi · 2024-11-19T20:36:51Z

worlds/osrs/__init__.py

+    def task_within_skill_levels(self, skills_required):
+        # Loop through each required skill. If any of its requirements are out of the defined limit, return false
+        for skill in skills_required:
+            max_level_for_skill = getattr(self.options, f"max_{skill.skill.lower()}_level")
+            if skill.level > max_level_for_skill:
+                return False
+        return True


You don't actually need to do this, I'm doing this purely as an exercise for myself lol

So this could be...

return all( skill.level <= getattr(self.options, f"max_{skill.skill.lower()}_level") for skill in skills_required )

Hey that looks kinda nice, maybe you should do this instead LOL nah only if you want to

NewSoupVi · 2024-11-19T20:42:56Z

worlds/osrs/__init__.py

+    def pre_fill(self) -> None:
+        all_state = self.multiworld.get_all_state(False)
+        for location in self.multiworld.get_locations(self.player):
+            # if not location.can_reach(all_state):
+            #     options = self.options
+            #     print(f"{location} can't be reached even with every progression item.\n{all_state.prog_items[self.player]}")
+            assert location.can_reach(
+                all_state), f"{location} can't be reached even with every progression item.\n{all_state.prog_items[self.player]}"
+


This is a nice debugging tool, but I don't actually think it should be in main :D

NewSoupVi · 2024-11-20T00:22:19Z

95140043061228385161
12545918943402021278
players.zip

Fill.FillError: Not enough locations for progression items. There are 1 more progression items than there are available locations.

Very rare, but this seems very fixable

digiholic · 2024-11-21T23:30:57Z

95140043061228385161 12545918943402021278 players.zip

Fill.FillError: Not enough locations for progression items. There are 1 more progression items than there are available locations.

Very rare, but this seems very fixable

Still trying to work this out. As far as I can tell, the seed it builds is legitimately unbeatable. I just mapped out the items and locations available and it legitimately cannot place that last item anywhere it can actually be reached at. I think I'll need to find a way to check for this during Task generation and swap out tasks if it fails.

Exempt-Medic · 2024-11-21T23:59:56Z

Still trying to work this out. As far as I can tell, the seed it builds is legitimately unbeatable. I just mapped out the items and locations available and it legitimately cannot place that last item anywhere it can actually be reached at. I think I'll need to find a way to check for this during Task generation and swap out tasks if it fails.

I'm confused/concerned that it seems to only happen with multiple OSRS players. I did three thousand solo gens and didn't hit this at all, but hit it several times in just 100 2-player gens

NewSoupVi · 2024-11-22T01:53:52Z

There's something strange going on here. I need to do some investigation.

NewSoupVi · 2024-11-22T02:21:46Z

This is not your fault. It's a bug with Fill.
Even if there are no locations left to place an item, Fill enters a mode called "swap" to try to swap items around to make a valid placement.
It actually succeeds at this, but then chokes on the last meters for no reason.

NewSoupVi · 2024-11-22T03:22:56Z

There is still an unaddressed comment, otherwise I am good to merge this

…en Regions are accessible (ArchipelagoMW#4188) * Removes explicit indirect conditions * Changes special rules function add rule instead of setting, and call it unconditionally * Fixes issues in rule generation that have been around but unused the whole time * Finally moves rules out into a separate file. Fixes level-related logic * Removes redundant max skill level checks on canoes, since they're in the skill training rules now * For some reason, canoe logic assumed you could always walk from lumbridge to south varrock without farms. This has been fixed * Apply suggestions from code review Co-authored-by: Exempt-Medic <[email protected]> * Quests now respect skill limits and can be excluded. Tasks that take multiple skills how actually check all skills * Adds alternative route for cooking that doesn't require fishing * Remove debug code --------- Co-authored-by: Exempt-Medic <[email protected]> Co-authored-by: NewSoupVi <[email protected]>

digiholic added 6 commits November 6, 2024 11:12

Removes explicit indirect conditions

d212e7b

Changes special rules function add rule instead of setting, and call …

53b7f76

…it unconditionally

Fixes issues in rule generation that have been around but unused the …

c80d127

…whole time

Finally moves rules out into a separate file. Fixes level-related logic

c1f7fb1

Removes redundant max skill level checks on canoes, since they're in …

6516c75

…the skill training rules now

For some reason, canoe logic assumed you could always walk from lumbr…

028fcad

…idge to south varrock without farms. This has been fixed

github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Nov 14, 2024

Exempt-Medic reviewed Nov 14, 2024

View reviewed changes

worlds/osrs/Rules.py Outdated Show resolved Hide resolved

Exempt-Medic reviewed Nov 14, 2024

View reviewed changes

worlds/osrs/__init__.py Outdated Show resolved Hide resolved

Exempt-Medic added is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. is: enhancement Issues requesting new features or pull requests implementing new features. is: refactor/cleanup Improvements to code/output readability or organizization. labels Nov 14, 2024

Apply suggestions from code review

e4bb4b4

Co-authored-by: Exempt-Medic <[email protected]>

NewSoupVi added the affects: release/blocker Issues/PRs that must be addressed before next official release. label Nov 18, 2024

NewSoupVi reviewed Nov 18, 2024

View reviewed changes

Exempt-Medic reviewed Nov 18, 2024

View reviewed changes

digiholic added 2 commits November 18, 2024 17:59

Quests now respect skill limits and can be excluded. Tasks that take …

0ccf224

…multiple skills how actually check all skills

Merge branch 'osrs-entrance-fix' of https://github.com/digiholic/Arch…

9220560

…ipelago into osrs-entrance-fix

Adds alternative route for cooking that doesn't require fishing

01f9648

NewSoupVi reviewed Nov 19, 2024

View reviewed changes

NewSoupVi requested changes Nov 19, 2024

View reviewed changes

ThePhar changed the title ~~[OSRS] Fixes to Logic errors related to Max Skill Level determining when Regions are accessible~~ OSRS: Fixes to Logic errors related to Max Skill Level determining when Regions are accessible Nov 21, 2024

NewSoupVi mentioned this pull request Nov 22, 2024

Fill chokes on itself on the last meters in games with both minimal players and full players #4224

Open

Remove debug code

1502000

NewSoupVi approved these changes Nov 22, 2024

View reviewed changes

NewSoupVi merged commit 2424b79 into ArchipelagoMW:main Nov 22, 2024
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OSRS: Fixes to Logic errors related to Max Skill Level determining when Regions are accessible #4188

OSRS: Fixes to Logic errors related to Max Skill Level determining when Regions are accessible #4188

digiholic commented Nov 14, 2024

NewSoupVi Nov 18, 2024 •

edited

Loading

NewSoupVi Nov 18, 2024 •

edited

Loading

Exempt-Medic Nov 18, 2024

Exempt-Medic Nov 18, 2024 •

edited

Loading

NewSoupVi commented Nov 18, 2024 •

edited

Loading

Exempt-Medic commented Nov 19, 2024

NewSoupVi Nov 19, 2024 •

edited

Loading

NewSoupVi Nov 19, 2024

NewSoupVi commented Nov 20, 2024 •

edited

Loading

digiholic commented Nov 21, 2024

Exempt-Medic commented Nov 21, 2024 •

edited

Loading

NewSoupVi commented Nov 22, 2024

NewSoupVi commented Nov 22, 2024 •

edited

Loading

NewSoupVi commented Nov 22, 2024

OSRS: Fixes to Logic errors related to Max Skill Level determining when Regions are accessible #4188

OSRS: Fixes to Logic errors related to Max Skill Level determining when Regions are accessible #4188

Conversation

digiholic commented Nov 14, 2024

What is this fixing or adding?

How was this tested?

NewSoupVi Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

NewSoupVi Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Exempt-Medic Nov 18, 2024

Choose a reason for hiding this comment

Exempt-Medic Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

NewSoupVi commented Nov 18, 2024 • edited Loading

Exempt-Medic commented Nov 19, 2024

NewSoupVi Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

NewSoupVi Nov 19, 2024

Choose a reason for hiding this comment

NewSoupVi commented Nov 20, 2024 • edited Loading

digiholic commented Nov 21, 2024

Exempt-Medic commented Nov 21, 2024 • edited Loading

NewSoupVi commented Nov 22, 2024

NewSoupVi commented Nov 22, 2024 • edited Loading

NewSoupVi commented Nov 22, 2024

NewSoupVi Nov 18, 2024 •

edited

Loading

NewSoupVi Nov 18, 2024 •

edited

Loading

Exempt-Medic Nov 18, 2024 •

edited

Loading

NewSoupVi commented Nov 18, 2024 •

edited

Loading

NewSoupVi Nov 19, 2024 •

edited

Loading

NewSoupVi commented Nov 20, 2024 •

edited

Loading

Exempt-Medic commented Nov 21, 2024 •

edited

Loading

NewSoupVi commented Nov 22, 2024 •

edited

Loading