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 Time.get_unix_time_from_system() not including msecs #64101

Merged
merged 1 commit into from
Aug 8, 2022

Conversation

timothyqiu
Copy link
Member

Fixes #64059

OS::get_unix_time() is still uint64_t on 3.x. So a dedicated OS::get_subsecond_unix_time() is added.

This new function is intended to be used only by Time so it's not exposed to scripting.

@timothyqiu timothyqiu added bug topic:core cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release labels Aug 8, 2022
@timothyqiu timothyqiu added this to the 3.6 milestone Aug 8, 2022
@timothyqiu timothyqiu requested review from a team as code owners August 8, 2022 10:54
@akien-mga
Copy link
Member

I would also document the difference between OS.get_unix_time() and Time.get_unix_time_from_system() because it's confusing that they don't give the same result.

@timothyqiu
Copy link
Member Author

Added a note for OS.get_unix_time().

Time.get_unix_time_from_system() already has a note about it being different from other integer timestamps.

doc/classes/OS.xml Outdated Show resolved Hide resolved
@akien-mga akien-mga merged commit 6eee835 into godotengine:3.x Aug 8, 2022
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 3.5.1.

@akien-mga akien-mga removed the cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release label Aug 8, 2022
@timothyqiu timothyqiu deleted the subsecond branch August 8, 2022 15:21
@RPicster
Copy link
Contributor

I think this is a pretty bad change to be honest. It's a breaking change and it would have deserved an own function.
We were now searching for a new bug since changing to 3.5.1 until we found out about this.

@timothyqiu
Copy link
Member Author

The documentation of Time.get_unix_time_from_system() is not changed. Although the fraction part is zero, the value returned is also a float in 3.5. So I think this is a bugfix of the implementation rather than a change in interface.

@azagaya
Copy link
Contributor

azagaya commented Oct 24, 2022

The documentation of Time.get_unix_time_from_system() is not changed. Although the fraction part is zero, the value returned is also a float in 3.5. So I think this is a bugfix of the implementation rather than a change in interface.

In 3.5 this code:

func _ready():
	var timestamp = Time.get_unix_time_from_system()
	print(timestamp)

returns this:

1666621969

In 3.5.1:

1666622043.819422

Both of them are floats as you mention. As we were converting to string to use inside custom expressions, and then parsing it, that "." was causing bugs that werent in 3.5

@timothyqiu
Copy link
Member Author

When converting a float to string, how to handle the fraction part should always be considered I think.

Since the documentation in 3.5 mentioned that the return value is in seconds and explained it's float for sub-second precision, I believe the bug in 3.5 hide the bug in your code, and fixing the engine bug exposed the bug in your code rather than causing it.

@azagaya
Copy link
Contributor

azagaya commented Oct 25, 2022

yeah you are right. Thanks for taking time to answer!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants