Slow down

[Sloth]

I noticed a serious slowdown of the new version this week and was able to track it down to revision r3773 by Rob. I'm not sure if it is the change made to ImageCache.java or using getCurSetCode() in CardDetailPanel. Maybe you have an idea what could cause it Rob? The game becomes nearly unplayable on my machine, when 10 cards are on the battlefield.

[Chris H.]

I just played a game using rev 3865 and was able to reach a stalemate. I allowed the computer and myself to put a large number of cards into play, dozens? I noticed a slight slow down with this number of cards. But nothing abnormal.

[Rob Cashwalker]

Yeah, I just played a couple games where I let the computer fill up his side of the table, nothing noticeable in speed.

[Sloth]

Do you use card pictures? because I don't use any. Maybe the game searches for them all the time?

[Rob Cashwalker]

Why wouldn't you use images?

That's a good point, the ImageCache change is likely the cause. It only attempts to load a picture the first time it's displayed. If it never finds an image, it seems like it won't try again for that card object for the rest of the game.

[Jaedayr]

I also have experienced a slow down in the past week. Could it be related to the processing of the messages in this post? viewtopic.php?f=26&t=787&start=3525#p46951

[Chris H.]

I removed the pics from my /res/pics/ folder and was able to re-create the problem that Sloth mentioned.

Hmmm, I guess that some of the people who are new to forge might not know how to download the pics when they first try the game. Having the game quickly slow down might be a turn off. Will they trash the game before they figure out how to download the pics?

I do not have any good answers at this time.

[Hellfish]

As Rob pointed out, the problem is in ImageCache. Everytime a new card image is loaded (i.e. every time one enters the playing field/gets tapped/is scaled up/is turned face up), if it's a card whose image can't be found, a method in ImageCache throws an exception that that is immideately written to the log file. Happens a lot, too, netted me about 300 MB of log in just 3 short matches and I still have most images! I modded the code to keep a list of the images it already has thrown an exception for,which fixed it, but I'm hesitant to commit it because I'm not 100% it doesn't break something else or if there is a better way to do it. Rob, you know the code well, could you have a look?

Code: Select all

//This is new
public static ArrayList<String> isReportedMissing = new ArrayList<String>();
   
    static {
        imageCache = new MapMaker().softValues().makeComputingMap(new Function<String, BufferedImage>() {
            public BufferedImage apply(String key) {
                try {
                    //DEBUG
                    /*System.out.printf("Currently %d %s in the cache%n", imageCache.size(),
                            imageCache.size() == 1? "image":"images");*/
                   //DEBUG
                    //System.out.printf("New Image for key: %s%n", key);
                    if(key.endsWith(NORMAL)) {
                        //normal
                        key = key.substring(0, key.length() - NORMAL.length());
                        return getNormalSizeImage(imageCache.get(key));
                    } else if(key.endsWith(TAPPED)) {
                        //tapped
                        key = key.substring(0, key.length() - TAPPED.length());
                        return getTappedSizeImage(imageCache.get(key));
                    }
                    Matcher m = FULL_SIZE.matcher(key);
                   
                    if(m.matches()) {
                        //full size
                        key = m.group(1);
                        return getFullSizeImage(imageCache.get(key), parseDouble(m.group(2)));
                    } else {
                        //original
                        File path;
                        if(key.endsWith(TOKEN)) {
                            key = key.substring(0, key.length() - TOKEN.length());
                            path = ForgeProps.getFile(IMAGE_TOKEN);
                        } else path = ForgeProps.getFile(IMAGE_BASE);
                       
                        File file = null;
                        file = new File(path, key + ".jpg");
                        if(!file.exists()) {
                           //DEBUG
                            //System.out.println("File not found, no image created: " + file);
                            //return null;
                           //String newKey = getKey()
                           
                        }
                        BufferedImage image = ImageUtil.getImage(file);
                        return image;
                    }
                } catch(Exception ex) {
                   //DEBUG
                    //System.err.println("Exception, no image created");
                        //Surrounding if-clause,list add and return null is new
                   if(!isReportedMissing.contains(key))
                   {
                      isReportedMissing.add(key);
                      if(ex instanceof ComputationException) throw (ComputationException) ex;
                       else throw new ComputationException(ex);
                   }
                   return null;
                }
            }
        });
    }

I'm mostly worried about returning null there.. Or is that the result if an exception is thrown anyway?

[silly freak]

MapMaker doesn't like null return values, but that's absolutely fine; see #getImage(String)

Code: Select all

} catch(NullPointerException ex) {
    //unfortunately NullOutputException, thrown when apply() returns null, is not public
    //NullOutputException is a subclass of NullPointerException
    //legitimate, happens when a card has no image
    return null;
}


[Rob Cashwalker]

I kind of figured out part of what's wrong. While the pics in play are only refreshed once, the blow-up is refreshed every time the mouse moves. I figured that out when I tried adding a number to the lands. It looked fine for the cards in hand, but as I moved onto them, the blow-up cycled through all images from the set.

I do know that I have to make the image filename testing and generation external to ImageCache. Let ImageCache do the job of loading and displaying the file, but determining IF and WHAT that file will be needs to be handled differently. I hesitate to check during startup, and launching a game takes a little while as it is, but it's as good a time as any.

Good this was caught now before the beta. When this is resolved, I will not be monkeying around with sets until afterwards.

[Snacko]

someone fucked this

Code: Select all

if(!file.exists()) {
                           //DEBUG
                            //System.out.println("File not found, no image created: " + file);
                            //return null;
                           //String newKey = getKey()


it should be

Code: Select all

if(!file.exists()) {
                           //DEBUG
                            //System.out.println("File not found, no image created: " + file);
                            return null;


then you just get the proper null value meaning no picture was found

still there's memory leak in game, you can just hover trough different cards and the memory will increase without end, this doesn't happen in deckeditor

[Rob Cashwalker]

Yeah, I guess I did that. I was originally going to handle searching for files as the response to the file not existing, but realized that that section of code has no reference to the Card object. That's why it ended up in the getKey block of code, which I now see is wrong too.

[Rob Cashwalker]

I've made a couple changes to my existing ImageCache code.

The first, is I put the (return null) back.

The second, is I use (card.getRandomPicture() + 1) as the number for lands. I now have a stable game window with multiple lands.

My gameplan:
GameAction.newGame:

Code: Select all

String PC = card.getSVar("PicCount");
                int n = 0;
                if (PC.matches("[0-9][0-9]?"))
                   n = Integer.parseInt(PC);
                if (n > 1)
                    card.setRandomPicture(generator.nextInt(n));
                if ((card.getSets().size() > 0) && card.getCurSetCode().equals(""))
                   card.setRandomSetCode();

I'll find some place to make a helper method to do the file searching, and eventually, attempt to spawn a download if the file isn't found. Then the Card object will have a filename property that ImageCache simply reads. "none" shall be a valid value for the filename property, which can map to a generic picture file... so that a file is never not found, in SOME form.

I don't know very much about this code really... I'm not sure how exactly to relieve the memory leak, but given how I noticed ImageCache.getKey is hammered on the mouse-over, that's gotta be related.

[Rob Cashwalker]

I just committed an update that should take care of the slow down issue.

I reverted the ImageCache, then made a couple changes to pull the file from a new property of the Card.

I added a buildFilename method to CardUtil to actually process the set image searching.

I added a getMostRecentSet method to the Card, to enable display of the most recent printing for DeckEditor.

If the set image can't be found, use none.jpg:

The attachment none.jpg is no longer available