Is there a way to pass a thread as an argument safely?

Question

I have the start of an android game. It has a GameLoop (thread) class which keeps track of an fps value and a GameView (surfaceview) which instatiates a GameLoop object and sets it running and so on. Now i have a separate FpsText object that knows how to draw itself to the screen but to update i need to do something like fpsText.setText(gameloop.getFps());

This isnt a problem if i do it in the GameView class but i wanted to try and have the an FpsText object that could take care of updating itself. I don't have any other experience with threads and I thought it might backfire but none the less i tried passing fpsText the gameloop reference as an argument. Unsurprisingly i get different unwanted results everytime i run the application so i wanted to know what you think the best way to handle this situation would be?

As an alternative to updating fpsText in the GameView i could always just make the fps value in the gameloop class public but i know that's bad practice! Neither sound like good solutions, perhaps there's a better way?

For reference here's my gameloop, with the second change suggested by RedOrav implemented:

package biz.hireholly.engine;

import android.graphics.Canvas;
import android.util.Log;
import android.view.SurfaceHolder;
import biz.hireholly.gameplay.FpsText;

import java.util.ArrayList;


/**
 * The GameLoop is a thread that will ensure updating and drawing is done at set intervals.
 * The thread will sleep when it has updated/rendered quicker than needed to reach the desired fps.
 * The loop is designed to skip drawing if the update/draw cycle is taking to long, up to a MAX_FRAME_SKIPS.
 * The Canvas object is created and managed to some extent in the game loop,
 * this is so that we can prevent multiple objects trying to draw to it simultaneously.
 * Note that the gameloop has a reference to the gameview and vice versa.
 */

public class GameLoop extends Thread {

    private static final String TAG = GameLoop.class.getSimpleName();
    //desired frames per second
    private final static int MAX_FPS = 30;
    //maximum number of drawn frames to be skipped if drawing took too long last cycle
    private final static int MAX_FRAME_SKIPS = 5;
    //ideal time taken to update & draw
    private final static int CYCLE_PERIOD = 1000 / MAX_FPS;

    private SurfaceHolder surfaceHolder;
    //the gameview actually handles inputs and draws to the surface
    private GameView gameview;

    private boolean running;
    private long beginTime = 0; // time when cycle began
    private long timeDifference = 0; // time it took for the cycle to execute
    private int sleepTime = 0; // milliseconds to sleep (<0 if drawing behind schedule) 
    private int framesSkipped = 0; // number of render frames skipped

    private double lastFps = 0; //The last FPS tracked, the number displayed onscreen
    private ArrayList<Double> fpsStore = new ArrayList<Double>(); //For the previous fps values
    private long lastTimeFpsCalculated = System.currentTimeMillis(); //used in trackFps
    FpsText fpsText;

    public GameLoop(SurfaceHolder surfaceHolder, GameView gameview, FpsText fpsText) {
        super();
        this.surfaceHolder = surfaceHolder;
        this.gameview = gameview;
        this.fpsText = fpsText;
    }

    public void setRunning(boolean running) {
        this.running = running;     
    }
    @Override
    public void run(){

        Canvas c;

        while (running) {
            c = null;
            //try locking canvas, so only we can edit pixels on surface
            try{
                c = this.surfaceHolder.lockCanvas();
                //sync so nothing else can modify while were using it
                synchronized (surfaceHolder){ 

                    beginTime = System.currentTimeMillis();
                    framesSkipped = 0; //reset frame skips

                    this.gameview.update();
                    this.gameview.draw(c);

                    //calculate how long cycle took
                    timeDifference = System.currentTimeMillis() - beginTime;
                    //good time to trackFps?
                    trackFps();

                    //calculate potential sleep time
                    sleepTime = (int)(CYCLE_PERIOD - timeDifference);

                    //sleep for remaining cycle
                    if (sleepTime >0){
                        try{
                            Thread.sleep(sleepTime); //saves battery! :)
                        } catch (InterruptedException e){}
                    }
                    //if sleepTime negative then we're running behind
                    while (sleepTime < 0 && framesSkipped < MAX_FRAME_SKIPS){
                        //update without rendering to catch up
                        this.gameview.update();
                        //skip as many frame renders as needed to get back into
                        //positive sleepTime and continue as normal
                        sleepTime += CYCLE_PERIOD;
                        framesSkipped++;
                    }

                }

            } finally{
                //finally executes regardless of exception, 
                //so surface is not left in an inconsistent state
                if (c != null){
                    surfaceHolder.unlockCanvasAndPost(c);
                }
            }
        }

    }

    /* Calculates the average fps every second */
    private void trackFps(){
        synchronized(fpsStore){
            long currentTime = System.currentTimeMillis();

            if(timeDifference != 0){
                fpsStore.add((double)(1000 / timeDifference));
            }
            //If a second has past since last time average was calculated,
            // it's time to calculate a new average fps to display
            if ((currentTime - 1000) > lastTimeFpsCalculated){


                for (Double fps : fpsStore){
                lastFps += fps;
                }

                lastFps /= fpsStore.size();

                synchronized(fpsText){ //update the Drawable FpsText object
                fpsText.setText(String.valueOf((int)lastFps));
                }

                lastTimeFpsCalculated = System.currentTimeMillis();

                //Log.d("trackFPS()", " fpsStore.size() = "+fpsStore.size()+"\t"+fpsStore.toString());
                fpsStore.clear();


            }
        }
    }

}

Answer 1

What are your unwanted results? Making your fps variable public and setting a getfps() are exactly the same thing as far as getting your fps value is concerned, and modifying the fps value in your GameView would never make any sense.

I suspect what you are doing in GameLoop is modifying your fps variable between seconds, i.e. getting intermediate values that your GameView is probably also reading. I suggest modifying the fps only every second and keep temporal variables in between for doing your calculations. Say you have 0 fps at the beginning. GameLoop does its calculations and once a second has passed, you've already collected how many frames it's processed and the time (1 second). Until then, GameView has always read 0 regardless of what GameLoop is doing. After that second, say you accomplished 60 fps, the fps variable is modified, and untouched until the next second. Therefore, for the following second, even if GameView does a lot more reads of your variable, it will always get 60.

// Inside GameLoop

private int fps; // Variable you're going to read
private int frameCount; // Keeps track or frames processed
private long lastUpdated;

while(running) {

   if(System.currentTimeMillis() - lastUpdated > 1000) { // 1 second elapsed
      fps = frameCount;
      frameCount = 0;
   }
   frameCount++;
}

public int getFps() {
   return fps;
}

Another way of doing it is passing a reference to fpsText to the GameLoop, which will modify it only after each second has passed. That way, your rendering thread only has to worry about rendering, and your GameLoop about the frames per second. Keep in mind, however, that you could potentially encounter a race condition (where you setText() in GameLoop right in the middle of your render method) that you wouldn't have to worry about the other way. Enclosing the fpsText object in synchronized brackets when you render and when you modify it should do the trick for you.

Let us know if that helped!