Problem with simple timer function

I'm writing a simple function to regulate frames per second, but somethings going wrong and as far as I can see it should work. Heres the code:

Code:

void FPS_Cap(unsigned cap)
{
    static unsigned this_frame;
    unsigned last_frame, time_taken;

    cap = 1000 / cap;
    last_frame = this_frame;
    this_frame = SDL_GetTicks();
    time_taken = this_frame - last_frame;
    
    if(time_taken >= cap) return;

    SDL_Delay(cap-time_taken);
}

For those of you that don't use SDL SDL_GetTicks() is like a portable version of clock(), and SDL_Delay() sets the prog to sleep for however many millisecs.

When using this in a simple program capped at 60 the frame rate is only being restricted to ~95. In theory it should only be possible to be <= 60. Is there something wrong with my maths here?

Originally Posted by mike_g

When using this in a simple program capped at 60 the frame rate is only being restricted to ~95. In theory it should only be possible to be <= 60. Is there something wrong with my maths here?

Perhaps it's a clock granularity issue.
I assume you are asking for a 16ms delay (1000/60) but it seems you are
getting, say, 11 or 12. Try hardcoding the delay to 20 or 30ms and see if it slows down.

hmm. I don't think it has to do with the accuracy of the clock. If I set the FPS cap at 10 i get an actual fps of 18 which, proportionately, is even more out of sync than before.

It does seem, tho', that oogabooga is about right - why not do the following:
- Check the result of your calculations [as in print/log your SDL_GetTicks() and SDL_Delay() results].
- Check if the SDL_GetTicks() actually give the correct number when you get back from SDL_Delay().

--
Mats

It seems to me you have some logical problem

You enter this function say at 40ms
then store this moment
then decide, that you need to sleep for 20ms
then start drawing your frame (that takes for example 15ms)
then you go back to your function but to determine the required delay you do not take the moment when the frame was started to process (60ms) but the moment when you previously entered the funtion 40ms... so your new delay will be incorect IMHO

Ok, I wrote a small test app for it:

Code:

#include "SDL/SDL.h"
#include <stdio.h>

void FPS_Cap(unsigned cap)
{
	static unsigned this_frame;
	unsigned last_frame, time_taken;
	
	cap = 1000 / cap;
	last_frame = this_frame;
	this_frame = SDL_GetTicks(); 
	time_taken = this_frame - last_frame;

	if(time_taken >= cap) return;
	
	SDL_Delay(cap-time_taken);	
}


int main()
{
	unsigned int now, i;
	
	printf("Using Frame Cap Function\n");
	for(i=0; i<10; i++)
	{
		now = SDL_GetTicks();
		FPS_Cap(60);
		now = SDL_GetTicks() - now;
		printf("%i\n", now);
	}

	printf("Using Fixed Delay\n");	
	
	for(i=0; i<10; i++)
	{
		now = SDL_GetTicks();
		SDL_Delay(16);
		now = SDL_GetTicks() - now;
		printf("%i\n", now);
	}	

	return 0;
}

And I got some weird output:

Code:

Using Frame Cap Function
0
16
0
16
0
27
0
17
0
17
Using Fixed Delay
16
17
16
16
16
16
16
16
17
16

For some reason every second time the frame cap function runs I get no delay Also, once, it seems to delay quite a bit too long. Can anyone see why this is happening, or what I could do to fix it?

It seems to me you have some logical problem

You enter this function say at 40ms
then store this moment
then decide, that you need to sleep for 20ms
then start drawing your frame (that takes for example 15ms)
then you go back to your function but to determine the required delay you do not take the moment when the frame was started to process (60ms) but the moment when you previously entered the funtion 40ms... so your new delay will be incorect IMHO

If I understand you correctly, that shouldent cause too much of a problem, this function is only intended to be run once, at the end of a loop after all drawing is done. It is however pretty crude as it uses the average frame time; if one or two frames take abnormally long to draw the the FPS will get slowed down.

Why not print what the different parts are inside your function FPS_Cap() too?

--
Mats

Ok here some output listing all the variables in the function:

Code:

Using Frame Cap Function
Cap: 16 Last: 0 This: 2025589929 Time Taken: 2025589929
 Cap: 16 Last: 2025589929 This: 2025589929 Time Taken: 0
 Cap: 16 Last: 2025589929 This: 2025589945 Time Taken: 16
 Cap: 16 Last: 2025589945 This: 2025589952 Time Taken: 7
 Cap: 16 Last: 2025589952 This: 2025589961 Time Taken: 9
 Cap: 16 Last: 2025589961 This: 2025589968 Time Taken: 7
 Cap: 16 Last: 2025589968 This: 2025589986 Time Taken: 18
 Cap: 16 Last: 2025589986 This: 2025589986 Time Taken: 0
 Cap: 16 Last: 2025589986 This: 2025590002 Time Taken: 16
 Cap: 16 Last: 2025590002 This: 2025590002 Time Taken: 0
 Using Fixed Delay
16
16
16
16
18
16
21
16
17
16

As you can see the times returned seem to be wrong, and it may well have to do with clock granularity, but it leaves me asking the same question: What can I do about it?

Cheers.

Average over a longer time-frame (e.g. calculate the cap per every 5 frames or some such)?

--
Mats

Yeah that should be better, I'll have a go at it and see what it come out like. I'm a little disappointed at how inaccurate the clock is :'(

It's a logic error! (Sorry about the clock granularity red herring.)
The problem is that you are saving (in the static variable) the time
BEFORE your call to SDL_Delay(). You need the time AFTER the call.

you're right oogabooga! Nice one

[edit] Oh I guess that was what vart was saying too [/edit]

Ok, I'm still clueless. oogaboogas suggestion must be right, but now the function always delays 16ms regardless of how many millisecs have already gone by.

At first I thought it would be because the first time the function runs 'this_frame' is not initialized; therefore it wont get set anytime soon.

So if (time_taken >= cap) i get the time for 'this_frame' before returning. I thought that would have fixed it, but it dosent

It seems I am still doing something wrong here, anyone know what it is?

Code:

void FPS_Cap(unsigned cap)
{
	static unsigned this_frame;
	unsigned last_frame, time_taken;
	
	cap = 1000 / cap;
	last_frame = this_frame;
	time_taken = this_frame - last_frame;

	if(time_taken >= cap) 
        {
 	       this_frame = SDL_GetTicks();                
               return;
	}
	SDL_Delay(cap-time_taken);	
	this_frame = SDL_GetTicks(); 
}

I wrote my own sync function once. Here it is:

Code:

	// Let's set it to update 50 frames/sec.
	const DWORD dwTimeFrame = 1000; // The time frame we're targeting (in ms)
	const DWORD dwFramesPerTimeFrame = 50; // Number of times per time frame cycle to do rendering
	const DWORD dwTickRate = dwTimeFrame / dwFramesPerTimeFrame; // Calculate amount of time to sleep each time
	INT32 nSleepNeeded; // Stores the amount of time we need to sleep
	DWORD dwTimeElapsed; // Time elapsed since beginning of time frame
	DWORD dwNextSleepBoundary; // The next "target" time to sleep until.
	DWORD dwTick; // Used to hold a snapshot of timeGetTime so we can calculate elapsed time
	DWORD Count = 1; // Simple counter just for cosmetics - just to print something
	DWORD dwTick2 = timeGetTime(); // Snapshot of the beginning of the code sample

	for(;;)
	{
		// Initialize variables
		nSleepNeeded = 0;
		dwTimeElapsed = 0;
		dwNextSleepBoundary = dwTickRate; // Set the next sleep boundary to the next tick.
		dwTick = timeGetTime(); // Get a snapshot of the current time
		while (dwNextSleepBoundary <= dwTimeFrame)
		{
			// Call rendering here
			dwTimeElapsed = timeGetTime() - dwTick; // Calculate elapsed time
			nSleepNeeded = dwNextSleepBoundary - dwTimeElapsed; // Calculate amount of time we need to sleep (at least)
			if (nSleepNeeded > 0) // Safesty check; we don't want to do Sleep(0) or sleep a negative value since Sleep takes a DWORD
				Sleep(nSleepNeeded); // Sleep for the amount of time necessary before next drawing
			dwNextSleepBoundary += dwTickRate; // Increase sleep "boundary" to next time frame tick
			cout << Count++ << endl; // Print counter
		}
		DWORD dwTick3 = timeGetTime() - dwTick2; // Calculate amount of time the entire loop took
		cout << "Took " << dwTick3 << " ms!\n"; // Print out the time it took
		__asm int 3; // Do a debug break
	}

You could theoretically just switch the clock and the sleep since it shouldn't matter much.
I don't know if it will help.

Cool, and thanks. I was hoping to get my function working with everything wrapped up inside it, but it seems it wont work as I have to calculate 'time_taken' after I get the time and before I call the delay.