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.

Sign up for GitHub

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

Jump to bottom

Core: Add item.filler helper #4081

Merged
merged 3 commits into from
Nov 29, 2024
Merged

Core: Add item.filler helper #4081

merged 3 commits into from
Nov 29, 2024

Conversation

Exempt-Medic
Copy link
Member

@Exempt-Medic Exempt-Medic commented Oct 22, 2024 •
edited
Loading

What is this fixing or adding?

Adds item.filler to go with item.advancement, item.useful, and item.trap

How was this tested?

Tested by modifying Metroid Prime's code to use it

@github-actions github-actions bot added affects: core Issues/PRs that touch core and may need additional validation. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Oct 22, 2024
@Exempt-Medic Exempt-Medic added the is: enhancement Issues requesting new features or pull requests implementing new features. label Oct 22, 2024
hesto2
hesto2 reviewed Oct 22, 2024
View reviewed changes
BaseClasses.py Outdated
@@ -1264,6 +1264,10 @@ def useful(self) -> bool:
def trap(self) -> bool:
return ItemClassification.trap in self.classification

@property
def filler(self) -> bool:
return not self.classification
Copy link

Choose a reason for hiding this comment

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

🙏

@NewSoupVi
Copy link
Member

NewSoupVi commented Oct 23, 2024 •
edited
Loading

I think this should be not (0b111 & self.classification) to leave the door open for filler_skip_balancing and to work with worlds that use their own custom flags

@Exempt-Medic
Copy link
Member Author

I think this should be not (0b111 & self.classification) to leave the door open for filler_skip_balancing and to work with worlds that use their own custom flags

So the idea is that world-added flags can't change the "filler" status because filler is defined as a lack of trap/useful/progression? Thoughts on just being explicit with not (self.trap or self.useful or self.advancement) ?

@NewSoupVi
Copy link
Member

I think this should be not (0b111 & self.classification) to leave the door open for filler_skip_balancing and to work with worlds that use their own custom flags

So the idea is that world-added flags can't change the "filler" status because filler is defined as a lack of trap/useful/progression? Thoughts on just being explicit with not (self.trap or self.useful or self.advancement) ?

I think I would say so. With the exception of trap ( :/ ), flags determine how core handles the item, and there is no way for a world to affect that with custom bits on the bitflag

hesto2
hesto2 approved these changes Oct 24, 2024
View reviewed changes
Copy link

@hesto2 hesto2 left a comment

Choose a reason for hiding this comment

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

Tested this in my apworld by pulling it in and verifying I could use it in my logic as expected (tests setup around this continued to pass, I verified they were not false positives)

        def is_hintable_filler_item(item: Item) -> bool:
            return item.filler and CivVIHintClassification.FILLER.value in self.options.pre_hint_items.value

nicholassaylor
nicholassaylor approved these changes Nov 27, 2024
View reviewed changes
Copy link
Contributor

@nicholassaylor nicholassaylor left a comment

Choose a reason for hiding this comment

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

LGTM, this is what I'd expect such a helper to do

@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: peer-review Issue/PR has not been reviewed by enough people yet. labels Nov 27, 2024
NewSoupVi
NewSoupVi approved these changes Nov 27, 2024
View reviewed changes
Copy link
Member

@NewSoupVi NewSoupVi left a comment

Choose a reason for hiding this comment

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

Marked for merge after hotfix window

How am I actually going to remember which PRs I've "marked for merge"? I didn't think this through

@Exempt-Medic Exempt-Medic added the waiting-on: other Issue/PR is waiting for something else, like another PR. label Nov 27, 2024
@Exempt-Medic
Copy link
Member Author

Marked for merge after hotfix window

How am I actually going to remember which PRs I've "marked for merge"? I didn't think this through

Marked for merge after hotfix window

How am I actually going to remember which PRs I've "marked for merge"? I didn't think this through

Add waiting on other tag

@NewSoupVi NewSoupVi merged commit c022c74 into ArchipelagoMW:main Nov 29, 2024
16 checks passed
@Exempt-Medic Exempt-Medic deleted the patch-1 branch November 30, 2024 04:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicholassaylor nicholassaylor nicholassaylor approved these changes

@NewSoupVi NewSoupVi NewSoupVi approved these changes

@hesto2 hesto2 hesto2 approved these changes

Assignees
No one assigned
Labels
affects: core Issues/PRs that touch core and may need additional validation. is: enhancement Issues requesting new features or pull requests implementing new features. waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. waiting-on: other Issue/PR is waiting for something else, like another PR.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Exempt-Medic @NewSoupVi @hesto2 @nicholassaylor