taw / magic-search-engine

Search engine for Magic cards
MIT License
45 stars 20 forks source link

Fix boosters broken by new color balance #228

Closed axxroytovu closed 1 year ago

axxroytovu commented 1 year ago

Because of new color-balancing, color balanced sheets cannot be used as a subsheet inside "any/rate". "any/chance" works fine.

axxroytovu commented 1 year ago

I wonder if there's a more engineered way to do this, since balanced sheets inside another sheet don't make any sense.

There should be a way inside of the sheet factory to prevent sub sheets from being color balanced.

taw commented 1 year ago

If that's causing the problem, then we should fix it, as including common in any should just ignore balanced flag and treat it the same as including query: "r:c". I'll take a look at it when I have some free time.

axxroytovu commented 1 year ago

That change probably worked but I can't test it right now. I'll try in a few hours.

axxroytovu commented 1 year ago

Verified that the above fix does resolve the same issue if it would come up in future boosters. It's still best practice to avoid mixing use and any-rate because it can behave unintuitively. Better practice is to use either query instead of use, or any-chance instead of any-rate when possible.

taw commented 1 year ago

It should just work now fine either way, so this shouldn't be needed.

axxroytovu commented 1 year ago

That best practice is less about balanced sheets and more about how rate interacts with sheets that have subsheets. Rate scales with the number of objects contained within a sheet, whether those are cards or subsheets. So it can be unintuitive if you are only expecting it to scale on the number of cards. For example, I would expect this to be a reasonable way to define the foil sheet:

  any:
  - use: common_with_showcase
    rate: 1
  - use: basic
    rate: 3

Because my intuition is that this would "scale" each card in common_with_showcase by 1, and scale each card in basic by 3, resulting in the correct ratios. But because common_with_showcase has subsheets, instead it treats that sheet as only having 3 total cards when calculating probabilities.

I hope that makes sense.

axxroytovu commented 1 year ago

I'll close this for now but include the best practice in the ReadMe in another commit