Fix 15 card booster generation #13206
Conversation
I noticed the code for reducing boosters to 15 cards and realised it wouldn't work well with collated boosters, or with boosters with fixing in the first slot.
There was a problem hiding this comment.
I agree that the current algorithm is not ideal and makes a bad assumption... but what testing have you done for the new algorithm? Can you be sure that no errors will arise as a result?
What sets currently generate boosters of more than 15 cards that would be affected?
I might want to add a test for every set with booster generation to verify the number of cards (and implicitly catch booster generation errors), I'll have to think about that
| // removing positional cards from collated boosters is going to mess with balancing and as-fan | ||
| // also for sets with common lands in the land slot, this may eliminate the majority of fixing from a pack | ||
| // instead removing a random card that is not in the last four (where rare and uncommons usually are - though some uncommons may be displayed by cards with special collation - eg DFC or bonus sheet). | ||
| if (this.Booster.size() > 15 && this.Booster.get(0).getRarity() == Rarity.LAND) { |
There was a problem hiding this comment.
this.Booster is not correct
There was a problem hiding this comment.
That's what I get for rushing to submit just before the tournament started - fixed now.
As for your first question, the only one I know of is CLB, but there could be others. For CLB, the last four cards (that this protects from being eliminated) are Background, Legendary, Rare, and Foil (which could be any rarity). So the uncommons are at risk. (Edit - changed code so now the uncommons are protected also.)
(According to lethe, 2X2 comes in 16card boosters, with Cryptic Spires being the 16th card. So that is another potential one to trigger this in the future.)
I had considered a slightly more complicated approach of multistep processing.
List<Card> foundCards = [find all basiclands in booster] ;
if (theBooster.size() - foundCards.size() >= 15 ) {
theBooster.remove(foundCards);
foundCards = [find all commons in booster] ;
if (theBooster.size() - foundCards.size() >= 15 ) {
theBooster.remove(foundCards);
foundCards = [find all uncommons in booster] ;
}
}
if (theBooster.size() < 15 ) {
[raise an error]
}
while (theBooster.size() - foundCards.size() < 15 ) {
foundCards.remove( RandomUtil.nextInt( foundCards.size()) );
}
theBooster.remove( foundCards );
// if still greater than 15 and removed all basics, lands, and uncommons, just go ahead with positional removal
while (theBooster.size() > 15) {
theBooster.remove(0);
}
| @@ -145,6 +145,7 @@ public static ExpansionSetComparator getComparator() { | |||
| protected boolean hasAlternateBoosterPrintings = true; // not counting basic lands; e.g. Fallen Empires true, but Tenth Edition false | |||
|
|
|||
| protected int maxCardNumberInBooster; // used to omit cards with collector numbers beyond the regular cards in a set for boosters | |||
| protected int maxCardNumberBasePrint; // used to omit limit boosters to only contain cards with a base printing | |||
There was a problem hiding this comment.
why are you adding this? what does it have to do with your change?
There was a problem hiding this comment.
oops thought I had taken that one out. Was part of something else.
|
It's ok to remove random or land booster card instead first one. Just make sure it's generate too many cards by design, not by error in some collations or other code parts. |
|
As for testing, I tested that the array size and random number generation worked correctly. I also looked up Mage/src/main/java/mage/cards/Card.java to check for determining rarity (when checking if the first card was a basic land). Edit - changed code so that there is no chance of removing an uncommon, rare, or mythic that is preceded by enough commons to remove first. A higher rarity card can only be removed when it is the first card in the booster. I have no idea what create15CardBooster gets used for, just coming across it got me concerned. (It will be more and more relevant in the future due to modern sets being designed as 14 card boosters.) But that is a different topic. |
Tested with 20 card packs containing 13 commons followed by 5 uncommons, a rare, then a card of any rarity. Also tested with 20 card packs containing 3 commons followed by 15 uncommons, a rare, then a card of any rarity. ------- In the first scenario, only commons from the group of 13 were removed. In the second scenario, the first three commons got removed, then the first two uncommons in the pack. That was expected behaviour. Note - second scenario not expected to occur.
I noticed the code for reducing boosters to 15 cards and realised it wouldn't work well with collated boosters, or with boosters with fixing in the first slot.