Fix overflow when using sf::Clock for long time

[Mozzg]

• Has this change been discussed on the forum or in an issue before? It was an issue.
• Does the code follow the SFML Code Style Guide?
• Have you provided some example/test code for your changes?
• If you have additional steps which need to be performed list them as tasks!

---

Description

This PR is related to the issue #1765

When using sf::Clock class, method restart() may return negative value in continious work because of overflow when calculating current time in microseconds. This pull request is trying to fix that.

Tasks

• Tested on Linux
• Tested on Windows
• Tested on macOS
• Tested on iOS
• Tested on Android

How to test this PR?

See description in issue #1765

[Sakarah]

I am worried about the fact that this PR introduces floating point computation to resolve the overflow problem. This might be creating dangerous side effects, since precision now depends on the order of magnitude of the number and the division will be rounded to nearest even instead of floored.

I strongly suggest trying to fix the problem with integer arithmetic only instead.

[Mozzg]

@Sakarah This PR does not introduce floating point computation, because there was one to begin with, I only rearranged the order of operations. In return statement there was division, so it is floating point computation and it ended up casting to integer type anyway. Also I didn't see that SFML needed something more precise then microseconds, so any potential precision problems will not affect anything.
In the end the minimum time period is 1 microsecond and any precision problems will affect something much much less than that.

[Sakarah]

Well, an integer division is clearly not a floating point operation. It is the arithmetically correct division that you can add back with the remainder to perfectly restore the original number.
The fact that you had to introduce a .0 and a static_cast actually stems from the fact that you actually did not simply reorder the operands.

After thinking about it, I guess your fix is OK as it does not loose precision as long as you do not exceed 52 bits in the result. This is I admit very unlikely for a result in microseconds. (2^52 ms is over 100 years)

[Mozzg]

The introduction of .0 and static_cast is simply to avoid warnings. I thought it was a good programming practise to do those things. If you want I can remove them. I mainly program on Delphi, so I may not know some specifics.

As for floating point and precision I have to disagree, but let's move on. Do you have some suggestions on how we can avoid those operations? I can't think of one.

[Sakarah]

Yes obviously you want to explicit the conversions as you did, the warning says that the operation exist but is not written so the code might be misleading if you don't write these things.

Where do you disagree ? I just said that precision is fine until the result time is greater than 52-bits. And this is arguably enough. In case I wasn't clear I changed my mind about the necessity of pure integer arithmetic in this case. I was actually more worried than against. A comment justifying that a 52-bit precision provided by double is enough might be a good idea to convince maintainers if they are as picky as me.

Just for information, if you wanted to do this with pure integer arithmetic, you can use a code such as:

Int64 full_seconds = time.QuadPart / frequency.QuadPart;
Int64 remainder = time.QuadPart % frequency.QuadPart;
return full_seconds * 1000000 + remainder * 1000000 / frequency.QuadPart;

However I benched the performances out of curiosity, and your code is actually faster on x86_64, gcc -O3 than both the current SFML and my solution. Floating point multiplication is incredibly cheap compared to 64 bit integer division.

[Mozzg]

@Sakarah Thank you for replies. I hope this PR will be added to SFML as soon as posible, Clock class is very convenient to use.

I didn't think of your solution and maybe I think I understand your concern. In my mind every division operation is a floating point operation, because even when two operands are integers, result is a floating point type.

[Bromeon]

@Mozzg Thanks a lot for your PR. Did this code solve the problem you had in #1765?
I'll try to test it myself soon.

In my mind every division operation is a floating point operation, because even when two operands are integers, result is a floating point type.

No, integer division is a different machine instruction than floating-point division, see e.g. DIV and FDIV. The result of integer division is again an integer, not a floating point type.

Also, could you squash your commits into one (git rebase or the UI of your choice)? For additional changes, you can use git commit --amend to modify the last commit. Let me know if you need help with that.

[eXpl0it3r]

Also, could you squash your commits into one (git rebase or the UI of your choice)? For additional changes, you can use git commit --amend to modify the last commit. Let me know if you need help with that.

If you don't know how to do that, don't worry about it. Since you allowed editing of the branch by the maintainers, I can align the commits the way we usually handle them. But if you want to learn more about git, feel free to do the changes. 🙂

[Mozzg]

@Bromeon @eXpl0it3r
I tried to merge commits into one but failed and ended up creating more commits. I did not worked with GitHub on this level, so I'll leave it to you. I also adjusted the changes.

I did not used this exact code in my project, rebuilding SFML would be too much work. I just replaced sf::Clock class and time manipulation with this adjusted code. I tested the code by adding a constant, I wrote this test method in my issue #1765. The run/test without using a constant (but only with custom code without Clock class) is going right now and results should be known in 3 days.

My code looks like this:

LARGE_INTEGER clockFreq;	
LARGE_INTEGER clockMainNow;
LARGE_INTEGER clockMainPrev;
LARGE_INTEGER clockMainElapsed;
QueryPerformanceFrequency(&clockFreq);
QueryPerformanceCounter(&clockMainNow);		
clockMainPrev.QuadPart = clockMainNow.QuadPart;
clockMainElapsed.QuadPart = 0;
double clockReverseFreq = 1000000.0 / clockFreq.QuadPart;
sf::Uint64 elapsedMicro = 0;
...
while (running)
{
	...
	QueryPerformanceCounter(&clockMainNow);
	if (clockMainNow.QuadPart < clockMainPrev.QuadPart)  //just in case
	{
		clockMainElapsed.QuadPart = 0;				
	}
	else
	{
		clockMainElapsed.QuadPart = clockMainNow.QuadPart - clockMainPrev.QuadPart;
	}
	clockMainPrev.QuadPart = clockMainNow.QuadPart;			
	elapsedMicro = static_cast<sf::Uint64>(clockMainElapsed.QuadPart * clockReverseFreq);
	...
}

[eXpl0it3r]

I squashed the commits and rebased onto master.

[Bromeon]

Looks good to me 👍🏼

I ran the tennis example for a few minutes on Windows (with a displayed clock), and seems to work well from what I can tell. I didn't let it run long enough to trigger the overflow, though 😉