Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OSRS: Add missing indirect conditions #4029

Merged

Conversation

Mysteryem
Copy link
Contributor

What is this fixing or adding?

All entrances to Cooks_Guild and Crafting_Guild and all entrances using special logic for canoes were missing indirect conditions for the regions that the cooking, crafting and woodcutting skill rules require access to.

How was this tested?

Ran the spheres test in #3924 multiple times and ran many generations with my own implementation of a missing indirect condition checker (prints a stacktrace on a recursive can_reach(region) call if the indirect condition is missing).

All entrances to Cooks_Guild and Crafting_Guild and all entrances using
special logic for canoes were missing indirect conditions for the
regions that the cooking, crafting and woodcutting skill rules require
access to.
@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Oct 4, 2024
@Mysteryem
Copy link
Contributor Author

@digiholic

The alternative would be removing all the existing registrations of indirect conditions and setting explicit_indirect_conditions = False on the world class.

@Exempt-Medic Exempt-Medic added is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world. labels Oct 4, 2024
Copy link
Member

@Exempt-Medic Exempt-Medic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM and my own checking for missing indirect conditions didn't show any more

@Exempt-Medic Exempt-Medic removed the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Oct 4, 2024
Copy link
Collaborator

@digiholic digiholic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Man I can never fully wrap my head around these indirect connections, I always feel like I miss something. I didn't think of all of these training regions being indirect connections! Thanks for sorting all of these out.

@Exempt-Medic Exempt-Medic added waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. and removed waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world. labels Oct 23, 2024
@NewSoupVi
Copy link
Member

Really wonder if this specific implementation might not have so many indirect conditions at this point that explicit_indirect_conditions: False might just be faster. It's worth a shot I think. Anyway, merging this for now

@NewSoupVi NewSoupVi merged commit edacc07 into ArchipelagoMW:main Oct 28, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants