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

HK: fix grubhunt required grubs count #4094

Merged
merged 6 commits into from
Oct 30, 2024

Conversation

qwint
Copy link
Contributor

@qwint qwint commented Oct 25, 2024

What is this fixing or adding?

somehow this mixup got into the final grubhunt PR

the original code that somehow got mixed up in the final grubhunt PR (in a per player loop, which is why we need the loop in the new code)

if grub.location and grub.location.player in relevant_groups:
    # not counting our grubs stuck in item links because we also will count the group's copy
    pass
else:
    world.grub_count += 1

How was this tested?

gen'd with a breakpoint to confirm world.grub_count with 4 grubhunt: all players using both replacement grubs and null, and confirmed that the number gets honored when not using all still

If this makes graphical changes, please attach screenshots.

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Oct 25, 2024
@ScipioWright ScipioWright added is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. affects: PR/blocker Issue/PR blocks another PR. labels Oct 25, 2024
@Exempt-Medic
Copy link
Collaborator

Given how complicated this seems to be, can you add unit tests for it?

@NewSoupVi NewSoupVi added affects: release/blocker Issues/PRs that must be addressed before next official release. and removed affects: PR/blocker Issue/PR blocks another PR. labels Oct 25, 2024
worlds/hk/__init__.py Outdated Show resolved Hide resolved
"link_replacement": True,
"replacement_item": "Grub",
}]
expected_grubs = 46 + 23
Copy link
Collaborator

Choose a reason for hiding this comment

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

I asked in DMs, but I should put this here as well:
Why is the expected grub count 69 when there are 92 Grubs in the pool and playthrough also expects players to get 92 Grubs?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Apparently only one of the players is linking the replacement Grub. I hate itemlinks

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. There are always 1 fewer replacement items, per linked item, than the number of items being linked together.

If there were 6 players in an item link, linking an item between them would become 1 linked item + 5 replacements.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, what I said is also apparently completely wrong. The link_relacement is for the group, the replacement item is per player

Copy link
Collaborator

@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.

Tested some configurations of players and saw that the proper values were found. The unit tests pass and the code makes sense after I, regretfully, learned a lot more about itemlinks

Copy link
Member

@black-sliver black-sliver left a comment

Choose a reason for hiding this comment

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

I trust medic

@black-sliver black-sliver merged commit 0b5c7fe into ArchipelagoMW:main Oct 30, 2024
18 checks passed
@qwint qwint deleted the hk_fix_grubhunt branch October 30, 2024 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: release/blocker Issues/PRs that must be addressed before next official release. is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants