Skip to content

Conversation

@MatthewMarinets
Copy link
Contributor

What is this fixing or adding?

unexcluded_items wouldn't affect items excluded by vanilla_items_only like can be done with overpowered items or unreleased units.

How was this tested?

Added a new unit test. Existing unit tests ensure that default vanilla_items_only behaviour remains unchanged.

If this makes graphical changes, please attach screenshots.

None.

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Oct 6, 2025
Copy link
Collaborator

@Ziktofel Ziktofel left a comment

Choose a reason for hiding this comment

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

Check the Regen biosteel count if the vanilla items only is true. Shall be 1 if vanilla only
As I think that this check:

if item_name in item_groups.terran_original_progressive_upgrades:
                auto_excludes[item_name] = max(item_data.quantity - 1, auto_excludes.get(item_name, 0))

Can allow 2 stacks

Either way expected results:
OP items either state + vanilla only true -> 1
OP items excluded + vanilla only false -> 2
OP items included + vanilla only false -> 3

I'd like to see a unit test for that

@Ziktofel Ziktofel added is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. waiting-on: author Issue/PR is waiting for feedback or changes from its author. labels Oct 6, 2025
@MatthewMarinets
Copy link
Contributor Author

Check the Regen biosteel count if the vanilla items only is true. Shall be 1 if vanilla only As I think that this check:

if item_name in item_groups.terran_original_progressive_upgrades:
                auto_excludes[item_name] = max(item_data.quantity - 1, auto_excludes.get(item_name, 0))

Can allow 2 stacks

Either way expected results: OP items either state + vanilla only true -> 1 OP items excluded + vanilla only false -> 2 OP items included + vanilla only false -> 3

I'd like to see a unit test for that

The unit test already exists, it's test_generation.test_vanilla_items_only_excludes_terran_progressives(). It checks that all terran progressives in terran_progressive_items appear at most once if vanilla_items_only is true.

The code is correct. Remember auto_excludes is an excluded amount, so setting it to quantity - 1 means all but 1 is excluded, or at most 1 is included. The max means that if OP items auto-excludes something, then vanilla items can exclude more but won't add to it.

I guess I can add a unit test to cover the situation of OP items excluded and vanilla items only together still allowing one level of regen biosteel together. Seems too niche to be worth testing in my personal opinion, but if it speeds along approval I'm game.

@Ziktofel Ziktofel 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: author Issue/PR is waiting for feedback or changes from its author. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Oct 10, 2025
@Ziktofel
Copy link
Collaborator

As this is a main PR, I prefer rather to double check in order to avoid introducing any new bugs

@Snarkie
Copy link
Contributor

Snarkie commented Oct 12, 2025

Did a manual test on my end:

  • confirmed the issue without this PR, unexcluded items does not affect vanilla_items_only restriction
  • generated a local campaign with vanilla_items_only: true and unexcluded a non-vanilla item
  • checked the spoiler log, the unexcluded item properly gets included with this PR
  • checked ingame if the item is placed and rewarded properly

Also tested the mentioned corner case with Regen Biosteel

Looks good to me.

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.

3 participants