Java animation does not refresh properly and uses too much CPU

Question

I have a problem with a simple Java game I am creating right now. I want a dot (a car) to be movable across the game screen, but instead of this all I can see on the screen is the long "snake" created by the dot moved by me:

Other problem is that activity manager on my Mac shows that the game uses huge amount of CPU power - my laptop gets very hot very fast. I suspect that there is something wrong with my game loop, but since now I haven't found any solution:

BoardPanel.java:

import java.awt.Dimension;
import java.awt.Graphics;
import java.awt.Graphics2D;
import java.awt.image.BufferedImage;
import java.awt.event.*;

import javax.swing.*;

@SuppressWarnings("serial")
public class BoardPanel extends JPanel implements KeyListener, Runnable {
    public static final int WIDTH = 600;
    public static final int HEIGHT = 600;


    private Thread thread;
    private boolean running;

    private BufferedImage image;
    private Graphics2D g;

    private int FPS = 30;
    private int targetTime = 1000/FPS;

    private Map map;
    private Car car;

    public BoardPanel() {
        super();
        setPreferredSize(new Dimension(WIDTH, HEIGHT));
        setFocusable(true);
        requestFocus();
    }

    public void addNotify() {
        super.addNotify();
        if(thread == null) {
            thread = new Thread(this);
            thread.start();
        }
        addKeyListener(this);
    }

    public void run() {
        init();

        long startTime;
        long reTime;
        long waitTime;

        while (running) {
            startTime = System.nanoTime();
            update();
            render();
            draw();

            reTime = System.nanoTime() - startTime;
            waitTime = targetTime - reTime;
            try {
                Thread.sleep(waitTime);
            }
            catch(Exception e) {

            }
        }
    }

    private void init() {
        running = true;
        image = new BufferedImage(WIDTH, HEIGHT, BufferedImage.TYPE_INT_RGB);
        g = (Graphics2D) image.getGraphics();

        map = new Map();
        car = new Car(map);
        car.setxpos(50);
        car.setypos(50);

    }

    private void update() {
        map.update();
        car.update();

    }

    private void render() {
        map.draw(g);
        car.draw(g);
    }

    private void draw() {
        Graphics g2 = getGraphics();
        g2.drawImage(image, 0, 0, null);
        g2.dispose();
    }

    public void keyTyped(KeyEvent key) {

    }

    public void keyPressed(KeyEvent key) {
        int code = key.getKeyCode();

        if(code == KeyEvent.VK_LEFT) {
            car.setLeft(true);
        }
        if(code == KeyEvent.VK_RIGHT) {
            car.setRight(true);
        }
        if(code == KeyEvent.VK_UP) {
            car.setUp(true);
        }
        if(code == KeyEvent.VK_DOWN) {
            car.setDown(true);
        }
    }

    public void keyReleased(KeyEvent key) {
        int code = key.getKeyCode();

        if(code == KeyEvent.VK_LEFT) {
            car.setLeft(false);
        }
        if(code == KeyEvent.VK_RIGHT) {
            car.setRight(false);
        }
        if(code == KeyEvent.VK_UP) {
            car.setUp(false);
        }
        if(code == KeyEvent.VK_DOWN) {
            car.setDown(false);
        }
    }
}

Car.java

import java.awt.*;

public class Car {
    private double xpos;
    private double ypos;

    //private int xsize;
    //private int ysize;

    private boolean left;
    private boolean right;
    private boolean up;
    private boolean down;

    private Map map;

    public Car(Map m) {
        map = m;
    }

    public void setxpos(int i) {
        xpos = i;
    }

    public void setypos(int i) {
        ypos = i;
    }

    public void setLeft (boolean b) {
        left = b;
    }

    public void setRight (boolean b) {
        right = b;
    }

    public void setUp (boolean b) {
        up = b;
    }

    public void setDown (boolean b) {
        down = b;
    }

    public void update() {
        if(left) {
            xpos--;
        }

        if(right) {
            xpos++;
        }
        if(up) {
            ypos--;
        }
        if(down) {
            ypos++;
        }
    }

    public void draw(Graphics2D g) {
        int mx = map.getx();
        int my = map.gety();

        g.setColor(Color.BLUE);
        g.fillOval((int)(mx+xpos-20/2), (int)(my+ypos-20/2), 20, 20);
    }
}

Map.java (I haven't created map yet, right now only want the dot to move properly)

import java.awt.*;

public class Map {
    public int x;
    public int y;

    public int getx() {
        return x;
    }

    public int gety() {
        return y;
    }

    public void setx(int i) {
        x = i;
    }

    public void sety(int i ) {
        y = i;
    }

    public void update() {

    }

    public void draw(Graphics2D g) {

    }


}

RacerMain.java

import javax.swing.JFrame;


public class RacerMain {
    public static void main (String[]args) {
        //MainFrame mf = new MainFrame();
        JFrame mf = new JFrame("Racer");
        mf.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        mf.setContentPane(new BoardPanel());;
        mf.pack();
        mf.setVisible(true);

    }

}

Many thanks for your help!!!

Answer 1

In addition to what camickr said: Be careful with your usage of the Graphics. A rule of thumb:

Never call getGraphics on a Component!

Additionally, you are never disposing the Graphics that you are fetching from the BufferedImage. You are instead disposing the Graphics that you obtained from the Component, which may be even worse than fetching it in the first place!

I wonder why you are overriding the addNotify method. You should not implement any functionality based on averriding this method....

So you should change the respecive parts of your BoardPanel class roughly as follows:

public class BoardPanel extends JPanel implements KeyListener, Runnable     {
    ...
    // private Graphics2D g; // Don't store this here

    public BoardPanel() {
        ...

        // Create the thread here instead of in the "addNotify" method!
        if(thread == null) {
            thread = new Thread(this);
            thread.start();
        }
        addKeyListener(this);
    }


    public void run() {
        ...
        while (running) {
            ...
            //draw(); // Don't call this method
            repaint(); // Trigger a repaint instead!
        }
    }


    private void render() {
        Graphics2D g = image.createGraphics();

        // Clear the background (see camickrs answer)
        g.setColor(Color.BLACK);
        g.fillRect(0,0,image.getWidth(),image.getHeight());

        try
        {
            map.draw(g);
            car.draw(g);
        }
        finally
        {
            g.dispose(); // Dispose the Graphics after it has been used
        }
    }

    /** Don't call "getGraphics" on a component!   
    private void draw() {
        Graphics g2 = getGraphics();
        g2.drawImage(image, 0, 0, null);
        g2.dispose();
    }
    */

    // Override the paintComponent method instead:
    @Override
    protected void paintComponent(Graphics g)
    {
        super.paintComponent(g);
        g.drawImage(image,0,0,null);
    }

Answer 2

In your draw() method you need to clear the BufferedImages background before your invoke the fillOval method. Something like:

g.setColor( Color.BLACK );
g.fillRect(...);
g.setColor( Color.BLUE );
g.fillOval(...);

Print out your "waitTime" to make sure you are waiting a reasonable time.