[SOLVED] Loading an Image only once and using it in multiple objects

Hi.

I have an object that is able to draw itself ( A loaded BufferedImage ) with the help of a method. The object reads a *.png image from a specific destination into its own BufferedImage variable each time it is instantiated.

I use a lot of these objects at the same time.

I then have a JPanel iterate through all of these objects ( with the help of List ) and have them invoke their own paintMe() method within a JPanel graphics. This allows the objects to “handle” their own drawing in their respective places ( they are scattered around the screen ).

This is inefficient… becaue I’m loading the same image into memory n times for n amount of objects even though they all use the exact same *.png file.

Is there a better way to load images in java so that you won’t need to load the same image twice, but just use the one you already have in memory?

(NOTE: I’m not looking for a SPECIFIC fix ie. Not to make the BufferedImage static for the object. Because the object should be able to load any *.png file ( ie the same instance of the class can have any kind of image representing itself ) and still not be forced to load a specific image twice.

thanks,
Jon

Put the images into a static HashMap, then when the object is instantiated check if the image is loaded already if it is move on, if it isn’t load it and put it in the HashMap.

I looked into the HashMap class and found a way to load images with it so that no image is loaded twice. Thanks:)

Here’s my implementation for those interested.


/*
 * SpriteBank class
 * Desc: Handles the loading of images/sprites.
 * Other objects use this class to access/load their images
 * so that no image is loaded twice and that all images are handled
 * neatly and seperately.
 *
 * NOTE: Is a Singleton object! ( Can be instantiated once, and only once ).
 */
 
 import java.util.*; // for List and HashMap
 import java.awt.Image;
 import java.awt.image.BufferedImage;
 import java.awt.*;
 import java.awt.image.*;
 import javax.imageio.*;
 import java.io.*;
 
 public class SpriteBank {
	
	// HashMap - used to store the imagesuniquely as to prevent
	// loading the same image twice.
	//private HashMap<String,BufferedImage> //HashMap<K,V>
	//					hm = new HashMap<String,BufferedImage>();
	private HashMap<String,BufferedImage> //HashMap<K,V>
						hm = new HashMap<String,BufferedImage>();
	////
	private String path = "gfx/";
	
		//Singleton code -+-+-+-+-+-+-+-+-+-+-++-+-+-+-+-+-+-+-+-+-+-+-
	// Used to holds a reference to ourself (Singleton design)
	private static SpriteBank ref = null;
	
	// private Constructor ( Singleton method )
	private SpriteBank() {
		/* No code required here
		 * - we just want to make sure that no one can instantiate this class
		 *   without our (itselfs) permission
		 */
	}
	
	public static synchronized
			SpriteBank start() {
		// Start/Instantiate the SpriteBank class
		if (ref == null) {
			// Make sure this class is only instantiated once
			// and that it afterwards only references itself
			ref = new SpriteBank();
			}
		// return ref which now points to the instance of SpriteBank we just made
		return ref;
		}
	
	@Override
	public Object clone() 
			throws CloneNotSupportedException {
		// Override the generic clone method ( inherited from Object )
		// to prevent cloning ( so there can't be more SpriteBanks than 1
		// , not even clones )
		throw new CloneNotSupportedException();
		// If you try to clone() you'll get tazered with an exception! :P
	}
		//Singleton code end -+-+-+-+-+-+-+-+-+-+-++-+-+-+-+-+-+-+-+-+-+-+-
	// normal code
	
	private synchronized BufferedImage loadSprite(String sprName) {
		// Attempts to load sprite sprName
		BufferedImage spr = null;
		try {
			spr = ImageIO.read(new File(path+sprName));
		} catch (IOException e) {
			// Error occured
			return null;
		}
		return spr;
	}
	private boolean sprExists(String sprName) {
		// checks if a sprite has already been added/loaded
		// into the hash map
		boolean b = hm.containsKey(sprName);
		return b;
	}
	
	
	
	public synchronized BufferedImage getSprite(String sprName) {
		// used by other objects to load/get their image
		// that represents their sprite name.
		if(sprExists(sprName)) {
			// Sprite already loaded.
			return hm.get(sprName);
		} else {
			// Sprite hasn't been loaded yet.
			BufferedImage spr;
			spr = loadSprite(sprName);
			if(spr == null) {
				// An error occured, couldn't load sprite.
				return null;
			} // else move on
			  /* transform the loaded sprite into an image
			   * with the top left pixel color as transparent */
			spr = 
				imageToBufferedImage(  makeColorTransparent(spr)  );
			
			// save the loaded sprite (and now transparent) into the hashmap
			hm.put(sprName, spr);
			// and then return the sprite
			return spr;
		}
	}
	
	// Transforms a BufferedImage with a transparent color
	// into an Image ( use imagetoBufferedImage(Image imgage) method
	// to turn it back into a BufferedImage.
	private Image 
				makeColorTransparent(BufferedImage img) {
					final Color c =  new Color(img.getRGB(0,0)); /* The color to turn transparent
											  * is in the top left corner
											  * of the image */
					ImageFilter filter = new RGBImageFilter() {
						// the color to turn into transparent (opaque)
						int markerRGB = c.getRGB() | 0xFF000000;
						
						public final int filterRGB(int x, int y, int rgb) {
							if( (rgb | 0xFF000000) == markerRGB) {
								// Mark the alpha bits as zero - transparent
								return 0x00FFFFFF & rgb;
							} else {
								//do nothing
								return rgb;
							}
						}
					};
					
					ImageProducer ip = new FilteredImageSource(img.getSource(), filter);
					return Toolkit.getDefaultToolkit().createImage(ip);
	}
	
	// Turn an Image into a BufferedImage
	private BufferedImage imageToBufferedImage(Image image) {
    	BufferedImage bufferedImage = new BufferedImage(image.getWidth(null), image.getHeight(null), BufferedImage.TYPE_INT_ARGB);
    	Graphics2D g2 = bufferedImage.createGraphics();
    	g2.drawImage(image, 0, 0, null);
    	g2.dispose();
    	return bufferedImage;
		/* Source code
		http://stackoverflow.com/questions/665406/how-to-make-a-color-transparent-in-a-bufferedimage-and-save-as-png
		*/
    }
	
	public synchronized int size() {
		// return the number of images loaded into memory
		return hm.size();
	}
	
 }

Ugh. There is absolutely no reason to make that into a singleton other than lazyness. >:(

I was going to PM you but I can’t find a PM button anywhere - either I’m blind or could someone tell me?

I was going to ask why not to use a SingleTone? The only other way would be to make the whole object Static? And why would it be lazy to make a Singletone? Making the object static sound more lazy to me… Is there another better way to accomplish this? I’d be interested to hear it.

Thanks,
Jon

Click a user and then there is a link “Send this member a personal message.”

I never use singletons, only static classes…

How about neither of those? Just use it like a regular class, make a single object and pass that into the objects that need to use it.

Using it as a singleton hides important dependances, makes resource management harder and more error prone and gives you no advantages other than a little less typing.

So I can basically achieve the same thing by just making the HashMap variable static, as well as the getSprite() metod static ( the method other objects use to get/load their sprites through my class)? What you said about hidden dependencies is pretty enlightening. :point:

The reason I fell into using a singleton is because I was looking for a way to have 1 single class handle the loading and distribution of images throughout my objects. And because in order to use a HashMap I needed to instantiate the class which made me worried about accidentally instantiating the class twice (or more) and mess up the whole idea of having 1 single class handle the images.

Looking at it now I find it quite redundant and potentially harmful. :-X

I have a static SpriteList class that stores my sprites for me, so all I have to do when I need a Sprite is call SpriteList.get(“SpriteName”); works pretty well for me and it just eliminates having to pass around a Sprite, or a SpriteList object. Err… scratch that… I refactored my code so that it uses a non-static non-singleton SpriteList and I like it more x). I just pass either a specific Sprite/set of Sprites around or a reference to the SpriteList. ;D

No no. Banish all thoughts of anything being static. Nothing here needs to be static. Your SpriteBank class stays as it is, but without the singleton bits. Then, somewhere in your Game or App class (whatever is near the top) you create one SpriteBank, and pass it in as an argument (probably in the c’tor) to your Map or Level or Player or whatever needs to actually call getSprite().

And you’ll probably find too many places calling getSprite(), which is almost certainly an indication that you’ve got too much code that talks about graphics-y stuff that really shouldn’t. So there’s more possibilities for refactoring there too.

Very, very few bits of mutable state actually need to be static in a game. I actually can’t remember a time when I added mutable global/static state and didn’t later refactor it to be non-static and found the result to be simpler and more flexible.

I also can’t remember the last time I actually made or needed a singleton. But once you get stuck in a mindset of making things singletons, suddenly everything becomes a singleton and you’re pretty screwed.

But if I load a sprite seperately for every object I will end up with hundreds of objects that have loaded the exact same image into memory multiple times. I don’t see how without a static variable this can be achieved… Well, unless like you said you create one SpriteBank class and pass its reference around… but that would mean I’d have to do something like Game.getSpriteClass().getSprite() or Game.sprClass.getSprite(); - Rather just have them talk directly to the SpriteBank class. I see the minimum amount of ‘static’ use would be to make the HashMap static since at least then, even if there are multple SpriteBank instances around, they will all check with the Static HashMap if a certain image has already been loaded into memory.

Wouldn’t you agree?

Not really. Put simply you’re Doing It Wrong.

You keep trying to inject or reach out to global state where none is required. Think about something along these lines:


class Game
{
  public static void main(String[] args)
  {
    Game game = new Game();
    game.run();
  }

  private void run()
  {
     SpriteBank spriteBank = new SpriteBank();
     Map map = new Map(spriteBank);
     Player player = new Player(spriteBank);

     while (gameRunning)
     {
        map.update();
        player.update();

        map.draw();
      }
  }
}

You create your sprite bank, pass it into the objects that need it, and they call getSprite() as needed. Obviously lots missing from the example code but no global or static data is needed (or desired).

Perhaps if you posted some of your code we’d be able to give you some more specific hints.

I made a small mashup with my excellent MS Paint skills http://i.imgur.com/14SyM.png

All of my classes are in separate files.

What happens is that whenever I instantiate a Block class ( which is a subclass of the abstract ISOObject ) it gives SpriteBank its String sprName variable. The SpriteBank class determines if this sprName points to a valid image file and (loads the image the first time) and references it back to the Block object.

The drawing of the ISOObjects start from the JPanel class (which creates the “Map” (ISORoom room = new ISORoom():wink: that holds a sorted ArrayList of ALL ISOObjects that exist in the room). The JPanel calls the room.paintAll() method. The room.paintAll() method iterates through its ArrayList variables, invoking each ISOObjects paint() method. The ISOObjects.paint() method uses the drawImage() method to draw its image on the JPanel’s Graphics “screen”.

Are there any significant reasons why NOT to make the class/HashMap static?

The evils of singletons and global mutable state are well documented by people more expressive and smarter than me, so you should probably just google that. You could certainly start with singletons are evil. But largely they’re hard to test, cause dependancy nightmares and generally make code more brittle.

For your code, why not just create a single SpriteBank in ISOFrame and hold it as a member variable there? Then you’d pass it down to ISORoom and ISOObject via their constructors are required. Then if (for example) different rooms needed different sprite banks (say, one room was darker and all sprites needed to be made darker when being loaded) you could do that without any further changes. With a singleton that’d be a much messier thing to implement.

Singletons are not evil per se – this SpriteBank class is a singleton, for example. What’s evil is static singletons, where their “singletonicity”, to coin a fancy new term, is maintained by static members instead of some container object. MyClass.getInstance() is doing singletons wrong; Spring and Guice do singleton right (and for games, I’d recommend Guice – Spring is kinda “enterprisey”).

Jeez. I think it does make sense to keep it static, or at least separate from the game state. The underlying file system could be considered “static” in a way, so I think a resource cache should also be static in my opinion. It would be a little bit simpler to not have to pass around an instance of SpriteBank all the time, and it works, so why overcomplicate things?

A file system isn’t static, nor is it a singleton, nor is a SpriteBank a file system. That analogy doesn’t really work for me I’m afraid.

Secondly, passing around a reference is simpler than implementing a singleton. Passing around reference is what we do when we program. Singletons subvert a large amount of the structure that programs (and languages) attempt to build.

More practically, the constraints for a singleton (only a single one must ever exist, and it must be globally accessible) are very, very rarely met. Sometimes one or the other is, but both must be satisfied for a singleton to be reasonably justified. Should a sound mixer have global, unfettered access to a sprite bank? Should a keyboard input device have access to a sprite bank? I don’t think you can make a reasonable case for this.

If you need to pass around a ready-made instance of your singleton whenever you create a new instance pass that depends on it, and that instance maybe needs an instance of some other object and so forth, your job gets a lot easier when you have a class factory that keeps track of these dependencies, creates them on demand, and injects them into new objects. This is exactly what Dependency Injection (DI) containers like Spring and Guice do. You’re not stuck with globals, you don’t have to worry about making them threadsafe (the container does that), and you can special-case some classes to inject a different instance instead of your singleton when you need it (such as for testing).

What I meant was for a resource loader+cache, not necessarily a SpriteBank. Such a class could be used in many places (textures, sound, input key bindings), but I realize that this isn’t even a smart idea (having such a cache I mean), and that it hardly applies to this case in the first place, so you’re right.

I’d like to thank everyone for their input even though I derailed the issue slightly off-topic. I’ve learned more than expected.

As a new programmer the big picture is still blurred to say the least but I think I can at least sniff some of the aromas of less static and non-singleton design.

I have refactored my code to not use singleton or static design - I’m too much of a newbie to have a valid opinion on this but I have a feeling I’d later in life adhere to it. :stuck_out_tongue:

Thanks again,
jon