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

Custom ice traps #1982

Closed
wants to merge 12 commits into from
Closed

Conversation

rrealmuto
Copy link

Adds setting to select a custom amount of ice traps. When pots/crates/etc. are enabled, there is a pretty big jump between Ice Trap "Mayhem" and "Onslaught" because all of the junk items from pots don't get included under Mayhem. This setting allows more fine-grained control over how many ice traps are in the seed.

Two additional items are available under the "Ice Traps" drop down:

  • Custom (count): Specify an exact amount of "Junk" items to be converted to Ice Traps
  • Custom (%): Specify a percentage of the "Junk" items to be converted to Ice Traps.

In both of these cases, "Junk" items includes base pool junk.

@fenhl fenhl added Component: Setting specific to setting(s) Type: Enhancement New feature or request labels May 20, 2023
@cjohnson57
Copy link
Collaborator

Requesting to resolve merge conflicts/update to the new typing reqs, this PR is fairly small so hopefully shouldn't be too bad

@cjohnson57 cjohnson57 added the Status: Waiting for Author Changes or response requested label Jun 17, 2023
@rrealmuto
Copy link
Author

Still need to make sure this actually works but I rebased it.

@rrealmuto
Copy link
Author

It seems to be working now. Found a problem where it wasn't properly working because of pending junk but I think that's fixed now.

@cjohnson57
Copy link
Collaborator

A couple things:

The count option goes up to 4 digits, but it looks like the box only displays up to 3. You'll probably have to dig into the forbidden CSS files for this
image

Just to confirm, I noticed that if I set the count way too high, instead of erroring it simply gives the same amount of ice traps as if I set it to 100%, this is intended behavior? (Ftr I think it's good)

@fenhl fenhl added Status: Needs Review Someone should be looking at it Status: Needs Testing Probably should be tested and removed Status: Waiting for Author Changes or response requested labels Feb 15, 2024
@fenhl fenhl removed the Status: Needs Testing Probably should be tested label Mar 25, 2024
Copy link
Collaborator

@fenhl fenhl left a comment

Choose a reason for hiding this comment

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

Got some code style nits, this seems good other than that.

ItemPool.py Outdated Show resolved Hide resolved
ItemPool.py Outdated Show resolved Hide resolved
ItemPool.py Outdated Show resolved Hide resolved
ItemPool.py Outdated Show resolved Hide resolved
ItemPool.py Outdated Show resolved Hide resolved
SettingsList.py Outdated Show resolved Hide resolved
SettingsList.py Outdated Show resolved Hide resolved
@fenhl fenhl added the Status: Waiting for Author Changes or response requested label Sep 18, 2024
@fenhl fenhl added Status: Waiting for Release This PR is ready for merge, but we're holding off on it until after the next release and removed Status: Waiting for Author Changes or response requested Status: Needs Review Someone should be looking at it labels Sep 20, 2024
@fenhl fenhl removed the Status: Waiting for Release This PR is ready for merge, but we're holding off on it until after the next release label Sep 30, 2024
@fenhl fenhl added this to the next milestone Sep 30, 2024
fenhl added a commit that referenced this pull request Sep 30, 2024
@fenhl
Copy link
Collaborator

fenhl commented Sep 30, 2024

I must have done something wrong so GitHub didn't auto-close this PR, but this was merged in a75ab38.

@fenhl fenhl closed this Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Setting specific to setting(s) Type: Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants