Is this the correct approach to animation in Java?

Question

I was fiddling with animation in Java and wrote this:

import java.awt.Graphics;
import java.awt.Image;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.util.ArrayList;
import javax.swing.Timer;
import javax.swing.ImageIcon;
import javax.swing.JFrame;
class Main extends JFrame implements ActionListener {
 private Timer timer = new Timer(500, this);
 private Image mario1 = new ImageIcon("C:/Users/User/workspace/Game Development/src/mario.jpg").getImage();
 private Image mario2 = new ImageIcon("C:/Users/User/workspace/Game Development/src/mario2.jpg").getImage();
 private int counter = 0;
 private ArrayList<Image> scenes = new ArrayList<Image>();
 public Main() {
 setSize(500, 500);
 setVisible(true);
 addScene(mario1);
 addScene(mario2);
 timer.start();
 }
 public void actionPerformed(ActionEvent e) {
 nextScene();
 }
 public void addScene(Image image) {
 scenes.add(image);
 }
 public void nextScene() {
 counter++;
 repaint();
 }
 @Override
 public void paint(Graphics g) {
 super.paint(g);
 if (!(counter < 0 || counter >= scenes.size())) g.drawImage(scenes.get(counter), 100, 100, null);
 }
 public static void main(String[] args) {
 new Main();
 }
}

Is this the correct approach or is this just bad practice?

• GlenPeterson: You might have a look at double-buffering or page-flipping: docs.oracle.com/javase/tutorial/extra/fullscreen/doublebuf.html

Answer 1

Two recommendations:

• It is better that you don't "do any action" in the constructor; create, say, a .start() method and do new Main().start() in main();

• rather than testing on count in animation, use this, where nrImages is the number of images:

  count = (count + 1) % nrImages;
  

Also, declare your list as a List, not an ArrayList: use interfaces when available.

• asteri: Why is it better to make a start() method? I've always seen JFrame initialization performed in the constructor.

Answer 2

You have the right general idea for animation, yes. Basically you have created animation with 2 FPS (frames-per-second).

I think the thing that can most improve your code is using a different data structure than a List. It's my final note at the end of this post because my sample code uses some of the other best-practices suggestions detailed throughout.

class Main 

From a purist perspective, your class should explicitly be public.

class Main extends JFrame

JFrame implements the Serializable interface, so it's a good practice to have a serialVersionUID like so: private static final long serialVersionUID = 1L;. (Not having this should produce a warning if you are using Eclipse as your IDE.)

private Image mario1 = new ImageIcon("C:/Users/User/workspace/Game Development/src/mario.jpg").getImage();
private Image mario2 = new ImageIcon("C:/Users/User/workspace/Game Development/src/mario2.jpg").getImage();

Since you are pre-loading your images, consider making them final. This prevents you from accidentally overwriting them at some other point in your code, and it also makes it very clear for those reading your code that mario1 reference will always point to the same Image. If you do this, these are essentially constants and should be renamed in all uppercase with words separated by underscores (such as MARIO_1 and MARIO_2).

I would also consider renaming these variables to something more meaningful in this case. What is unique about the images? For example, private final Image MARIO_STILL and private final Image MARIO_JUMPING are much more descriptive and improve your code's readability.

And of course, if these images can be reused throughout all of your game instances, you can make them true constants and make them static as well.

public Main() {
 setSize(500, 500);
 setVisible(true);
 addScene(mario1);
 addScene(mario2);
 timer.start();
}

Consider explicitly calling super() at the beginning of this constructor. Also, this doesn't make a huge difference with such a trivial animation, but I would perform your addScene() calls before the setVisible(true) call. Like I said, it doesn't make much difference in this specific scenario, but if addScene() performed a lot more processing which was heavy computationally, the user of your program would experience a lag time between when the frame became visible and when the animations actually began.

public void actionPerformed(ActionEvent e) {
 nextScene();
}

It's implicit, but for style points you should explicitly add the @Override annotation. It's also a good idea to always check for the source of the ActionEvent (as in if(e.getSource() == timer)), since when your program gets more complex you will likely be receiving events from multiple objects.

@Override
public void paint(Graphics g) {
 super.paint(g);
 if (!(counter < 0 || counter >= scenes.size())) g.drawImage(scenes.get(counter), 100, 100, null);
}

Consider calling timer.stop() when there are no more Images left to display. This is just an efficiency concern and not really something crucial.

Finally, a more advanced note. While the way you've implemented the frames here works fine, it's not the most convenient data structure for what you're trying to accomplish. I think a Queue would be more intuitive for you. A Queue is a FIFO data structure which will let you add and remove elements to it on a "first-in, first-out" basis. This prevents you from having to have a counter variable which you increment manually each time a scene is displayed. You can just keep adding Images to the Queue and remove the next one each time the Timer fires.

This greatly simplifies your code:

public void actionPerformed(ActionEvent e) {
 if(e.getSource() == timer) repaint();
}
@Override
public void paint(Graphics g) {
 super.paint(g);
 if (imageQueue.peek() != null) {
 g.drawImage(imageQueue.remove(), 100, 100, null);
 if(imageQueue.peek() == null) timer.stop();
 }
}

Note that this also only works for displaying images. If you intend to ever do other processing while animating (like accepting user input from the keyboard), your animation will need to occur in a separate Thread.