From fb27b66621fee8422b7e2cbeeec9b00ff296eb86 Mon Sep 17 00:00:00 2001 From: leriomaggio Date: Wed, 25 Aug 2021 12:07:00 +0100 Subject: [PATCH 01/23] Added docstring to getCardInSet method --- forge-core/src/main/java/forge/card/CardEdition.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/forge-core/src/main/java/forge/card/CardEdition.java b/forge-core/src/main/java/forge/card/CardEdition.java index 995086dad64..9df2546479f 100644 --- a/forge-core/src/main/java/forge/card/CardEdition.java +++ b/forge-core/src/main/java/forge/card/CardEdition.java @@ -373,6 +373,13 @@ public final class CardEdition implements Comparable { } private ListMultimap cardsInSetLookupMap = null; + + /** + * Get all the CardInSet instances with the input card name. + * @param cardName Name of the Card to look for. + * @return A List of all the CardInSet instances for a given name. + * If not fount, an Empty sequence (view) will be returned instead! + */ public List getCardInSet(String cardName){ if (cardsInSetLookupMap == null) { // initialise From b49fe104fc3ca1a711dc5222f9ff644aac450c1d Mon Sep 17 00:00:00 2001 From: leriomaggio Date: Wed, 25 Aug 2021 12:18:53 +0100 Subject: [PATCH 02/23] Added constant for NO artist name (for later re-use) --- forge-core/src/main/java/forge/item/IPaperCard.java | 1 + 1 file changed, 1 insertion(+) diff --git a/forge-core/src/main/java/forge/item/IPaperCard.java b/forge-core/src/main/java/forge/item/IPaperCard.java index 8ce1fa895e4..36ad4812e54 100644 --- a/forge-core/src/main/java/forge/item/IPaperCard.java +++ b/forge-core/src/main/java/forge/item/IPaperCard.java @@ -20,6 +20,7 @@ public interface IPaperCard extends InventoryItem, Serializable { String NO_COLLECTOR_NUMBER = "N.A."; // Placeholder for No-Collection number available int DEFAULT_ART_INDEX = 1; int NO_ART_INDEX = -1; // Placeholder when NO ArtIndex is Specified + String NO_ARTIST_NAME = ""; /** * Number of filters based on CardPrinted values. From 0db01499b3cd360b3747793c24acc8319ed60d08 Mon Sep 17 00:00:00 2001 From: leriomaggio Date: Wed, 25 Aug 2021 12:19:19 +0100 Subject: [PATCH 03/23] Returning correct foiled card (no code reps) --- forge-gui-desktop/src/test/java/forge/card/LegacyCardDb.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/forge-gui-desktop/src/test/java/forge/card/LegacyCardDb.java b/forge-gui-desktop/src/test/java/forge/card/LegacyCardDb.java index 70c656dcce9..876910c870d 100644 --- a/forge-gui-desktop/src/test/java/forge/card/LegacyCardDb.java +++ b/forge-gui-desktop/src/test/java/forge/card/LegacyCardDb.java @@ -183,7 +183,7 @@ public class LegacyCardDb { } public PaperCard getFoiled(PaperCard card0) { - return new PaperCard(card0.getRules(), card0.getEdition(), card0.getRarity(), card0.getArtIndex(), true, card0.getArtist()); + return card0.getFoiled(); } public PaperCard getCardFromEdition(final String cardName, LegacySetPreference fromSet) { From c726cf6fb039e9537a5d8a28a1a1fd6597c6a3f2 Mon Sep 17 00:00:00 2001 From: leriomaggio Date: Wed, 25 Aug 2021 12:22:02 +0100 Subject: [PATCH 04/23] Various Optimisations for re-use and removed dead-code Various optimisation to the PaperCard implementation: - removed once and for all retrieveCollectorNumber method - we're now safe and sound that all PaperCard instances will have a collector Number matched. - That is also made sure by removing all the many (legacy) class constructors. Only left two - with correct parameter passing! - finally, collectorNumberSortingKey is created onces, and saved - as this won't change (for better performance) --- .../src/main/java/forge/item/PaperCard.java | 73 ++++--------------- 1 file changed, 14 insertions(+), 59 deletions(-) diff --git a/forge-core/src/main/java/forge/item/PaperCard.java b/forge-core/src/main/java/forge/item/PaperCard.java index 36ceceb3ad8..3441fe833b8 100644 --- a/forge-core/src/main/java/forge/item/PaperCard.java +++ b/forge-core/src/main/java/forge/item/PaperCard.java @@ -52,7 +52,7 @@ public final class PaperCard implements Comparable, InventoryItemFro By default the attribute is marked as "unset" so that it could be retrieved and set. (see getCollectorNumber()) */ - private String collectorNumber = null; + private String collectorNumber; private final String artist; private final int artIndex; private final boolean foil; @@ -75,18 +75,6 @@ public final class PaperCard implements Comparable, InventoryItemFro @Override public String getCollectorNumber() { - /* The collectorNumber attribute is managed in a property-like fashion. - By default it is marked as "unset" (-1), which integrates with all constructors - invocations not including this as an extra parameter. In this way, the new - attribute could be added to the API with minimum disruption to code - throughout the other packages. - If "unset", the corresponding collectorNumber will be retrieved - from the corresponding CardEdition (see retrieveCollectorNumber) - * */ - if (collectorNumber == null) { - collectorNumber = this.retrieveCollectorNumber(); - } - return collectorNumber; } @@ -169,15 +157,12 @@ public final class PaperCard implements Comparable, InventoryItemFro }; public PaperCard(final CardRules rules0, final String edition0, final CardRarity rarity0){ - this(rules0, edition0, rarity0, IPaperCard.DEFAULT_ART_INDEX); + this(rules0, edition0, rarity0, IPaperCard.DEFAULT_ART_INDEX, false, + IPaperCard.NO_COLLECTOR_NUMBER, IPaperCard.NO_ARTIST_NAME); } - public PaperCard(final CardRules rules0, final String edition0, final CardRarity rarity0, final int artIndex0) { - this(rules0, edition0, rarity0, artIndex0, false, ""); - } - - public PaperCard(final CardRules rules0, final String edition0, final CardRarity rarity0, final int artIndex0, - final boolean foil0, final String artist0) { + public PaperCard(final CardRules rules0, final String edition0, final CardRarity rarity0, + final int artIndex0, final boolean foil0, final String collectorNumber0, final String artist0) { if (rules0 == null || edition0 == null || rarity0 == null) { throw new IllegalArgumentException("Cannot create card without rules, edition or rarity"); } @@ -187,16 +172,8 @@ public final class PaperCard implements Comparable, InventoryItemFro artIndex = Math.max(artIndex0, IPaperCard.DEFAULT_ART_INDEX); foil = foil0; rarity = rarity0; - artist = (artist0 != null ? artist0 : ""); - } - - public PaperCard(final CardRules rules0, final String edition0, final CardRarity rarity0, - final int artIndex0, final boolean foil0, final String collectorNumber0, final String artist) { - this(rules0, edition0, rarity0, artIndex0, foil0, artist); - if ((collectorNumber0 == null) || (collectorNumber0.length() == 0)) - collectorNumber = IPaperCard.NO_COLLECTOR_NUMBER; - else - collectorNumber = collectorNumber0; + artist = (artist0 != null ? artist0 : IPaperCard.NO_ARTIST_NAME); + collectorNumber = (collectorNumber0 != null) && (collectorNumber0.length() > 0) ? collectorNumber0 : IPaperCard.NO_COLLECTOR_NUMBER; } // Want this class to be a key for HashTable @@ -268,10 +245,14 @@ public final class PaperCard implements Comparable, InventoryItemFro return CardEdition.CardInSet.getSortableCollectorNumber(collectorNumber); } + private String sortableCNKey = null; public String getCollectorNumberSortingKey(){ - // Hardly the case, but just invoke getter rather than direct - // attribute to be sure that collectorNumber has been retrieved already! - return makeCollectorNumberSortingKey(getCollectorNumber()); + if (sortableCNKey == null) { + // Hardly the case, but just invoke getter rather than direct + // attribute to be sure that collectorNumber has been retrieved already! + sortableCNKey = makeCollectorNumberSortingKey(getCollectorNumber()); + } + return sortableCNKey; } @@ -309,32 +290,6 @@ public final class PaperCard implements Comparable, InventoryItemFro rarity = pc.getRarity(); } - // FIXME: @leriomaggio - remember to get rid of this method once and for all :) - private String retrieveCollectorNumber() { - StaticData data = StaticData.instance(); - CardEdition edition = data.getEditions().get(this.edition); - if (edition == null) { - edition = data.getCustomEditions().get(this.edition); - if (edition == null) // don't bother continuing - non-existing card! - return NO_COLLECTOR_NUMBER; - } - int artIndexCount = 0; - String collectorNumberInEdition = ""; - for (CardEdition.CardInSet card : edition.getAllCardsInSet()) { - if (card.name.equalsIgnoreCase(this.name)) { - artIndexCount += 1; - if (artIndexCount == this.artIndex) { - collectorNumberInEdition = card.collectorNumber; - break; - } - } - } - // CardEdition stores collectorNumber as a String, which is null if there isn't any. - // In this case, the NO_COLLECTOR_NUMBER value (i.e. 0) is returned. - return ((collectorNumberInEdition != null) && (collectorNumberInEdition.length() > 0)) ? - collectorNumberInEdition : NO_COLLECTOR_NUMBER; - } - @Override public String getImageKey(boolean altState) { String imageKey = ImageKeys.CARD_PREFIX + name + CardDb.NameSetSeparator From f45f9f6c04be3cebc9ac879cefbc48f0565bde29 Mon Sep 17 00:00:00 2001 From: leriomaggio Date: Wed, 25 Aug 2021 12:23:00 +0100 Subject: [PATCH 05/23] Updated Foil of PaperCard - re-using getFoiled method instead of useless removed constructor. --- .../screens/deckeditor/controllers/ACEditorBase.java | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/forge-gui-desktop/src/main/java/forge/screens/deckeditor/controllers/ACEditorBase.java b/forge-gui-desktop/src/main/java/forge/screens/deckeditor/controllers/ACEditorBase.java index df6040d1d3b..15df28f0886 100644 --- a/forge-gui-desktop/src/main/java/forge/screens/deckeditor/controllers/ACEditorBase.java +++ b/forge-gui-desktop/src/main/java/forge/screens/deckeditor/controllers/ACEditorBase.java @@ -536,13 +536,7 @@ public abstract class ACEditorBase Date: Wed, 25 Aug 2021 12:28:13 +0100 Subject: [PATCH 06/23] First CardDb optimisation - reusing same strategy for lazyLoading There was a FIXME/TODO comment I just have found out relating to putCard implementation being inefficient. I've re-used the new-methods implemented for lazyCardLoading to improve existing implementation. Also, now all the PaperCard constructors won't miss collectorNumber and ArtistNames as gathered from CardInSet in CardEdition - YAY --- .../src/main/java/forge/card/CardDb.java | 24 +++++++++++-------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/forge-core/src/main/java/forge/card/CardDb.java b/forge-core/src/main/java/forge/card/CardDb.java index f573c618792..a1dd0cc8b98 100644 --- a/forge-core/src/main/java/forge/card/CardDb.java +++ b/forge-core/src/main/java/forge/card/CardDb.java @@ -1006,29 +1006,33 @@ public final class CardDb implements ICardDatabase, IDeckGenPool { // 1. generate all paper cards from edition data we have (either explicit, or found in res/editions, or add to unknown edition) List paperCards = new ArrayList<>(); if (null == whenItWasPrinted || whenItWasPrinted.isEmpty()) { - // TODO Not performant Each time we "putCard" we loop through ALL CARDS IN ALL editions + // @friarsol: Not performant Each time we "putCard" we loop through ALL CARDS IN ALL editions + // @leriomaggio: DONE! re-using here the same strategy implemented for lazy-loading! for (CardEdition e : editions.getOrderedEditions()) { int artIdx = IPaperCard.DEFAULT_ART_INDEX; - for (CardInSet cis : e.getAllCardsInSet()) { - if (!cis.name.equals(cardName)) { - continue; - } - paperCards.add(new PaperCard(rules, e.getCode(), cis.rarity, artIdx++)); - } + for (CardInSet cis : e.getCardInSet(cardName)) + paperCards.add(new PaperCard(rules, e.getCode(), cis.rarity, artIdx++, false, + cis.collectorNumber, cis.artistName)); } } else { String lastEdition = null; int artIdx = 0; for (Pair tuple : whenItWasPrinted) { if (!tuple.getKey().equals(lastEdition)) { - artIdx = IPaperCard.DEFAULT_ART_INDEX; + artIdx = IPaperCard.DEFAULT_ART_INDEX; // reset artIndex lastEdition = tuple.getKey(); } CardEdition ed = editions.get(lastEdition); - if (null == ed) { + if (ed == null) { continue; } - paperCards.add(new PaperCard(rules, lastEdition, tuple.getValue(), artIdx++)); + List cardsInSet = ed.getCardInSet(cardName); + if (cardsInSet.isEmpty()) + continue; + int cardInSetIndex = Math.max(artIdx-1, 0); // make sure doesn't go below zero + CardInSet cds = cardsInSet.get(cardInSetIndex); // use ArtIndex to get the right Coll. Number + paperCards.add(new PaperCard(rules, lastEdition, tuple.getValue(), artIdx++, false, + cds.collectorNumber, cds.artistName)); } } if (paperCards.isEmpty()) { From 637aadc1880f5056f4fc9839feff4131e1843bc0 Mon Sep 17 00:00:00 2001 From: leriomaggio Date: Wed, 25 Aug 2021 18:13:39 +0100 Subject: [PATCH 07/23] Forcing ImageKeys.hasImage to true in Mock objects! The alternative case is covered by another specific testcase --- .../src/test/java/forge/card/ForgeCardMockTestCase.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/forge-gui-desktop/src/test/java/forge/card/ForgeCardMockTestCase.java b/forge-gui-desktop/src/test/java/forge/card/ForgeCardMockTestCase.java index 3f54907726a..2b24a3f22d2 100644 --- a/forge-gui-desktop/src/test/java/forge/card/ForgeCardMockTestCase.java +++ b/forge-gui-desktop/src/test/java/forge/card/ForgeCardMockTestCase.java @@ -5,6 +5,7 @@ import forge.ImageKeys; import forge.Singletons; import forge.StaticData; import forge.gamesimulationtests.util.CardDatabaseHelper; +import forge.item.PaperCard; import forge.localinstance.properties.ForgeConstants; import forge.localinstance.properties.ForgePreferences; import forge.model.FModel; @@ -133,6 +134,9 @@ public class ForgeCardMockTestCase extends PowerMockTestCase { PowerMockito.mockStatic(ImageKeys.class); initForgeConstants(); + // Always Has Image (there is a separated test case to cover the opposite case) + PowerMockito.when(ImageKeys.hasImage(Mockito.any(PaperCard.class))).thenReturn(false); + //Mocking some more static stuff PowerMockito.mockStatic(Singletons.class); PowerMockito.mockStatic(FModel.class); From 841c98dc125ebac4ecf18bf95fd11b283b097783 Mon Sep 17 00:00:00 2001 From: leriomaggio Date: Wed, 25 Aug 2021 18:15:28 +0100 Subject: [PATCH 08/23] First round of optimisation to CardDb This optimisation removes redundant queries when looking for cards by specificed Art Preference. getCardsFromSet has been worked-around, avoiding querying for potential candidates already in memory. A preliminary benchmark/tests is implemented too. --- .../src/main/java/forge/card/CardDb.java | 43 ++++++---- .../forge/card/CardDbPerformanceTests.java | 86 +++++++++++++++++++ 2 files changed, 112 insertions(+), 17 deletions(-) create mode 100644 forge-gui-desktop/src/test/java/forge/card/CardDbPerformanceTests.java diff --git a/forge-core/src/main/java/forge/card/CardDb.java b/forge-core/src/main/java/forge/card/CardDb.java index a1dd0cc8b98..c57fb351c73 100644 --- a/forge-core/src/main/java/forge/card/CardDb.java +++ b/forge-core/src/main/java/forge/card/CardDb.java @@ -465,7 +465,7 @@ public final class CardDb implements ICardDatabase, IDeckGenPool { // So now check whether the cards exists in the DB first, // and select pick the card based on current SetPreference policy as a fallback Collection cards = getAllCards(request.cardName); - if (cards == null) + if (cards.isEmpty()) // Never null being this a view in MultiMap return null; // Either No Edition has been specified OR as a fallback in case of any error! // get card using the default card art preference @@ -640,6 +640,7 @@ public final class CardDb implements ICardDatabase, IDeckGenPool { @Override public boolean apply(PaperCard c) { CardEdition ed = editions.get(c.getEdition()); + if (ed == null) return false; if (releasedBeforeFlag) return ed.getDate().before(releaseDate); else @@ -648,22 +649,24 @@ public final class CardDb implements ICardDatabase, IDeckGenPool { })); } - if (cards.size() == 0) // Don't bother continuing! No card has been found! - return null; - /* 2. Retrieve cards based of [Frame]Set Preference ================================================ */ // Collect the list of all editions found for target card - LinkedHashSet cardEditions = new LinkedHashSet<>(); + List cardEditions = new ArrayList<>(); + Map candidatesCard = new HashMap<>(); for (PaperCard card : cards) { + if (card.getArtIndex() != cr.artIndex) + continue; String setCode = card.getEdition(); + CardEdition ed = null; if (setCode.equals(CardEdition.UNKNOWN.getCode())) - cardEditions.add(CardEdition.UNKNOWN); - else { - CardEdition ed = editions.get(card.getEdition()); - if (ed != null) - cardEditions.add(ed); + ed = CardEdition.UNKNOWN; + else + ed = editions.get(card.getEdition()); + if (ed != null) { + cardEditions.add(ed); + candidatesCard.put(setCode, card); } } // Filter Cards Editions based on set preferences @@ -681,16 +684,19 @@ public final class CardDb implements ICardDatabase, IDeckGenPool { If this happens, we won't try to iterate over an empty list. Instead, we will fall back to original lists of editions (unfiltered, of course) AND STILL sorted according to chosen art preference. */ - if (acceptedEditions.size() == 0) + if (acceptedEditions.isEmpty()) acceptedEditions.addAll(cardEditions); - Collections.sort(acceptedEditions); // CardEdition correctly sort by (release) date - if (artPref.latestFirst) - Collections.reverse(acceptedEditions); // newest editions first + if (acceptedEditions.size() > 1) { + Collections.sort(acceptedEditions); // CardEdition correctly sort by (release) date + if (artPref.latestFirst) + Collections.reverse(acceptedEditions); // newest editions first + } + PaperCard candidate = null; for (CardEdition ed : acceptedEditions) { - PaperCard cardFromSet = getCardFromSet(cr.cardName, ed, cr.artIndex, cr.isFoil); - if (candidate == null && cardFromSet != null) + PaperCard cardFromSet = candidatesCard.get(ed.getCode()); //getCardFromSet(cr.cardName, ed, cr.artIndex, cr.isFoil); + if (candidate == null) // save the first card found, as the last backup in case no other candidate *with image* will be found candidate = cardFromSet; @@ -699,8 +705,11 @@ public final class CardDb implements ICardDatabase, IDeckGenPool { break; // we're done here: found card **with Image** } } + if (candidate == null) + return null; + //If any, we're sure that at least one candidate is always returned nevertheless it has image or not - return candidate; // any foil request already handled in getCardFromSet + return cr.isFoil ? candidate.getFoiled() : candidate; } @Override diff --git a/forge-gui-desktop/src/test/java/forge/card/CardDbPerformanceTests.java b/forge-gui-desktop/src/test/java/forge/card/CardDbPerformanceTests.java new file mode 100644 index 00000000000..2e8129862c0 --- /dev/null +++ b/forge-gui-desktop/src/test/java/forge/card/CardDbPerformanceTests.java @@ -0,0 +1,86 @@ +package forge.card; + +import forge.ImageKeys; +import forge.item.PaperCard; +import org.mockito.Mockito; +import org.powermock.api.mockito.PowerMockito; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.Test; + +import java.util.Collection; +import java.util.Set; +import java.util.TreeSet; + +import static org.testng.Assert.assertNotNull; + +public class CardDbPerformanceTests extends CardDbTestCase { + + private Set fullDbCardNames = new TreeSet<>(); + + @Override + @BeforeMethod + public void setup() { + super.setup(); + Collection uniqueCards = this.cardDb.getUniqueCards(); + for (PaperCard card : uniqueCards) + this.fullDbCardNames.add(card.getName()); + } + + @Test + public void testBenchmarkFullDbGetCardLegacyImplementation() { + int nRuns = 100; + long averageTime = 0; + long minTime = 10000; // 10 secs + long maxTime = 0; + for (int r = 1; r <= nRuns; r++) { + long start = System.currentTimeMillis(); + for (String name : this.fullDbCardNames) { + PaperCard card = this.legacyCardDb.getCard(name); + assertNotNull(card); + } + long timeRun = System.currentTimeMillis() - start; + averageTime += timeRun; + if (timeRun < minTime) + minTime = timeRun; + if (timeRun > maxTime) + maxTime = timeRun; + } + System.out.println("[LEGACY] Total Time (in sec): " + ((double) averageTime)/ 1000); + System.out.println("[LEGACY] Average Time (in sec): " + ((double) averageTime / nRuns)/ 1000); + System.out.println("[LEGACY] Best Time (in sec): " + ((double) minTime)/ 1000); + System.out.println("[LEGACY] Worst Time (in sec): " + ((double) maxTime)/ 1000); + } + + @Test + public void testBenchmarkFullDbGetCardNewDbImplementation() { + int nRuns = 100; + long averageTime = 0; + long minTime = 10000; // 10 secs + long maxTime = 0; + for (int r = 1; r <= nRuns; r++) { + long start = System.currentTimeMillis(); + for (String name : this.fullDbCardNames) { + PaperCard card = this.cardDb.getCard(name); + assertNotNull(card); + } + long timeRun = System.currentTimeMillis() - start; + averageTime += timeRun; + if (timeRun < minTime) + minTime = timeRun; + if (timeRun > maxTime) + maxTime = timeRun; + } + System.out.println("[NEW] Total Time (in sec): " + ((double) averageTime)/ 1000); + System.out.println("[NEW] Average Time (in sec): " + ((double) averageTime / nRuns)/ 1000); + System.out.println("[NEW] Best Time (in sec): " + ((double) minTime)/ 1000); + System.out.println("[NEW] Worst Time (in sec): " + ((double) maxTime)/ 1000); + } + + @Test + public void testGetCardFullDbNewImplementationToProfile(){ + for (String name : this.fullDbCardNames) { + PaperCard card = this.cardDb.getCard(name); + assertNotNull(card); + } + } +} From eff9bc4d895c5abba6a2b3878c0f96271cb9a456 Mon Sep 17 00:00:00 2001 From: leriomaggio Date: Wed, 25 Aug 2021 18:31:26 +0100 Subject: [PATCH 09/23] typo - has to return true not false! --- .../src/test/java/forge/card/ForgeCardMockTestCase.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/forge-gui-desktop/src/test/java/forge/card/ForgeCardMockTestCase.java b/forge-gui-desktop/src/test/java/forge/card/ForgeCardMockTestCase.java index 2b24a3f22d2..61acc9cd91a 100644 --- a/forge-gui-desktop/src/test/java/forge/card/ForgeCardMockTestCase.java +++ b/forge-gui-desktop/src/test/java/forge/card/ForgeCardMockTestCase.java @@ -135,7 +135,7 @@ public class ForgeCardMockTestCase extends PowerMockTestCase { initForgeConstants(); // Always Has Image (there is a separated test case to cover the opposite case) - PowerMockito.when(ImageKeys.hasImage(Mockito.any(PaperCard.class))).thenReturn(false); + PowerMockito.when(ImageKeys.hasImage(Mockito.any(PaperCard.class))).thenReturn(true); //Mocking some more static stuff PowerMockito.mockStatic(Singletons.class); From 7b70a34da08c60be7e5df1eafce766f45d88a599 Mon Sep 17 00:00:00 2001 From: leriomaggio Date: Wed, 25 Aug 2021 18:44:59 +0100 Subject: [PATCH 10/23] further optimisation by using iterator rather than forloop with if-clauses --- .../src/main/java/forge/card/CardDb.java | 44 +++++++++---------- .../forge/card/CardDbPerformanceTests.java | 8 ++++ 2 files changed, 30 insertions(+), 22 deletions(-) diff --git a/forge-core/src/main/java/forge/card/CardDb.java b/forge-core/src/main/java/forge/card/CardDb.java index c57fb351c73..8efffe05ec5 100644 --- a/forge-core/src/main/java/forge/card/CardDb.java +++ b/forge-core/src/main/java/forge/card/CardDb.java @@ -90,6 +90,12 @@ public final class CardDb implements ICardDatabase, IDeckGenPool { this.collectorNumber = collectorNumber; } + public static String compose(String cardName, boolean isFoil){ + if (isFoil) + return cardName+foilSuffix; + return cardName; + } + public static String compose(String cardName, String setCode) { setCode = setCode != null ? setCode : ""; cardName = cardName != null ? cardName : ""; @@ -462,15 +468,15 @@ public final class CardDb implements ICardDatabase, IDeckGenPool { } // 2. Card lookup in edition with specified filter didn't work. - // So now check whether the cards exists in the DB first, + // So now check whether the cards exist in the DB first, // and select pick the card based on current SetPreference policy as a fallback Collection cards = getAllCards(request.cardName); if (cards.isEmpty()) // Never null being this a view in MultiMap return null; // Either No Edition has been specified OR as a fallback in case of any error! // get card using the default card art preference - result = getCardFromEditions(request.cardName, this.defaultCardArtPreference, request.artIndex); - return result != null && request.isFoil ? result.getFoiled() : result; + String cardRequest = CardRequest.compose(request.cardName, request.isFoil); + return getCardFromEditions(cardRequest, this.defaultCardArtPreference, request.artIndex); } /* @@ -567,8 +573,8 @@ public final class CardDb implements ICardDatabase, IDeckGenPool { } @Override - public PaperCard getCardFromEditions(final String cardName, final CardArtPreference artPreference, int artIndex) { - return this.tryToGetCardFromEditions(cardName, artPreference, artIndex); + public PaperCard getCardFromEditions(final String cardInfo, final CardArtPreference artPreference, int artIndex) { + return this.tryToGetCardFromEditions(cardInfo, artPreference, artIndex); } /* @@ -651,7 +657,6 @@ public final class CardDb implements ICardDatabase, IDeckGenPool { /* 2. Retrieve cards based of [Frame]Set Preference ================================================ */ - // Collect the list of all editions found for target card List cardEditions = new ArrayList<>(); Map candidatesCard = new HashMap<>(); @@ -659,7 +664,7 @@ public final class CardDb implements ICardDatabase, IDeckGenPool { if (card.getArtIndex() != cr.artIndex) continue; String setCode = card.getEdition(); - CardEdition ed = null; + CardEdition ed; if (setCode.equals(CardEdition.UNKNOWN.getCode())) ed = CardEdition.UNKNOWN; else @@ -669,6 +674,9 @@ public final class CardDb implements ICardDatabase, IDeckGenPool { candidatesCard.put(setCode, card); } } + if (cardEditions.isEmpty()) + return null; // nothing to do + // Filter Cards Editions based on set preferences List acceptedEditions = Lists.newArrayList(Iterables.filter(cardEditions, new Predicate() { @Override @@ -693,22 +701,14 @@ public final class CardDb implements ICardDatabase, IDeckGenPool { Collections.reverse(acceptedEditions); // newest editions first } - PaperCard candidate = null; - for (CardEdition ed : acceptedEditions) { - PaperCard cardFromSet = candidatesCard.get(ed.getCode()); //getCardFromSet(cr.cardName, ed, cr.artIndex, cr.isFoil); - if (candidate == null) - // save the first card found, as the last backup in case no other candidate *with image* will be found - candidate = cardFromSet; - - if (cardFromSet != null && cardFromSet.hasImage()) { - candidate = cardFromSet; - break; // we're done here: found card **with Image** - } + final Iterator editionIterator = acceptedEditions.iterator(); + CardEdition ed = editionIterator.next(); + PaperCard candidate = candidatesCard.get(ed.getCode()); + while (!candidate.hasImage() && editionIterator.hasNext()) { + ed = editionIterator.next(); + candidate = candidatesCard.get(ed.getCode()); } - if (candidate == null) - return null; - - //If any, we're sure that at least one candidate is always returned nevertheless it has image or not + //If any, we're sure that at least one candidate is always returned despite it having any image return cr.isFoil ? candidate.getFoiled() : candidate; } diff --git a/forge-gui-desktop/src/test/java/forge/card/CardDbPerformanceTests.java b/forge-gui-desktop/src/test/java/forge/card/CardDbPerformanceTests.java index 2e8129862c0..5393335fd18 100644 --- a/forge-gui-desktop/src/test/java/forge/card/CardDbPerformanceTests.java +++ b/forge-gui-desktop/src/test/java/forge/card/CardDbPerformanceTests.java @@ -83,4 +83,12 @@ public class CardDbPerformanceTests extends CardDbTestCase { assertNotNull(card); } } + + @Test + public void testGetCardFullDbLegacyImplementationToProfile(){ + for (String name : this.fullDbCardNames) { + PaperCard card = this.legacyCardDb.getCard(name); + assertNotNull(card); + } + } } From f507edada5b04880bf1c2d3e5503ffbc788b337f Mon Sep 17 00:00:00 2001 From: leriomaggio Date: Wed, 25 Aug 2021 18:49:03 +0100 Subject: [PATCH 11/23] Same optimisation using iterator on getCardFromSet --- forge-core/src/main/java/forge/card/CardDb.java | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/forge-core/src/main/java/forge/card/CardDb.java b/forge-core/src/main/java/forge/card/CardDb.java index 8efffe05ec5..ee51b916bbd 100644 --- a/forge-core/src/main/java/forge/card/CardDb.java +++ b/forge-core/src/main/java/forge/card/CardDb.java @@ -536,17 +536,13 @@ public final class CardDb implements ICardDatabase, IDeckGenPool { if (candidates.isEmpty()) return null; - PaperCard candidate = candidates.get(0); + Iterator candidatesIterator = candidates.iterator(); + PaperCard candidate = candidatesIterator.next(); // Before returning make sure that actual candidate has Image. // If not, try to replace current candidate with one having image, // so to align this implementation with old one. - if (!candidate.hasImage()) { - for (PaperCard card : candidates) { - if (card.hasImage()) { - candidate = card; - break; // found, ready to go - } - } + while (!candidate.hasImage() && candidatesIterator.hasNext()) { + candidate = candidatesIterator.next(); } return isFoil ? candidate.getFoiled() : candidate; } From 02a241de12f604593a0038fbb68368c138f79643 Mon Sep 17 00:00:00 2001 From: leriomaggio Date: Wed, 25 Aug 2021 19:03:38 +0100 Subject: [PATCH 12/23] worst case benchmark forcing all candidates with no image! --- .../src/test/java/forge/card/CardDbPerformanceTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/forge-gui-desktop/src/test/java/forge/card/CardDbPerformanceTests.java b/forge-gui-desktop/src/test/java/forge/card/CardDbPerformanceTests.java index 5393335fd18..e1704651927 100644 --- a/forge-gui-desktop/src/test/java/forge/card/CardDbPerformanceTests.java +++ b/forge-gui-desktop/src/test/java/forge/card/CardDbPerformanceTests.java @@ -13,7 +13,7 @@ import java.util.TreeSet; import static org.testng.Assert.assertNotNull; -public class CardDbPerformanceTests extends CardDbTestCase { +public class CardDbPerformanceTests extends CardDbTestWithNoImage { private Set fullDbCardNames = new TreeSet<>(); From bccf6e0a27a05bc07ea3880f70e85d4f2306b83a Mon Sep 17 00:00:00 2001 From: leriomaggio Date: Wed, 25 Aug 2021 19:39:54 +0100 Subject: [PATCH 13/23] new method with tests to get all cards with a filter predicate. New method in Interface API to get all cards in a given setCode. This has changed the implementation of getArtCount avoiding to iterate over all the possible card prints. --- .../src/main/java/forge/card/CardDb.java | 26 +++++++----- .../main/java/forge/card/ICardDatabase.java | 1 + .../test/java/forge/card/CardDbTestCase.java | 42 +++++++++++++++++++ 3 files changed, 58 insertions(+), 11 deletions(-) diff --git a/forge-core/src/main/java/forge/card/CardDb.java b/forge-core/src/main/java/forge/card/CardDb.java index ee51b916bbd..6338021bbaa 100644 --- a/forge-core/src/main/java/forge/card/CardDb.java +++ b/forge-core/src/main/java/forge/card/CardDb.java @@ -29,6 +29,7 @@ import forge.util.Lang; import forge.util.TextUtil; import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.tuple.Pair; +import org.checkerframework.checker.nullness.compatqual.NullableDecl; import java.util.*; import java.util.Map.Entry; @@ -721,18 +722,16 @@ public final class CardDb implements ICardDatabase, IDeckGenPool { } @Override - public int getArtCount(String cardName, String setName) { - if (cardName == null || setName == null) + public int getArtCount(String cardName, String setCode) { + if (cardName == null || setCode == null) return 0; - Collection cards = getAllCards(cardName); - if (null == cards || cards.size() == 0) - return 0; - int artCount = 0; - for (PaperCard pc : cards) { - if (pc.getEdition().equalsIgnoreCase(setName)) - artCount++; - } - return artCount; + Collection cardsInSet = getAllCards(cardName, new Predicate() { + @Override + public boolean apply(PaperCard card) { + return card.getEdition().equalsIgnoreCase(setCode); + } + }); + return cardsInSet.size(); } // returns a list of all cards from their respective latest (or preferred) editions @@ -839,6 +838,11 @@ public final class CardDb implements ICardDatabase, IDeckGenPool { return Lists.newArrayList(Iterables.filter(getAllCards(), predicate)); } + @Override + public List getAllCards(final String cardName, Predicate predicate){ + return Lists.newArrayList(Iterables.filter(getAllCards(cardName), predicate)); + } + /** * Returns a modifiable list of cards matching the given predicate */ diff --git a/forge-core/src/main/java/forge/card/ICardDatabase.java b/forge-core/src/main/java/forge/card/ICardDatabase.java index 64646109a46..b158404d320 100644 --- a/forge-core/src/main/java/forge/card/ICardDatabase.java +++ b/forge-core/src/main/java/forge/card/ICardDatabase.java @@ -79,6 +79,7 @@ public interface ICardDatabase extends Iterable { Collection getAllCards(); Collection getAllCards(String cardName); Collection getAllCards(Predicate predicate); + Collection getAllCards(String cardName,Predicate predicate); Collection getAllCards(CardEdition edition); Collection getUniqueCards(); diff --git a/forge-gui-desktop/src/test/java/forge/card/CardDbTestCase.java b/forge-gui-desktop/src/test/java/forge/card/CardDbTestCase.java index 8a30919020a..3853918951c 100644 --- a/forge-gui-desktop/src/test/java/forge/card/CardDbTestCase.java +++ b/forge-gui-desktop/src/test/java/forge/card/CardDbTestCase.java @@ -1,5 +1,6 @@ package forge.card; +import com.google.common.base.Predicate; import forge.StaticData; import forge.item.IPaperCard; import forge.item.PaperCard; @@ -10,7 +11,10 @@ import org.testng.annotations.Test; import java.text.ParseException; import java.text.SimpleDateFormat; import java.time.Instant; +import java.util.ArrayList; +import java.util.Arrays; import java.util.Date; +import java.util.List; import static org.testng.Assert.*; @@ -101,6 +105,44 @@ public class CardDbTestCase extends ForgeCardMockTestCase { this.legacyCardDb = new LegacyCardDb(data.getCommonCards().getAllCards(), data.getEditions()); } + /* + * TEST FOR GET ALL CARDS + */ + + @Test + public void testGetAllCardsWithName(){ + List allCounterSpellPrints = this.cardDb.getAllCards(this.cardNameCounterspell); + assertNotNull(allCounterSpellPrints); + for (PaperCard card : allCounterSpellPrints) + assertEquals(card.getName(), this.cardNameCounterspell); + } + + @Test + public void testGetAllCardsThatWerePrintedInSets(){ + List allowedSets = new ArrayList<>(); + allowedSets.add(this.latestArtShivanDragonEdition); + Predicate wasPrinted = (Predicate) this.cardDb.wasPrintedInSets(allowedSets); + List allCardsInSet = this.cardDb.getAllCards(wasPrinted); + assertNotNull(allCardsInSet); + } + + @Test void testGetAllCardsOfaGivenNameAndLegalInSets(){ + List allowedSets = new ArrayList<>(Arrays.asList(this.editionsCounterspell)); + Predicate printedInSets = (Predicate) this.cardDb.wasPrintedInSets(allowedSets); + List allCounterSpellsInSets = this.cardDb.getAllCards(this.cardNameCounterspell, printedInSets); + assertNotNull(allCounterSpellsInSets); + assertTrue(allCounterSpellsInSets.size() > 0); + assertTrue(allCounterSpellsInSets.size() > 1); + for (PaperCard card : allCounterSpellsInSets) { + assertEquals(card.getName(), this.cardNameCounterspell); + } + } + + + /* + * TEST FOR CARD RETRIEVAL METHODS + */ + @Test public void testGetCardByName() { PaperCard legacyCard = this.legacyCardDb.getCard(cardNameShivanDragon); From fed48e01c2a6997326083c8adfb189dbe29d1717 Mon Sep 17 00:00:00 2001 From: leriomaggio Date: Wed, 25 Aug 2021 21:03:15 +0100 Subject: [PATCH 14/23] Pushed even further on improvements, making it run even faster than legacy The huge improvement is due to a re-use of a getAllCards(Name, Predicate) method I started implementing into another branch for DeckImporter - re-using this method internally resolves a **lot** of duplicated iterations, and checks, which made the whole method to run even faster than the original legacy implementation! An extra test method has been added to verify the corner case of querying for a card from editions with a very high (and weird) artIndex (like a land) - making sure that exact card is finally returned! --- .../src/main/java/forge/card/CardDb.java | 27 ++++++++++++------- .../forge/card/CardDbPerformanceTests.java | 2 +- .../test/java/forge/card/CardDbTestCase.java | 18 +++++++++++++ 3 files changed, 36 insertions(+), 11 deletions(-) diff --git a/forge-core/src/main/java/forge/card/CardDb.java b/forge-core/src/main/java/forge/card/CardDb.java index 6338021bbaa..d00310d38ce 100644 --- a/forge-core/src/main/java/forge/card/CardDb.java +++ b/forge-core/src/main/java/forge/card/CardDb.java @@ -517,9 +517,7 @@ public final class CardDb implements ICardDatabase, IDeckGenPool { cardName = cardNameRequest.cardName; isFoil = isFoil || cardNameRequest.isFoil; - List cards = getAllCards(cardName); - // Look for Code or Code2 to make the retrieval more robust - List candidates = Lists.newArrayList(Iterables.filter(cards, new Predicate() { + List candidates = getAllCards(cardName, new Predicate() { @Override public boolean apply(PaperCard c) { boolean artIndexFilter = true; @@ -533,7 +531,7 @@ public final class CardDb implements ICardDatabase, IDeckGenPool { collectorNumberFilter = (c.getCollectorNumber().equals(collectorNumber)); return setFilter && artIndexFilter && collectorNumberFilter; } - })); + }); if (candidates.isEmpty()) return null; @@ -637,11 +635,13 @@ public final class CardDb implements ICardDatabase, IDeckGenPool { if (cr.artIndex != artIndex && artIndex > IPaperCard.DEFAULT_ART_INDEX ) cr.artIndex = artIndex; // 2nd cond. is to verify that some actual value has been passed in. - List cards = getAllCards(cr.cardName); + List cards; if (releaseDate != null) { - cards = Lists.newArrayList(Iterables.filter(cards, new Predicate() { + cards = getAllCards(cr.cardName, new Predicate() { @Override public boolean apply(PaperCard c) { + if (c.getArtIndex() != cr.artIndex) + return false; // not interested anyway! CardEdition ed = editions.get(c.getEdition()); if (ed == null) return false; if (releasedBeforeFlag) @@ -649,8 +649,17 @@ public final class CardDb implements ICardDatabase, IDeckGenPool { else return ed.getDate().after(releaseDate); } - })); - } + }); + } else // filter candidates based on requested artIndex + cards = getAllCards(cr.cardName, new Predicate() { + @Override + public boolean apply(PaperCard card) { + return card.getArtIndex() == cr.artIndex; + } + }); + + if (cards.size() == 1) // if only one candidate, there much else we should do + return cr.isFoil ? cards.get(0).getFoiled() : cards.get(0); /* 2. Retrieve cards based of [Frame]Set Preference ================================================ */ @@ -658,8 +667,6 @@ public final class CardDb implements ICardDatabase, IDeckGenPool { List cardEditions = new ArrayList<>(); Map candidatesCard = new HashMap<>(); for (PaperCard card : cards) { - if (card.getArtIndex() != cr.artIndex) - continue; String setCode = card.getEdition(); CardEdition ed; if (setCode.equals(CardEdition.UNKNOWN.getCode())) diff --git a/forge-gui-desktop/src/test/java/forge/card/CardDbPerformanceTests.java b/forge-gui-desktop/src/test/java/forge/card/CardDbPerformanceTests.java index e1704651927..5393335fd18 100644 --- a/forge-gui-desktop/src/test/java/forge/card/CardDbPerformanceTests.java +++ b/forge-gui-desktop/src/test/java/forge/card/CardDbPerformanceTests.java @@ -13,7 +13,7 @@ import java.util.TreeSet; import static org.testng.Assert.assertNotNull; -public class CardDbPerformanceTests extends CardDbTestWithNoImage { +public class CardDbPerformanceTests extends CardDbTestCase { private Set fullDbCardNames = new TreeSet<>(); diff --git a/forge-gui-desktop/src/test/java/forge/card/CardDbTestCase.java b/forge-gui-desktop/src/test/java/forge/card/CardDbTestCase.java index 3853918951c..9bad0be201c 100644 --- a/forge-gui-desktop/src/test/java/forge/card/CardDbTestCase.java +++ b/forge-gui-desktop/src/test/java/forge/card/CardDbTestCase.java @@ -2061,6 +2061,24 @@ public class CardDbTestCase extends ForgeCardMockTestCase { assertEquals(legacyAinokCard, ainokCard); } + @Test + public void testGetIslandsFromEditionsWithSpecificArtIndex(){ + String cardName = "Island"; + assertEquals(this.cardDb.getCardArtPreference(), CardDb.CardArtPreference.LATEST_ART_ALL_EDITIONS); + PaperCard islandLatest = this.cardDb.getCardFromEditions(cardName, CardDb.CardArtPreference.LATEST_ART_ALL_EDITIONS, 12); + assertNotNull(islandLatest); + assertEquals(islandLatest.getName(), "Island"); + assertEquals(islandLatest.getEdition(), "SLD"); + assertEquals(islandLatest.getArtIndex(), 12); + + // PALP + PaperCard islandOriginal = this.cardDb.getCardFromEditions(cardName, CardDb.CardArtPreference.ORIGINAL_ART_CORE_EXPANSIONS_REPRINT_ONLY, 12); + assertNotNull(islandOriginal); + assertEquals(islandOriginal.getName(), "Island"); + assertEquals(islandOriginal.getEdition(), "SLD"); + assertEquals(islandOriginal.getArtIndex(), 12); + } + } From 3acf7384e3d69aa5d641c09efad2d745fcbba4ec Mon Sep 17 00:00:00 2001 From: leriomaggio Date: Thu, 26 Aug 2021 02:47:35 +0100 Subject: [PATCH 15/23] FIXED Sneaky bug that capped art index in DB requests to one digit! (Added tests in cardDb and CardRequest to cover the case!) --- forge-core/src/main/java/forge/card/CardDb.java | 2 +- .../src/test/java/forge/card/CardDbTestCase.java | 6 ++++++ .../src/test/java/forge/card/CardRequestTestCase.java | 11 +++++++++++ 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/forge-core/src/main/java/forge/card/CardDb.java b/forge-core/src/main/java/forge/card/CardDb.java index d00310d38ce..756d5a71ce4 100644 --- a/forge-core/src/main/java/forge/card/CardDb.java +++ b/forge-core/src/main/java/forge/card/CardDb.java @@ -142,7 +142,7 @@ public final class CardDb implements ICardDatabase, IDeckGenPool { } private static boolean isArtIndex(String s) { - return StringUtils.isNumeric(s) && s.length() == 1; + return StringUtils.isNumeric(s); } private static boolean isSetCode(String s) { diff --git a/forge-gui-desktop/src/test/java/forge/card/CardDbTestCase.java b/forge-gui-desktop/src/test/java/forge/card/CardDbTestCase.java index 9bad0be201c..e2bd0bdff47 100644 --- a/forge-gui-desktop/src/test/java/forge/card/CardDbTestCase.java +++ b/forge-gui-desktop/src/test/java/forge/card/CardDbTestCase.java @@ -2079,6 +2079,12 @@ public class CardDbTestCase extends ForgeCardMockTestCase { assertEquals(islandOriginal.getArtIndex(), 12); } + @Test + public void testMaxArtCountForBasicLand(){ + int maxArtIndex = this.cardDb.getMaxArtIndex("Island"); + assertEquals(maxArtIndex, 13); + } + } diff --git a/forge-gui-desktop/src/test/java/forge/card/CardRequestTestCase.java b/forge-gui-desktop/src/test/java/forge/card/CardRequestTestCase.java index 4279de97001..bf29efa4f84 100644 --- a/forge-gui-desktop/src/test/java/forge/card/CardRequestTestCase.java +++ b/forge-gui-desktop/src/test/java/forge/card/CardRequestTestCase.java @@ -215,4 +215,15 @@ public class CardRequestTestCase { assertNotEquals(request.artIndex, newRequest.artIndex); } + @Test + public void testCreatingCardRequestWithArtIndexGreaterThanNine(){ + String requestString = CardRequest.compose("Island", "SLD", 13); + CardRequest request = CardRequest.fromString(requestString); + + assertEquals(request.cardName, "Island"); + assertEquals(request.edition, "SLD"); + assertEquals(request.artIndex, 13); + assertEquals(request.collectorNumber, IPaperCard.NO_COLLECTOR_NUMBER); + } + } \ No newline at end of file From 6fef56c8b72425604329e15e6ac22d78a2515364 Mon Sep 17 00:00:00 2001 From: leriomaggio Date: Thu, 26 Aug 2021 02:48:15 +0100 Subject: [PATCH 16/23] Simplified isDirectoryWithFiles reducing I/O ops (as per Grimm suggestions!) --- forge-core/src/main/java/forge/util/FileUtil.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/forge-core/src/main/java/forge/util/FileUtil.java b/forge-core/src/main/java/forge/util/FileUtil.java index 64cbdfedf1b..a969b6367b5 100644 --- a/forge-core/src/main/java/forge/util/FileUtil.java +++ b/forge-core/src/main/java/forge/util/FileUtil.java @@ -78,8 +78,10 @@ public final class FileUtil { } public static boolean isDirectoryWithFiles(final String path) { + if (path == null) return false; final File f = new File(path); - return f.exists() && f.isDirectory() && f.list().length > 0; + final String[] fileList = f.list(); + return fileList!=null && fileList.length > 0; } public static boolean ensureDirectoryExists(final String path) { From 119ac88ad5d627d2b78243099b5282ae53ad4f60 Mon Sep 17 00:00:00 2001 From: leriomaggio Date: Thu, 26 Aug 2021 02:48:57 +0100 Subject: [PATCH 17/23] PaperCard now includes an ImageKeyFromSet attr. cached for later re-use! --- forge-core/src/main/java/forge/item/PaperCard.java | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/forge-core/src/main/java/forge/item/PaperCard.java b/forge-core/src/main/java/forge/item/PaperCard.java index 3441fe833b8..6cf473f85ed 100644 --- a/forge-core/src/main/java/forge/item/PaperCard.java +++ b/forge-core/src/main/java/forge/item/PaperCard.java @@ -25,6 +25,7 @@ import forge.card.CardEdition; import forge.card.CardRarity; import forge.card.CardRules; import forge.util.CardTranslation; +import forge.util.ImageUtil; import forge.util.Localizer; import forge.util.TextUtil; @@ -140,6 +141,13 @@ public final class PaperCard implements Comparable, InventoryItemFro return hasImage; } + private String imageKeyFromSet = null; + public String getImageKeyFromSet() { + if (this.imageKeyFromSet == null) + this.imageKeyFromSet = ImageUtil.getImageKey(this, false, true); + return imageKeyFromSet; + } + /** * Lambda to get rules for selects from list of printed cards. */ From dde96156be4173d6cece84792e960a64194e56aa Mon Sep 17 00:00:00 2001 From: leriomaggio Date: Thu, 26 Aug 2021 02:49:26 +0100 Subject: [PATCH 18/23] Simplified condition on artIndex when retrieving relative path for card image. --- forge-core/src/main/java/forge/util/ImageUtil.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/forge-core/src/main/java/forge/util/ImageUtil.java b/forge-core/src/main/java/forge/util/ImageUtil.java index d8b67d03b2c..5b2c0488e6a 100644 --- a/forge-core/src/main/java/forge/util/ImageUtil.java +++ b/forge-core/src/main/java/forge/util/ImageUtil.java @@ -56,7 +56,7 @@ public class ImageUtil { int artIdx = cp.getArtIndex() - 1; if (hasManyPictures) { if (cntPictures <= artIdx) // prevent overflow - artIdx = cntPictures == 0 ? 0 : artIdx % cntPictures; + artIdx = artIdx % cntPictures; s.append(artIdx + 1); } From c2ff9cf3c93f795d3d140fd1952940d8e86fe70f Mon Sep 17 00:00:00 2001 From: leriomaggio Date: Thu, 26 Aug 2021 02:50:11 +0100 Subject: [PATCH 19/23] Re-using default ImageKeyFromSet from PaperCard instance (This sort of things should be looked for, and optimised!) --- forge-gui-desktop/src/main/java/forge/ImageCache.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/forge-gui-desktop/src/main/java/forge/ImageCache.java b/forge-gui-desktop/src/main/java/forge/ImageCache.java index 5fc44718e67..709346de623 100644 --- a/forge-gui-desktop/src/main/java/forge/ImageCache.java +++ b/forge-gui-desktop/src/main/java/forge/ImageCache.java @@ -184,7 +184,7 @@ public class ImageCache { if (useArtCrop) { if (ipc != null && ipc.getRules().getSplitType() == CardSplitType.Flip) { // Art crop will always use front face as image key for flip cards - imageKey = ImageUtil.getImageKey((PaperCard) ipc, false, true); + imageKey = ((PaperCard) ipc).getImageKeyFromSet(); // ImageUtil.getImageKey((PaperCard) ipc, false, true); } imageKey = TextUtil.fastReplace(imageKey, ".full", ".artcrop"); } From 4fc0234ccc3724ebcde76da9c11a51e688f2cdee Mon Sep 17 00:00:00 2001 From: leriomaggio Date: Thu, 26 Aug 2021 03:02:52 +0100 Subject: [PATCH 20/23] Improved performance w/ cached content for ImageKeys.hasImage THe content of the setFolder is cached, filtering only images and key prefixes. Working on prefixes only to get rid of all .full or .fullborder possible variations. --- forge-core/src/main/java/forge/ImageKeys.java | 30 +++++++++++++++---- 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/forge-core/src/main/java/forge/ImageKeys.java b/forge-core/src/main/java/forge/ImageKeys.java index fe9360ab7a8..77553a9bd53 100644 --- a/forge-core/src/main/java/forge/ImageKeys.java +++ b/forge-core/src/main/java/forge/ImageKeys.java @@ -2,13 +2,11 @@ package forge; import forge.item.PaperCard; import forge.util.FileUtil; -import forge.util.ImageUtil; import forge.util.TextUtil; import org.apache.commons.lang3.StringUtils; import java.io.File; -import java.util.HashMap; -import java.util.Map; +import java.util.*; public final class ImageKeys { public static final String CARD_PREFIX = "c:"; @@ -68,9 +66,8 @@ public final class ImageKeys { } public static File getImageFile(String key) { - if (StringUtils.isEmpty(key)) { + if (StringUtils.isEmpty(key)) return null; - } final String dir; final String filename; @@ -220,14 +217,35 @@ public final class ImageKeys { //shortcut for determining if a card image exists for a given card //should only be called from PaperCard.hasImage() + static HashMap> cachedContent=new HashMap<>(); public static boolean hasImage(PaperCard pc) { Boolean editionHasImage = editionImageLookup.get(pc.getEdition()); if (editionHasImage == null) { String setFolder = getSetFolder(pc.getEdition()); editionHasImage = FileUtil.isDirectoryWithFiles(CACHE_CARD_PICS_DIR + setFolder); editionImageLookup.put(pc.getEdition(), editionHasImage); + if (editionHasImage){ + File f = new File(CACHE_CARD_PICS_DIR + setFolder); // no need to check this, otherwise editionHasImage would be false! + HashSet setFolderContent = new HashSet<>(); + for (String filename : Arrays.asList(f.list())) { + // TODO: should this use FILE_EXTENSIONS ? + if (!filename.endsWith(".jpg") && !filename.endsWith(".png")) + continue; // not image - not interested + setFolderContent.add(filename.split("\\.")[0]); // get rid of any full or fullborder + } + cachedContent.put(setFolder, setFolderContent); + } } + String[] keyParts = pc.getImageKeyFromSet().split(File.separator); + HashSet content = cachedContent.getOrDefault(keyParts[0], null); //avoid checking for file if edition doesn't have any images - return editionHasImage && getImageFile(ImageUtil.getImageKey(pc, false, true)) != null; + return editionHasImage && hitCache(content, keyParts[1]); + } + + private static boolean hitCache(HashSet cache, String filename){ + if (cache == null || cache.isEmpty()) + return false; + final String keyPrefix = filename.split("\\.")[0]; + return cache.contains(keyPrefix); } } From 26ec34e01945a617ca79eb6bf0e2836ab3f3361c Mon Sep 17 00:00:00 2001 From: leriomaggio Date: Thu, 26 Aug 2021 07:23:09 +0100 Subject: [PATCH 21/23] Updated tests and capped artIndex to be two digits! --- forge-core/src/main/java/forge/card/CardDb.java | 2 +- .../src/test/java/forge/card/CardRequestTestCase.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/forge-core/src/main/java/forge/card/CardDb.java b/forge-core/src/main/java/forge/card/CardDb.java index 756d5a71ce4..95e9fa57c3b 100644 --- a/forge-core/src/main/java/forge/card/CardDb.java +++ b/forge-core/src/main/java/forge/card/CardDb.java @@ -142,7 +142,7 @@ public final class CardDb implements ICardDatabase, IDeckGenPool { } private static boolean isArtIndex(String s) { - return StringUtils.isNumeric(s); + return StringUtils.isNumeric(s) && s.length() <= 2 ; // only artIndex between 1-99 } private static boolean isSetCode(String s) { diff --git a/forge-gui-desktop/src/test/java/forge/card/CardRequestTestCase.java b/forge-gui-desktop/src/test/java/forge/card/CardRequestTestCase.java index bf29efa4f84..444e55ab937 100644 --- a/forge-gui-desktop/src/test/java/forge/card/CardRequestTestCase.java +++ b/forge-gui-desktop/src/test/java/forge/card/CardRequestTestCase.java @@ -128,7 +128,7 @@ public class CardRequestTestCase { request = CardRequest.fromString(requestString); assertEquals(request.cardName, cardName); assertEquals(request.edition, edition); - assertEquals(request.artIndex, IPaperCard.DEFAULT_ART_INDEX); + assertEquals(request.artIndex, 20); assertEquals(request.collectorNumber, IPaperCard.NO_COLLECTOR_NUMBER); From 970d9b4b8d563dda5fc880178efed70978e74a84 Mon Sep 17 00:00:00 2001 From: leriomaggio Date: Thu, 26 Aug 2021 08:04:26 +0100 Subject: [PATCH 22/23] removed unused import --- forge-core/src/main/java/forge/card/CardDb.java | 1 - 1 file changed, 1 deletion(-) diff --git a/forge-core/src/main/java/forge/card/CardDb.java b/forge-core/src/main/java/forge/card/CardDb.java index 95e9fa57c3b..b0a390a896e 100644 --- a/forge-core/src/main/java/forge/card/CardDb.java +++ b/forge-core/src/main/java/forge/card/CardDb.java @@ -29,7 +29,6 @@ import forge.util.Lang; import forge.util.TextUtil; import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.tuple.Pair; -import org.checkerframework.checker.nullness.compatqual.NullableDecl; import java.util.*; import java.util.Map.Entry; From fb3db3d83ad53b71139048814219e4e89c4959a7 Mon Sep 17 00:00:00 2001 From: leriomaggio Date: Thu, 26 Aug 2021 08:05:53 +0100 Subject: [PATCH 23/23] removed unused import --- .../src/test/java/forge/card/CardDbPerformanceTests.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/forge-gui-desktop/src/test/java/forge/card/CardDbPerformanceTests.java b/forge-gui-desktop/src/test/java/forge/card/CardDbPerformanceTests.java index 5393335fd18..e1d409c6147 100644 --- a/forge-gui-desktop/src/test/java/forge/card/CardDbPerformanceTests.java +++ b/forge-gui-desktop/src/test/java/forge/card/CardDbPerformanceTests.java @@ -1,9 +1,6 @@ package forge.card; -import forge.ImageKeys; import forge.item.PaperCard; -import org.mockito.Mockito; -import org.powermock.api.mockito.PowerMockito; import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test;