Hello, I've written a timer class in C++ but it's not working properly. The trouble I'm having is that after a couple of minutes the animation becomes choppy. It sometimes fixes automatically after about 5 ~ 60 seconds.

#include <windows.h>

class Timer {

	int sleep_length_ms;
	double ratio;
	LONGLONG duration, next_mark;
	LARGE_INTEGER counter, frequency;

public:
	Timer(int frame_rate) {
		QueryPerformanceCounter(&counter);
		QueryPerformanceFrequency(&frequency);

		duration = frequency.QuadPart / frame_rate;
		next_mark = counter.QuadPart + duration;
		ratio = 1000.0 / frequency.QuadPart;
	}

	bool wait() {
		QueryPerformanceCounter(&counter);
		sleep_length_ms = (int) (ratio * (next_mark - counter.QuadPart));

		if(sleep_length_ms > 0) {
			Sleep(sleep_length_ms);
			next_mark += duration;					
		} else {
			// if running behind
			QueryPerformanceCounter(&counter);
			next_mark = counter.QuadPart + duration;
		}

		return true;
	}
};

This is how the class if used in a game loop:

Timer timer(60);

while(run_game) {
	update_game();
	render_game();

	timer.wait();
}

I'm testing this class using a simple Allegro program where a circle bounces off the edges. I tried using the timer class provided by Allegro and the issue resolves so I'm quite sure that the problem lies within my timer class. I can't seem to figure out what I'm doing wrong and would be very grateful if someone can have a read through the code. Thank you!

Sleep is evil. AFAIK the resolution of Sleep is that of the task scheduler timer. 20ms? Searching for that topic should help you out.

I know that Sleep provides no guarantee that the thread will "wake up" after the specified duration but I also don't want to rely on a do while loop for timing. I read through the relavent source files of Allegro and it also uses Sleep in the timer thread and it seems to work perfectly.

Add a check code. sleep_length_ms cannot be longer than the period of the timer. for your example 16ms. So if it is larger than 0 but even larger than the timer period something goes wrong.

Here is mine. I use the "HasElapsed" function to do things periodically in a main loop. For example, you can send a packet every 20ms by calling HasElapsed.

#pragma once
class GameTimer
{
public:
	GameTimer(void);
	~GameTimer(void);

	void  Reset();
	float Tick();
	bool HasElapsed(float milliseconds);

private:
	bool    IsPerfCounterAvailable;
	float   TimeScale;
	__int64 PerfCounterFrequency;
	__int64 LastTime;
	__int64 CurrentTime;	
};

GameTimer::GameTimer(void)
{
	//Check if a High resolution timer is available 
	if(QueryPerformanceFrequency((LARGE_INTEGER*)&PerfCounterFrequency)){ 
		IsPerfCounterAvailable = true;
		QueryPerformanceCounter((LARGE_INTEGER*)&CurrentTime); 
		TimeScale = 1.0f / PerfCounterFrequency;
	} else { 
		IsPerfCounterAvailable = false;
		CurrentTime = timeGetTime(); 
		TimeScale	= 0.001f;
    } 
	
	Reset();
}


GameTimer::~GameTimer(void)
{
}

bool GameTimer::HasElapsed(float milliseconds)
{
	if(IsPerfCounterAvailable){
		QueryPerformanceCounter((LARGE_INTEGER*)&CurrentTime);
	} else {
		CurrentTime = timeGetTime();
	}

	if( ((CurrentTime - LastTime) * TimeScale) > (milliseconds*0.001f))
	{
		LastTime = CurrentTime;
		return true;
	}

	return false;
}

float GameTimer::Tick()
{
	LastTime = CurrentTime;

	if(IsPerfCounterAvailable){
		QueryPerformanceCounter((LARGE_INTEGER*)&CurrentTime);
	} else {
		CurrentTime = timeGetTime();
	}

	// Calculate the elapsed time
	float ElapsedTime = (CurrentTime - LastTime) * TimeScale;

	return ElapsedTime;
}

void GameTimer::Reset()
{
	if(IsPerfCounterAvailable){
		QueryPerformanceCounter((LARGE_INTEGER*)&CurrentTime);
	} else {
		CurrentTime = timeGetTime();
	}

	// Initialization
	LastTime = CurrentTime;
}

Thank you for the suggestion. I just tried that and sleep_length_ms fluctuates between 16 and 17 so that seems fine. When the animation finally starts to stutter, sleep_length_ms is still 16 or 17. I also added a check to let me know when Sleep() makes the thread oversleep and that doesn't seem to be the problem either. So confusing, it's probably a silly mistake somewhere that I can't spot.

Try this with my timer.

GameTimer FrameTimer;
while(run_game) {
if(FrameTimer.HasElapsed(60))       //Render every 60ms
{
	update_game();
	render_game();
}
else
{
Sleep(0)
 }
}

I was using something similar to the code you posted but the CPU usage was around 25%. I know that there isn't exactly anything wrong with that and it's normal for games but I wanted to try and write a CPU efficient timer.

You can maybe get away with using Sleep(1).

I would decouple update_game() from render_game(). Such that update_game() works independently on FPS. Which means you need to pass it the elapsed time every time you call it and only update incrementally based on elapsed time. That way animation will be smooth and consistant regardless of FPS. This will cure your stuttering problems.

GameTimer FrameTimer;
GameTimer UpdateTimer;
while(run_game) {

update_game(UpdateTimer.Tick());

if(FrameTimer.HasElapsed(20)) {
	render_game();
} else {
Sleep(1);
 }

}

I considered using Sleep(1) but I believe it'll make thread oversleep fairly often. Another thing I tried was make the thread sleep for 70% of the required time and then rely on a do while loop for the remainder of the duration. That was fairly accurate and the dropped the usage to 10%. I'll follow your advice and decouple the physics/logic and rendering, I have read the fix your timestep article and implement that but my GPU's fan started making an unpleasant noise (the whole reason why I got obsessed with CPU/GPU usage).