How is my Game Loop?

Question

I wrote a game loop for a game I'm going to be making:

1. Check the current FPS
2. Try the keep the FPS to a hardcoded level
3. Draw and Update the game.

There are two classes, the abstract class and the implementing class.

Any suggestions?

Game

public abstract class Game {
 private int FRAMES_PER_SECOND;
 private boolean running = true;
 long targetTime;
 private long runningFPS;
 protected Game(int fps) {
 setTargetFPS(fps);
 }
 public void setTargetFPS(int fps) {
 this.FRAMES_PER_SECOND = fps;
 targetTime = 1000 / FRAMES_PER_SECOND;
 }
 public void run(JPanel panel, BufferedImage image) {
 int currentFPS = 0;
 long counterstart = System.nanoTime();
 long counterelapsed = 0;
 long start;
 long elapsed;
 long wait;
 targetTime = 1000 / FRAMES_PER_SECOND;
 while (running) {
 start = System.nanoTime();
 processInput();
 update();
 // time to update and process input
 elapsed = System.nanoTime() - start; 
 wait = targetTime - elapsed / 1000000;
 if (hasTimeToDraw(wait)) {
 //CREATE AND ANTIALIAS GRAPHICS
 Graphics2D g = image.createGraphics();
 g.addRenderingHints(new RenderingHints(RenderingHints.KEY_ANTIALIASING, RenderingHints.VALUE_ANTIALIAS_ON));
 g.setRenderingHint(RenderingHints.KEY_TEXT_ANTIALIASING, RenderingHints.VALUE_TEXT_ANTIALIAS_ON);
 g.setRenderingHint(RenderingHints.KEY_RENDERING, RenderingHints.VALUE_RENDER_QUALITY);
 //Draw
 draw(g);
 g.dispose();
 panel.repaint();
 //Take account for the time it took to draw
 elapsed = System.nanoTime() - start;
 wait = targetTime - elapsed / 1000000;
 }
 counterelapsed = System.nanoTime() - counterstart;
 currentFPS++;
 // at the end of every second
 if (counterelapsed >= 1000000000L) {
 //runningFPS is how many frames we processed last second
 runningFPS = currentFPS;
 currentFPS = 0;
 counterstart = System.nanoTime();
 }
 //dont wanna wait for negative time
 if (wait < 0)
 wait = 0;
 try {
 Thread.sleep(wait);
 } catch (InterruptedException e) {
 e.printStackTrace();
 }
 }
 }
 public long getCurrentFPS() {
 return runningFPS;
 }
 private boolean hasTimeToDraw(long wait) {
 //Not really sure how to implement this method... Maybe just time the draw method and hardcode it in?
 return true;
 }
 public void stop() {
 running = false;
 }
 public abstract void processInput();
 public abstract void update();
 public abstract void draw(Graphics2D g);
}

GameFrame

public class GameFrame extends JFrame{
 private static final long serialVersionUID = 1L;
 private static final int WIDTH = 800;
 private static final int HEIGHT = 800;
 private JPanel panel;
 private BufferedImage image = new BufferedImage(WIDTH, HEIGHT, BufferedImage.TYPE_3BYTE_BGR);
 private Game game = new Game(60){
 int i = 0;
 int x = 5;
 @Override
 public void processInput() {
 // TODO Auto-generated method stub
 }
 @Override
 public void update() {
 GameFrame.this.setTitle("FPS: " + getCurrentFPS());
 if(i > WIDTH || i < 0)
 x = -x;
 i += x;
 }
 @Override
 public void draw(Graphics2D g) {
 g.setColor(Color.BLACK);
 g.fillRect(0, 0, WIDTH, HEIGHT);
 g.setColor(Color.RED);
 g.fillRect(i, 50, 20, 53);
 }
 };
 public GameFrame(){
 panel = new JPanel(){
 private static final long serialVersionUID = 1L;
 @Override
 protected void paintComponent(Graphics g) {
 g.drawImage(image, 0, 0, null);
 }
 };
 panel.setPreferredSize(new Dimension(WIDTH,HEIGHT));
 this.add(panel);
 this.pack();
 this.setVisible(true);
 this.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
 }
 public void run(){
 game.run(panel,image);
 }
 public static void main(String[] args){
 new GameFrame().run();
 }
}

Answer 1

long start;
long elapsed;
long wait;

This is no Pascal, declare your variable where they get initialized. This is not always possible, maybe 1 such variable per 10 classes is unavoidable.

 //dont wanna wait for negative time
 if (wait < 0)
 wait = 0;
 try {
 Thread.sleep(wait);
 } catch (InterruptedException e) {
 e.printStackTrace();
 }

Better don't change a variable if you don't have to as it makes the code harder to understand. Doing

Thread.sleep(wait<0 ? 0 : wait);

is simpler, doing

Thread.sleep(Math.max(0, wait));

is even simpler and so is

if (wait>0) Thread.sleep(wait);

The main problem is that your method is far too long. Extracting some methods would help a lot, e.g.,

• Graphics2D g = newNiceGraphics(image);
• sleepUninterruptibly(wait);

Enough for now....

Answer 2

Reserve all capital names to static final constants. It seems that a better name instead of FRAMES_PER_SECOND would be targetFPS.

Also, this variable can be final, because you assign it once, though this is not clear immediately. It becomes clear when you reorganize the constructor and get rid of the pointless setTargetFPS method, like this:

private final int targetFPS;
protected Game(int fps) {
 this.targetFPS = fps;
}

targetTime doesn't need to be a member variable: although you set it in the constructor, you don't use that initial value, and then you overwrite it in the run method. So change that from a member variable to a local variable in the run method.

In the run method, if you don't want to wait for negative time, then don't! Instead of overwriting the value of wait, use conditionals. It's also pointless to wait for 0 time:

if (wait > 0) {
 try {
 Thread.sleep(wait);
 } catch (InterruptedException e) {
 e.printStackTrace();
 }
}

---

In GameFrame, the image and panel fields can be final. In general, make member fields final if possible. Non-editable things are inherently robust and lead to safer code.

Answer 3

Thread.sleep() is a bad call which can lead into many problems. I was told to never use it. instead I will show you the way I do my game loops. It might not be the perfect game loop but it is good.

I recommend implements runnable and putting your loop in your run method.

public void run(){
 init(); //initialisation of images, sound..etc. will be executed once only
 int fps = 60 //number of update per second.
 double tickPerSecond = 1000000000/fps;
 double delta = 0;
 long now;
 long lastTime = System.nanoTime();
 while(running){
 now = System.nanoTime();
 delta += (now - lastTime)/tickPerSecond;
 lastTime = now;
 if(delta >= 1){
 tick();
 render();
 delta--;
 }
 }
}
private void init(){
 //initialisation image, sound, loading world, generate maps....etc
}
private void tick(){
 //tick player, world, entities..etc
}
private void render(){
 //render graphics. 
}

Also don't forget to create start and stop methods for the thread. You can change the fps to what number you would like, no need to go higher than 60.

Explanation why thread.sleep() is a bad call: click-here

Its not that important but its better also if you gonna make 3d-game and using physics which requires accuracy in timing.

• mdfst13: This would be a better answer with more explanation of why Thread.sleep() is bad. A link wouldn't hurt, but you should include at least one example of a problem caused by Thread.sleep() in the post itself.