Refactor card ratings, fixed rare error exception "comparison method violates its general contract" (different ratings in same card);

Mage.Client/src/main/java/mage/client/MageFrame.java

@@ -33,6 +33,7 @@ import mage.client.util.stats.UpdateMemUsageTask;
 import mage.components.ImagePanel;
 import mage.components.ImagePanelStyle;
 import mage.constants.PlayerAction;
+import mage.game.draft.RateCard;
 import mage.interfaces.MageClient;
 import mage.interfaces.callback.CallbackClient;
 import mage.interfaces.callback.ClientCallback;
@@ -215,6 +216,7 @@ public class MageFrame extends javax.swing.JFrame implements MageClient {
         }
 
         RepositoryUtil.bootstrapLocalDb();
+        RateCard.bootstrapCardsAndRatings();
         ManaSymbols.loadImages();
         Plugins.instance.loadPlugins();
 

Mage.Server.Plugins/Mage.Player.AI/pom.xml

@@ -38,8 +38,8 @@
                 <groupId>org.apache.maven.plugins</groupId>
                 <artifactId>maven-compiler-plugin</artifactId>
                 <configuration>
-                    <source>1.7</source>
-                    <target>1.7</target>
+                    <source>1.8</source>
+                    <target>1.8</target>
                 </configuration>
             </plugin>
             <plugin>

Mage.Server.Plugins/Mage.Player.AI/src/main/java/mage/player/ai/ComputerPlayer.java

@@ -1,10 +1,6 @@
 
 package mage.player.ai;
 
-import java.io.IOException;
-import java.io.Serializable;
-import java.util.*;
-import java.util.Map.Entry;
 import mage.ConditionalMana;
 import mage.MageObject;
 import mage.MageObjectReference;
@@ -37,6 +33,7 @@ import mage.filter.predicate.permanent.ControllerIdPredicate;
 import mage.game.Game;
 import mage.game.combat.CombatGroup;
 import mage.game.draft.Draft;
+import mage.game.draft.RateCard;
 import mage.game.events.GameEvent;
 import mage.game.events.GameEvent.EventType;
 import mage.game.match.Match;
@@ -47,7 +44,6 @@ import mage.game.tournament.Tournament;
 import mage.player.ai.simulators.CombatGroupSimulator;
 import mage.player.ai.simulators.CombatSimulator;
 import mage.player.ai.simulators.CreatureSimulator;
-import mage.player.ai.utils.RateCard;
 import mage.players.Player;
 import mage.players.PlayerImpl;
 import mage.players.net.UserData;
@@ -60,8 +56,12 @@ import mage.util.TournamentUtil;
 import mage.util.TreeNode;
 import org.apache.log4j.Logger;
 
+import java.io.IOException;
+import java.io.Serializable;
+import java.util.*;
+import java.util.Map.Entry;
+
 /**
- *
  * suitable for two player games and some multiplayer games
  *
  * @author BetaSteward_at_googlemail.com
@@ -134,7 +134,7 @@ public class ComputerPlayer extends PlayerImpl implements Player {
 
         UUID randomOpponentId;
         if (target.getTargetController() != null) {
-            randomOpponentId = getRandomOpponent(target.getTargetController(), game);;
+            randomOpponentId = getRandomOpponent(target.getTargetController(), game);
         } else if (abilityControllerId != null) {
             randomOpponentId = getRandomOpponent(abilityControllerId, game);
         } else {
@@ -427,7 +427,7 @@ public class ComputerPlayer extends PlayerImpl implements Player {
 
         UUID randomOpponentId;
         if (target.getTargetController() != null) {
-            randomOpponentId = getRandomOpponent(target.getTargetController(), game);;
+            randomOpponentId = getRandomOpponent(target.getTargetController(), game);
         } else if (source != null && source.getControllerId() != null) {
             randomOpponentId = getRandomOpponent(source.getControllerId(), game);
         } else {
@@ -1423,11 +1423,10 @@ public class ComputerPlayer extends PlayerImpl implements Player {
     }
 
     /**
-     *
      * returns a list of Permanents that produce mana sorted by the number of
      * mana the Permanent produces that match the unpaid costs in ascending
      * order
-     *
+     * <p>
      * the idea is that we should pay costs first from mana producers that
      * produce only one type of mana and save the multi-mana producers for those
      * costs that can't be paid by any other producers
@@ -2089,13 +2088,13 @@ public class ComputerPlayer extends PlayerImpl implements Player {
         try {
             Card bestCard = pickBestCard(cards, chosenColors);
             int maxScore = RateCard.rateCard(bestCard, chosenColors);
-            int pickedCardRate = RateCard.getCardRating(bestCard);
+            int pickedCardRate = RateCard.getBaseCardScore(bestCard);
 
             if (pickedCardRate <= 30) {
                 // if card is bad
                 // try to counter pick without any color restriction
                 Card counterPick = pickBestCard(cards, null);
-                int counterPickScore = RateCard.getCardRating(counterPick);
+                int counterPickScore = RateCard.getBaseCardScore(counterPick);
                 // card is really good
                 // take it!
                 if (counterPickScore >= 80) {
@@ -2441,7 +2440,6 @@ public class ComputerPlayer extends PlayerImpl implements Player {
 
     /**
      * Sets a possible target player
-     *
      */
     private boolean setTargetPlayer(Outcome outcome, Target target, Ability source, UUID sourceId, UUID abilityControllerId, UUID randomOpponentId, Game game) {
         if (target.getOriginalTarget() instanceof TargetOpponent) {

Mage.Server/src/main/java/mage/server/Main.java

@@ -5,6 +5,7 @@ import mage.cards.Sets;
 import mage.cards.repository.CardScanner;
 import mage.cards.repository.PluginClassloaderRegistery;
 import mage.cards.repository.RepositoryUtil;
+import mage.game.draft.RateCard;
 import mage.game.match.MatchType;
 import mage.game.tournament.TournamentType;
 import mage.interfaces.MageServer;
@@ -135,6 +136,10 @@ public final class Main {
         }
         logger.info("Done.");
 
+        // cards preload with ratings
+        RateCard.bootstrapCardsAndRatings();
+        logger.info("Done.");
+
         logger.info("Updating user stats DB...");
         UserStatsRepository.instance.updateUserStats();
         logger.info("Done.");

Mage.Verify/src/test/java/mage/verify/VerifyCardDataTest.java

@@ -4,11 +4,14 @@ import mage.ObjectColor;
 import mage.abilities.keyword.MultikickerAbility;
 import mage.cards.*;
 import mage.cards.basiclands.BasicLand;
-import mage.cards.repository.*;
+import mage.cards.repository.CardInfo;
+import mage.cards.repository.CardRepository;
+import mage.cards.repository.CardScanner;
 import mage.constants.CardType;
 import mage.constants.Rarity;
 import mage.constants.SubType;
 import mage.constants.SuperType;
+import mage.game.draft.RateCard;
 import mage.game.permanent.token.Token;
 import mage.game.permanent.token.TokenImpl;
 import org.junit.Assert;
@@ -90,21 +93,6 @@ public class VerifyCardDataTest {
         skipListCreate("MISSING_ABILITIES");
     }
 
-    public static List<Card> allCards() {
-        Collection<ExpansionSet> sets = Sets.getInstance().values();
-        List<Card> cards = new ArrayList<>();
-        for (ExpansionSet set : sets) {
-            if (set.isCustomSet()) {
-                continue;
-            }
-            for (ExpansionSet.SetCardInfo setInfo : set.getSetCardInfo()) {
-                cards.add(CardImpl.createCard(setInfo.getCardClass(), new CardSetInfo(setInfo.getName(), set.getCode(),
-                        setInfo.getCardNumber(), setInfo.getRarity(), setInfo.getGraphicInfo())));
-            }
-        }
-        return cards;
-    }
-
     private void warn(Card card, String message) {
         outputMessages.add("Warning: " + message + " for " + card.getExpansionSetCode() + " - " + card.getName() + " - " + card.getCardNumber());
     }
@@ -119,7 +107,7 @@ public class VerifyCardDataTest {
 
     @Test
     public void verifyCards() throws IOException {
-        for (Card card : allCards()) {
+        for (Card card : CardScanner.getAllCards()) {
             Set<String> tokens = findSourceTokens(card.getClass());
             if (card.isSplitCard()) {
                 check(((SplitCard) card).getLeftHalfCard(), null);
@@ -658,6 +646,14 @@ public class VerifyCardDataTest {
         }
     }
 
+    private void checkLegalityFormats(Card card, JsonCard ref) {
+        if (skipListHaveName("LEGALITY", card.getExpansionSetCode(), card.getName())) {
+            return;
+        }
+
+        // TODO: add legality checks (by sets and cards, by banned)
+    }
+
     private String prepareRule(String cardName, String rule) {
         // remove and optimize rule text for analyze
         String newRule = rule;
@@ -901,4 +897,22 @@ public class VerifyCardDataTest {
         return result.toString();
     }
 
+    @Test
+    public void testCardRatingConsistency() {
+        // all cards with same name must have same rating (see RateCard.rateCard)
+        // cards rating must be consistency (same) for card sorting
+        List<Card> cardsList = new ArrayList<>(CardScanner.getAllCards());
+        Map<String, Integer> cardRates = new HashMap<>();
+        for (Card card : cardsList) {
+            int curRate = RateCard.rateCard(card, null, false);
+            int prevRate = cardRates.getOrDefault(card.getName(), 0);
+            if (prevRate == 0) {
+                cardRates.putIfAbsent(card.getName(), curRate);
+            } else {
+                if (curRate != prevRate) {
+                    Assert.fail("Card with same name have different ratings: " + card.getName());
+                }
+            }
+        }
+    }
 }

Mage/src/main/java/mage/cards/repository/CardScanner.java

@@ -4,6 +4,7 @@ import mage.cards.*;
 import org.apache.log4j.Logger;
 
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.List;
 
 /**
@@ -70,4 +71,23 @@ public final class CardScanner {
         }
         CardRepository.instance.saveCards(cardsToAdd, CardRepository.instance.getContentVersionConstant());
     }
+
+    public static List<Card> getAllCards() {
+        return getAllCards(true);
+    }
+
+    public static List<Card> getAllCards(boolean ignoreCustomSets) {
+        Collection<ExpansionSet> sets = Sets.getInstance().values();
+        List<Card> cards = new ArrayList<>();
+        for (ExpansionSet set : sets) {
+            if (ignoreCustomSets && set.isCustomSet()) {
+                continue;
+            }
+            for (ExpansionSet.SetCardInfo setInfo : set.getSetCardInfo()) {
+                cards.add(CardImpl.createCard(setInfo.getCardClass(), new CardSetInfo(setInfo.getName(), set.getCode(),
+                        setInfo.getCardNumber(), setInfo.getRarity(), setInfo.getGraphicInfo())));
+            }
+        }
+        return cards;
+    }
 }

Mage/src/main/java/mage/game/draft/RateCard.java

@@ -1,4 +1,4 @@
-package mage.player.ai.utils;
+package mage.game.draft;
 
 import mage.abilities.Ability;
 import mage.abilities.Mode;
@@ -7,9 +7,9 @@ import mage.abilities.effects.common.*;
 import mage.abilities.effects.common.continuous.BoostEnchantedEffect;
 import mage.abilities.effects.common.continuous.BoostTargetEffect;
 import mage.cards.Card;
+import mage.cards.repository.CardScanner;
 import mage.constants.ColoredManaSymbol;
 import mage.constants.Outcome;
-import mage.constants.Rarity;
 import mage.constants.SubType;
 import mage.target.Target;
 import mage.target.TargetPermanent;
@@ -30,9 +30,9 @@ import java.util.*;
  */
 public final class RateCard {
 
-    private static Map<String, Integer> ratings;
-    private static List<String> setsWithRatingsToBeLoaded;
+    private static Map<String, Integer> baseRatings = new HashMap<>();
     private static final Map<String, Integer> rated = new HashMap<>();
+    private static boolean isLoaded = false;
 
     /**
      * Rating that is given for new cards.
@@ -55,6 +55,15 @@ public final class RateCard {
     private RateCard() {
     }
 
+    public static void bootstrapCardsAndRatings() {
+        // preload cards and ratings
+        log.info("Loading cards and rating...");
+        List<Card> cards = CardScanner.getAllCards(false);
+        for (Card card : cards) {
+            RateCard.rateCard(card, null);
+        }
+    }
+
     /**
      * Get absolute score of the card.
      * Depends on type, manacost, rating.
@@ -65,10 +74,15 @@ public final class RateCard {
      * @return
      */
     public static int rateCard(Card card, List<ColoredManaSymbol> allowedColors) {
-        if (allowedColors == null && rated.containsKey(card.getName())) {
+        return rateCard(card, allowedColors, true);
+    }
+
+    public static int rateCard(Card card, List<ColoredManaSymbol> allowedColors, boolean useCache) {
+        if (useCache && allowedColors == null && rated.containsKey(card.getName())) {
             int rate = rated.get(card.getName());
             return rate;
         }
+
         int type;
         if (card.isPlaneswalker()) {
             type = 15;
@@ -83,10 +97,12 @@ public final class RateCard {
         } else {
             type = 6;
         }
-        int score = getCardRating(card) + 2 * type + getManaCostScore(card, allowedColors)
+        int score = getBaseCardScore(card) + 2 * type + getManaCostScore(card, allowedColors)
                 + 40 * isRemoval(card);
-        if (allowedColors == null)
+
+        if (useCache && allowedColors == null)
             rated.put(card.getName(), score);
+
         return score;
     }
 
@@ -153,78 +169,101 @@ public final class RateCard {
         return 0;
     }
 
+
     /**
      * Return rating of the card.
      *
      * @param card Card to rate.
      * @return Rating number from [1:100].
      */
-    public static int getCardRating(Card card) {
-        readRatingSetList();
-        String exp = card.getExpansionSetCode().toLowerCase();
-        readRatings(exp);
+    public static int getBaseCardScore(Card card) {
+        // same card name must have same rating
 
-        if (ratings != null && ratings.containsKey(card.getName())) {
-            return ratings.get(card.getName());
+        // ratings from files
+        // lazy loading on first request
+        prepareAndLoadRatings();
+
+        // ratings from card rarity
+        // some cards can have different rarity -- it's will be used from first set
+        int newRating;
+        switch (card.getRarity()) {
+            case COMMON:
+                newRating = DEFAULT_NOT_RATED_CARD_RATING;
+                break;
+            case UNCOMMON:
+                newRating = DEFAULT_NOT_RATED_UNCOMMON_RATING;
+                break;
+            case RARE:
+                newRating = DEFAULT_NOT_RATED_RARE_RATING;
+                break;
+            case MYTHIC:
+                newRating = DEFAULT_NOT_RATED_MYTHIC_RATING;
+                break;
+            default:
+                newRating = DEFAULT_NOT_RATED_CARD_RATING;
+                break;
         }
 
-        Rarity r = card.getRarity();
-        if (Rarity.COMMON == r) {
-            return DEFAULT_NOT_RATED_CARD_RATING;
-        } else if (Rarity.UNCOMMON == r) {
-            return DEFAULT_NOT_RATED_UNCOMMON_RATING;
-        } else if (Rarity.RARE == r) {
-            return DEFAULT_NOT_RATED_RARE_RATING;
-        } else if (Rarity.MYTHIC == r) {
-            return DEFAULT_NOT_RATED_MYTHIC_RATING;
+        int oldRating = baseRatings.getOrDefault(card.getName(), 0);
+        if (oldRating != 0 && oldRating != newRating) {
+            //log.info("card have different rating by sets: " + card.getName() + " (" + oldRating + " <> " + newRating + ")");
+        }
+
+        if (oldRating != 0) {
+            return oldRating;
+        } else {
+            baseRatings.put(card.getName(), newRating);
+            return newRating;
         }
-        return DEFAULT_NOT_RATED_CARD_RATING;
     }
 
     /**
-     * reads the list of sets that have ratings csv files
-     * populates the setsWithRatingsToBeLoaded
+     * reads the list of sets that have ratings csv files and read each file
      */
-    private synchronized static void readRatingSetList() {
+    public synchronized static void prepareAndLoadRatings() {
+        if (isLoaded) {
+            return;
+        }
+
+        // load sets list
+        List<String> setsToLoad = new LinkedList<>();
         try {
-            if (setsWithRatingsToBeLoaded == null) {
-                setsWithRatingsToBeLoaded = new LinkedList<>();
             InputStream is = RateCard.class.getResourceAsStream(RATINGS_SET_LIST);
             Scanner scanner = new Scanner(is);
             while (scanner.hasNextLine()) {
                 String line = scanner.nextLine();
                 if (!line.substring(0, 1).equals("#")) {
-                        setsWithRatingsToBeLoaded.add(line);
+                    setsToLoad.add(line);
                 }
             }
-            }
-        } catch (Exception e) {
-            log.info("failed to read ratings set list file: " + RATINGS_SET_LIST);
-            e.printStackTrace();
-        }
+        } catch (Throwable e) {
+            log.error("Failed to read ratings sets list file: " + RATINGS_SET_LIST, e);
         }
 
-    /**
-     * Reads ratings from resources and loads them into ratings map
-     */
-    private synchronized static void readRatings(String expCode) {
-        if (ratings == null) {
-            ratings = new HashMap<>();
+        // load set data
+        String rateFile = "";
+        try {
+            for (String code : setsToLoad) {
+                //log.info("Reading ratings for the set " + code);
+                rateFile = RATINGS_DIR + code + ".csv";
+                readFromFile(rateFile);
             }
-        if (setsWithRatingsToBeLoaded.contains(expCode)) {
-            log.info("reading draftbot ratings for the set " + expCode);
-            readFromFile(RATINGS_DIR + expCode + ".csv");
-            setsWithRatingsToBeLoaded.remove(expCode);
+        } catch (Exception e) {
+            log.error("Failed to read ratings set file: " + rateFile, e);
         }
+
+        isLoaded = true;
     }
 
     /**
      * reads ratings from the file
      */
     private synchronized static void readFromFile(String path) {
+        // card must get max rating from multiple cards
         Integer min = Integer.MAX_VALUE, max = 0;
         Map<String, Integer> thisFileRatings = new HashMap<>();
-        try {
+
+        // load
         InputStream is = RateCard.class.getResourceAsStream(path);
         Scanner scanner = new Scanner(is);
         while (scanner.hasNextLine()) {
@@ -242,19 +281,16 @@ public final class RateCard {
                 thisFileRatings.put(name, rating);
             }
         }
+
         // normalize for the file to [1..100]
         for (String name : thisFileRatings.keySet()) {
             int r = thisFileRatings.get(name);
-                int newrating = (int) (100.0f * (r - min) / (max - min));
-                if (!ratings.containsKey(name) ||
-                        (ratings.containsKey(name) && newrating > ratings.get(name))) {
-                    ratings.put(name, newrating);
+            int newRating = (int) (100.0f * (r - min) / (max - min));
+            int oldRating = baseRatings.getOrDefault(name, 0);
+            if (newRating > oldRating) {
+                baseRatings.put(name, newRating);
             }
         }
-        } catch (Exception e) {
-            log.info("failed to read ratings file: " + path);
-            e.printStackTrace();
-        }
     }
 
     private static final int[] SINGLE_PENALTY = {0, 1, 1, 3, 6, 9};