[Patch] Forge inline card image acquirer
Moderators: timmermac, Blacksmith, KrazyTheFox, Agetian, friarsol, CCGHQ Admins
21 posts
• Page 1 of 2 • 1, 2
[Patch] Forge inline card image acquirer
by OpenSS » 03 Nov 2014, 02:26
Hi everyone.
I have created a patch for Forge which will acquire the card images on an "as required" basis.
This patch adds an extra instruction to the end of Forge's card image loading method.
If the card can not be found in the card image directory, it will attempt to acquire the image via mtgimage (higher resolution than the in-game downloader) and save it to the card image directory so it will be available for immediate and future use.
It will do this only if the image does not already exist and it will acquire the set image for that card (IE. It will differentiate between Armageddon 5ED and Armageddon 6ED and will only download each one once).
As the image will need to be downloaded before it can be displayed, there is a small delay the first time a card is ever displayed while the image is being acquired (This is especially noticeable when viewing an entire deck).
I, however, prefer this to clicking a button to download all the images and waiting hours for it to complete as with this patch you can play the game with card images straight away and the image is still saved for future use as well to prevent having to download it again.
This has been built against Forge Desktop version 1.5.29 and tested under Windows 8.1.
I have not made it platform specific, but I have not tested any other platforms either.
Installation instructions (requires 7-Zip):
1. Download the attached ImageKeys.zip
2. Unzip ImageKeys.class from the ImageKeys.zip file
3. Navigate to the Forge directory
4. If Forge is currently running, close it
5. Make a backup of the Forge Jar file ("forge-gui-desktop-1.5.29-jar-with-dependencies" at time of writing) so you can revert this patch if you wish.
6. Open the original (not the backup) Forge JAR file as an archive in 7-Zip. You should be able to right click it, hover over 7-Zip and select the top "Open archive" option
7. In 7-Zip, navigate into the "forge" folder
8. Drag the extracted ImageKeys.class file into the 7-Zip window. It should replace the existing ImageKeys.class file
9. Close 7-Zip
10. Start Forge in your normal way and test the patch (I use Deck Editor for this)
Hope someone gets some enjoyment from this,
OpenSS
----------
Ver 1.0: 2014-11-03 (6 Downloads)
Initial version
Ver 1.0.1: 2014-11-05
1. Move exception handling earlier in case Forge call fails
2. Change hard-coded folder separator to OS-independent separatorChar
3. Build against 1.5.29 not 1.5.30
I have created a patch for Forge which will acquire the card images on an "as required" basis.
This patch adds an extra instruction to the end of Forge's card image loading method.
If the card can not be found in the card image directory, it will attempt to acquire the image via mtgimage (higher resolution than the in-game downloader) and save it to the card image directory so it will be available for immediate and future use.
It will do this only if the image does not already exist and it will acquire the set image for that card (IE. It will differentiate between Armageddon 5ED and Armageddon 6ED and will only download each one once).
As the image will need to be downloaded before it can be displayed, there is a small delay the first time a card is ever displayed while the image is being acquired (This is especially noticeable when viewing an entire deck).
I, however, prefer this to clicking a button to download all the images and waiting hours for it to complete as with this patch you can play the game with card images straight away and the image is still saved for future use as well to prevent having to download it again.
This has been built against Forge Desktop version 1.5.29 and tested under Windows 8.1.
I have not made it platform specific, but I have not tested any other platforms either.
Installation instructions (requires 7-Zip):
1. Download the attached ImageKeys.zip
2. Unzip ImageKeys.class from the ImageKeys.zip file
3. Navigate to the Forge directory
4. If Forge is currently running, close it
5. Make a backup of the Forge Jar file ("forge-gui-desktop-1.5.29-jar-with-dependencies" at time of writing) so you can revert this patch if you wish.
6. Open the original (not the backup) Forge JAR file as an archive in 7-Zip. You should be able to right click it, hover over 7-Zip and select the top "Open archive" option
7. In 7-Zip, navigate into the "forge" folder
8. Drag the extracted ImageKeys.class file into the 7-Zip window. It should replace the existing ImageKeys.class file
9. Close 7-Zip
10. Start Forge in your normal way and test the patch (I use Deck Editor for this)
Hope someone gets some enjoyment from this,
OpenSS
----------
Ver 1.0: 2014-11-03 (6 Downloads)
Initial version
Ver 1.0.1: 2014-11-05
1. Move exception handling earlier in case Forge call fails
2. Change hard-coded folder separator to OS-independent separatorChar
3. Build against 1.5.29 not 1.5.30
- Attachments
-
ImageKeys-ver1.0.1.zip- (4.17 KiB) Downloaded 379 times
Last edited by OpenSS on 05 Nov 2014, 12:45, edited 2 times in total.
- OpenSS
- Posts: 5
- Joined: 03 Nov 2014, 01:35
- Has thanked: 0 time
- Been thanked: 0 time
Re: [Patch] Forge inline card image acquirer
by aSfSteve » 05 Nov 2014, 10:09
This is as really great idea, and I personally think that this should be implemented into the public build. However, after every game 1 my client freezes and I have to close the program. I can't get through any best of 3's and I had to revert to the original jar file. Fix this bug and I will continue using this nice little plugin. I'm surprised that no one has commented on this post yet, great work.
P.S. This seems to skip more cards than the pre-existing LQ image finder.
P.S. This seems to skip more cards than the pre-existing LQ image finder.
- aSfSteve
- Posts: 4
- Joined: 14 Sep 2014, 21:04
- Has thanked: 0 time
- Been thanked: 0 time
Re: [Patch] Forge inline card image acquirer
by OpenSS » 05 Nov 2014, 11:03
EDIT: New version uploaded above. Please try the new version
I have played many best of 3s running this without error (constructed deck mode).
Any chance you can start the jar from command line as this will print info as to what's happening and may tell me if my code is getting stuck on something (it prints every time it is called).
Easiest way to do this is to navigate to the Forge folder, Shift + Right Click on white space, and choose "Open command window here". Then use the following to launch the jar:
java -jar forge-gui-desktop-1.5.29-jar-with-dependencies.jar
(Adjust jar name/version accordingly)
Also, which Operating System are you using?
As for the skipped images, I need to map the set names/codes Forge uses to the ones mtgimage uses if they are different.
Please let me know if there are any sets or cards that don't work so I can map them (or check if mtgimage even has them)
Thanks,
OpenSS
I have played many best of 3s running this without error (constructed deck mode).
Any chance you can start the jar from command line as this will print info as to what's happening and may tell me if my code is getting stuck on something (it prints every time it is called).
Easiest way to do this is to navigate to the Forge folder, Shift + Right Click on white space, and choose "Open command window here". Then use the following to launch the jar:
java -jar forge-gui-desktop-1.5.29-jar-with-dependencies.jar
(Adjust jar name/version accordingly)
Also, which Operating System are you using?
As for the skipped images, I need to map the set names/codes Forge uses to the ones mtgimage uses if they are different.
Please let me know if there are any sets or cards that don't work so I can map them (or check if mtgimage even has them)
Thanks,
OpenSS
- OpenSS
- Posts: 5
- Joined: 03 Nov 2014, 01:35
- Has thanked: 0 time
- Been thanked: 0 time
Re: [Patch] Forge inline card image acquirer
by OpenSS » 09 Nov 2014, 04:05
As requested, here is the source diff for ver 1.0.1
Except for the imports, this is all in one block at around line 145.
Thanks,
OpenSS
Except for the imports, this is all in one block at around line 145.
Thanks,
OpenSS
- Attachments
-
- ImageKeys-Diff.txt
- (1.75 KiB) Downloaded 287 times
- OpenSS
- Posts: 5
- Joined: 03 Nov 2014, 01:35
- Has thanked: 0 time
- Been thanked: 0 time
Re: [Patch] Forge inline card image acquirer
by jkarlsson » 20 Nov 2014, 22:12
I just wanted to say thank you. This is something that I have been missing since I first started using this software. I fact I was close to dusting of my rusty java skills and try implementing it myself 
I'm sticking with 1.5.29 until this gets merged. Thanks a million!
Of course there is some room for improvement. Ideally instead of freezing the game should continue with placeholder images while the images are downloaded in the background.
I'm sticking with 1.5.29 until this gets merged. Thanks a million!
Of course there is some room for improvement. Ideally instead of freezing the game should continue with placeholder images while the images are downloaded in the background.
- jkarlsson
- Posts: 3
- Joined: 07 Nov 2014, 23:39
- Has thanked: 0 time
- Been thanked: 0 time
Re: [Patch] Forge inline card image acquirer
by OpenSS » 21 Nov 2014, 01:50
Glad to hear someone likes it. Makes it worth it .
I do plan on updating it and improving it as time permits. Just wanted to see if anyone liked it first before I dedicated more of my scarce free time.
If anyone else is enjoying this, leaving a message is the best encouragement for updates.
Thanks,
OpenSS
.I do plan on updating it and improving it as time permits. Just wanted to see if anyone liked it first before I dedicated more of my scarce free time.
If anyone else is enjoying this, leaving a message is the best encouragement for updates.
Thanks,
OpenSS
- OpenSS
- Posts: 5
- Joined: 03 Nov 2014, 01:35
- Has thanked: 0 time
- Been thanked: 0 time
Re: [Patch] Forge inline card image acquirer
by felixsapiens » 22 Nov 2014, 09:37
Love this. First time I feel like I've got a full set of card images. The in game content downloader has always been sporadic.
With an implementation as suggested previously (placeholder images instead of game pausing) this should really be in the official release somehow; although I'm sure there's issues with servers and scraping the images.
With an implementation as suggested previously (placeholder images instead of game pausing) this should really be in the official release somehow; although I'm sure there's issues with servers and scraping the images.
- felixsapiens
- Posts: 12
- Joined: 14 Feb 2013, 06:36
- Has thanked: 5 times
- Been thanked: 0 time
Re: [Patch] Forge inline card image acquirer
by OpenSS » 22 Nov 2014, 23:18
I agree it would be great to have placeholder images until the image is loaded, but I need to figure out how to make it work with what Forge is doing. Hopefully I can get it to work.
As for sourcing the images, the mtgimage site says:
As for sourcing the images, the mtgimage site says:
Feel free to directly link (hotlink) to the images with your website or app.
The images are hosted on a dedicated server with a gigabit connection and tons of bandwidth.
- OpenSS
- Posts: 5
- Joined: 03 Nov 2014, 01:35
- Has thanked: 0 time
- Been thanked: 0 time
Re: [Patch] Forge inline card image acquirer
by jkarlsson » 24 Nov 2014, 13:25
Okay, so I've looked a bit into how to avoid freezing the UI while images are downloaded. I'm attaching a patch that includes both the original download code by OpenSS and my own experiments on top of that.
Now, this is the first time in ten years that I touch Java code and obviously the first time I see the forge code base so beware!
There is an explicit assert in ImageCache.getImage(final String) to prevent that method from ever running on another thread than the main ui thread. Since that was the whole point of this exercise I had to remove that. It is slightly worrying that someone took the time to actively prevent what is now being done. I did not see any strange concurrency errors but testing has been pretty limited.
I've only added this async behaviour to CardPanel so you should see it working in an actual match but not when viewing decks etc.
Now, this is the first time in ten years that I touch Java code and obviously the first time I see the forge code base so beware!
There is an explicit assert in ImageCache.getImage(final String) to prevent that method from ever running on another thread than the main ui thread. Since that was the whole point of this exercise I had to remove that. It is slightly worrying that someone took the time to actively prevent what is now being done. I did not see any strange concurrency errors but testing has been pretty limited.
I've only added this async behaviour to CardPanel so you should see it working in an actual match but not when viewing decks etc.
- Attachments
-
- Async-Card-Image-Loading.patch.txt
- (8.38 KiB) Downloaded 277 times
- jkarlsson
- Posts: 3
- Joined: 07 Nov 2014, 23:39
- Has thanked: 0 time
- Been thanked: 0 time
Re: [Patch] Forge inline card image acquirer
by Myrd » 01 Dec 2014, 17:12
After applying your patch, I get the following errors:jkarlsson wrote:Okay, so I've looked a bit into how to avoid freezing the UI while images are downloaded. I'm attaching a patch that includes both the original download code by OpenSS and my own experiments on top of that.
Now, this is the first time in ten years that I touch Java code and obviously the first time I see the forge code base so beware!
There is an explicit assert in ImageCache.getImage(final String) to prevent that method from ever running on another thread than the main ui thread. Since that was the whole point of this exercise I had to remove that. It is slightly worrying that someone took the time to actively prevent what is now being done. I did not see any strange concurrency errors but testing has been pretty limited.
I've only added this async behaviour to CardPanel so you should see it working in an actual match but not when viewing decks etc.
- Code: Select all
/forge-gui-desktop/src/main/java/forge/view/arcane/CardPanel.java:628
The method imageReady(BufferedImage) of type CardPanel must override or implement a supertype method
/forge-gui-desktop/src/main/java/forge/view/arcane/CardPanel.java:158
The method getImageAsync(CardView, int, int, IImageWaiter) in the type ImageCache is not applicable for the arguments (CardView, int, int, CardPanel)
BTW, I'm very excited about this - I think this should be made the default behavior (once the patch looks good to the team). Though I'm speaking on my own behalf - don't know what other devs think on the subject.
Re: [Patch] Forge inline card image acquirer
by Myrd » 01 Dec 2014, 17:18
Nevermind about the above errors - looks like eclipse didn't apply the patch very well and didn't actually change the line that made CardPanel implement IImageWaiter. After fixing that manually, it builds fine.
Re: [Patch] Forge inline card image acquirer
by Myrd » 01 Dec 2014, 17:40
So I took a look at the async patch and I think generally the approach looks good, but there's a couple problems from the code - somewhat related to the assert you commented out.
Since some of these functions are now being called from another thread, some of the code needs to now be made thread safe. Easiest way to do this is to put the synchronized keyword on the method declarations that are now being potentially called from different threads.
For example, scaleImage() accesses prefs (via FModel.getPreferences().getPrefBoolean()). Since that's backed by a map, which isn't thread safe, it's not OK to do this. Either we make the PreferencesStore class thread safe (i.e. by making methods synchronized) - or we could overload scaleImage() to take a param for that pref value - and query the pref at the time the ImageDownloadTask is constructed on UI thread.
Similarly, we need to audit other calls made by scaleImage() - for example getOriginalImage(). Note: It's not enough to just mark scaleImage() synchronized, since it may be calling lower-level APIs that other threads are also calling that are not synchronized.
Of note, it seems access to _CACHE is OK from multiple threads since the type of that object documents that it should be thread safe.
From the code, the above is the biggest issue I saw. There are a couple minor things I saw where things could be improved - mostly from a style perspective - e.g. making a helper method for the new code in ImageKeys.getImageFile().
That's about the code. In terms of functionality, I find the images downloaded with this don't look as good as the other ones. See attached screenshot. Herald of Anafenza was downloaded by this patch, whereas I believe the other images shown are from Forge's batch downloader. The new image has a much thicker black border - possibly because Forge adds its own, while the image this downloads probably already includes one. Maybe we should make the code crop the image it gets (assuming it always has a black border)?
Since some of these functions are now being called from another thread, some of the code needs to now be made thread safe. Easiest way to do this is to put the synchronized keyword on the method declarations that are now being potentially called from different threads.
For example, scaleImage() accesses prefs (via FModel.getPreferences().getPrefBoolean()). Since that's backed by a map, which isn't thread safe, it's not OK to do this. Either we make the PreferencesStore class thread safe (i.e. by making methods synchronized) - or we could overload scaleImage() to take a param for that pref value - and query the pref at the time the ImageDownloadTask is constructed on UI thread.
Similarly, we need to audit other calls made by scaleImage() - for example getOriginalImage(). Note: It's not enough to just mark scaleImage() synchronized, since it may be calling lower-level APIs that other threads are also calling that are not synchronized.
Of note, it seems access to _CACHE is OK from multiple threads since the type of that object documents that it should be thread safe.
From the code, the above is the biggest issue I saw. There are a couple minor things I saw where things could be improved - mostly from a style perspective - e.g. making a helper method for the new code in ImageKeys.getImageFile().
That's about the code. In terms of functionality, I find the images downloaded with this don't look as good as the other ones. See attached screenshot. Herald of Anafenza was downloaded by this patch, whereas I believe the other images shown are from Forge's batch downloader. The new image has a much thicker black border - possibly because Forge adds its own, while the image this downloads probably already includes one. Maybe we should make the code crop the image it gets (assuming it always has a black border)?
- Attachments
-
Last edited by Myrd on 01 Dec 2014, 18:05, edited 1 time in total.
Re: [Patch] Forge inline card image acquirer
by Myrd » 01 Dec 2014, 17:44
I also got the following exception when it tried to download images for a soldier token:
From the log, looks like the following exception also happened at some earlier point in time:
- UncheckedExecutionException | Open
- Code: Select all
Created a download task for t:w_1_1_soldier_m15
From the log, looks like the following exception also happened at some earlier point in time:
- NullPointerException | Open
Re: [Patch] Forge inline card image acquirer
by Myrd » 01 Dec 2014, 17:53
One other issue I noticed. Looks like it creates a ImageDownloadTask very often. Since this task actually won't be downloading the image each time, maybe it should have a different class name.
But also, I think it would be more efficient if the code first checked if the image was already in the cache - and if so, execute the old code path (on the UI thread) - and only spawn the task if the image isn't there.
But also, I think it would be more efficient if the code first checked if the image was already in the cache - and if so, execute the old code path (on the UI thread) - and only spawn the task if the image isn't there.
21 posts
• Page 1 of 2 • 1, 2
Who is online
Users browsing this forum: No registered users and 59 guests