Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Android time issue #131. #132

Merged
merged 2 commits into from Aug 18, 2019

Conversation

@and3md
Copy link
Contributor

and3md commented Aug 18, 2019

As I say in #131 Androdid have three clocks. I found that clock_gettime(CLOCK_MONOTONIC) uses clock that we need (https://stackoverflow.com/questions/3832097/how-to-get-the-current-time-in-native-android-code).

Because Android is linux I use clock_gettime(CLOCK_MONOTONIC, @tp); from linux module. This works for me. I played 2 hours my game without any freezes :) Tomorrow will do more test. Please test this too on your projects/devices.

Kraft

I also had to fix kraft. Do we use any special version? If I do PR in Kraft repo, will I be able to just use the latest version in CGE?

Copy link
Member

michaliskambi left a comment

Excellent finding! Thank you, it never occurred to me that we may have another clock on Android that is nicely monotonic.

I added some comments -- only nitpicking (as usual) :)

As for Kraft: the current version of src/physics/kraft/ should be an unmodified version from upstream, https://github.com/BeRo1985/kraft . You should make PR to fix this in Kraft too, and then CGE version of kraft.pas and upstream should be equal. (Until it is accepted in Kraft, we can of course have a fixed version just in CGE, as you did.)

(but we neved guaranteed in the interface that it's thread-safe...). }
(but we neved guaranteed in the interface that it's thread-safe...).
This variables can be removed once everyone confirms that the solution is
always working.

This comment has been minimized.

Copy link
@michaliskambi

michaliskambi Aug 18, 2019

Member

Improve the comment "solution" -> "solution of using CLOCK_MONOTONIC", to remember in the future what solution does this mention :)

@@ -26,7 +26,7 @@ interface

uses
{$ifdef MSWINDOWS} Windows, {$endif}
{$ifdef UNIX} BaseUnix, Unix, Dl, {$endif}
{$ifdef UNIX} BaseUnix, Unix, Dl, {$endif} {$ifdef ANDROID}linux, {$endif}
SysUtils, Math, Generics.Collections,

This comment has been minimized.

Copy link
@michaliskambi

michaliskambi Aug 18, 2019

Member

Linux (from uppercase), we write all unit names CamelCase :)

begin
FpGettimeofday(@tv, nil);
{ Android have three clocks we need use clock which never leaps forward

This comment has been minimized.

Copy link
@michaliskambi

michaliskambi Aug 18, 2019

Member

"have" -> "has" (3rd person singular in English uses "has").

).
Maybe they synchronize the time from the Internet, and do not take care
to keep it monotonic (unlike https://lwn.net/Articles/23313/ says?) }
{ I leave this test to check the solution, but it really isn't needed anymore }
if Result.Value < LastTimer.Value then

This comment has been minimized.

Copy link
@michaliskambi

michaliskambi Aug 18, 2019

Member

Here also improve comment "solution" -> "solution of using CLOCK_MONOTONIC".

@michaliskambi michaliskambi referenced this pull request Aug 18, 2019
@and3md

This comment has been minimized.

Copy link
Contributor Author

and3md commented Aug 18, 2019

Should be OK now. Thanks for the corrections :)

@michaliskambi michaliskambi merged commit b795533 into castle-engine:master Aug 18, 2019
@michaliskambi

This comment has been minimized.

Copy link
Member

michaliskambi commented Aug 18, 2019

Merged both your PRs. Thank you!!!

@and3md and3md referenced this pull request Aug 20, 2019
and3md added a commit to and3md/castle-engine that referenced this pull request Sep 3, 2019
and3md added a commit to and3md/castle-engine that referenced this pull request Sep 3, 2019
michaliskambi added a commit that referenced this pull request Sep 8, 2019
Removed unneeded (after #132) time measurement test on Android.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.