Skip to content

Conversation

@nicopop
Copy link
Contributor

@nicopop nicopop commented Nov 18, 2024

I told myself I should stop making PR past midnight but here's one.

this is #101 but the diff work this time

og. lowercase commit was split into #103
includes multiple small fixes to ItemValue functionality:
b21f576 Renamed world.item_values_cache to world.itemvalue_rule_cache to stop potential confusion with world.item_values
88854ef Split/changed the force reset parameter of the cache of get_items_with_value to be a skip instead. Mainly so it work more like the ItemValue rule function. I made some helper.py functions to replace the reset usage
5150bbc Made both datavalidation of ItemValue (prefill and assert) only check for region with locations in it and their parents using some recursion tech

to explain the how/why of 5150bbc

my fix for https://discord.com/channels/1097532591650910289/1307802587839594617

WHY:

Hooks and Options can remove locations.
I could easily add a check to only test for region with locations but its still possible for a region without to be logically required since parent region can be empty.

HOW:

it work basicaly the same for the PreFill test and the Assert one
the only difference is the way to find the parents

  • Assert: Since we only have the basic json, I build a dict(child,list(Parent)) that way I can do the recursion below Removed
  • Prefill: I could probably also use the json but I decided to use the entrances since multiworld did it for me anyway

in both case:

  1. it checks every region for locations, and if present, add the region to a list
  2. since a region can be empty but still be a parent of any of the region on the "has Loc" list, Check recursively the parents and add them to the list.
    2.1 add the parents to an "already checked" list so it's skipped in future recursion
  3. once all the used region are identified check their requires, entrance_require and exit require for ItemValue (if the region on the otherside is used of cource).
  4. check every location of the region for ItemValues requirement
  5. Finally check if we have the items to fullfill those requirements

@nicopop nicopop self-assigned this Nov 18, 2024
@nicopop nicopop mentioned this pull request Nov 18, 2024
@nicopop
Copy link
Contributor Author

nicopop commented Nov 21, 2024

I was waiting for some ItemValue bug report before undrafting this PR but it seems that report wont come so ill mark this as ready

@nicopop nicopop marked this pull request as ready for review November 21, 2024 17:17
@nicopop nicopop requested a review from FuzzyGamesOn November 23, 2024 21:26
Copy link
Collaborator

@FuzzyGamesOn FuzzyGamesOn left a comment

Choose a reason for hiding this comment

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

Minor typo, everything else seems good

FuzzyGamesOn
FuzzyGamesOn previously approved these changes Nov 24, 2024
@nicopop nicopop requested a review from silasary December 2, 2024 00:00
silasary
silasary previously approved these changes Dec 2, 2024
FuzzyGamesOn
FuzzyGamesOn previously approved these changes Dec 4, 2024
Copy link
Collaborator

@FuzzyGamesOn FuzzyGamesOn left a comment

Choose a reason for hiding this comment

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

Can't believe nico dismissed my previous review, smh my head

@FuzzyGamesOn FuzzyGamesOn removed the request for review from axxroytovu December 4, 2024 22:48
@silasary silasary dismissed stale reviews from FuzzyGamesOn and themself via 42ea278 December 5, 2024 10:17
Copy link
Collaborator

@FuzzyGamesOn FuzzyGamesOn left a comment

Choose a reason for hiding this comment

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

@silasary silasary merged commit 996d295 into main Dec 5, 2024
@silasary silasary deleted the ItemValue-Fixes branch December 5, 2024 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants