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

Add a Time singleton #49123

Merged
merged 1 commit into from
Jun 12, 2021
Merged

Add a Time singleton #49123

merged 1 commit into from
Jun 12, 2021

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented May 27, 2021

Note: This PR will fail the CI checks because @qarmin's test project uses the methods from OS. There is nothing that I can do about this, the test project will simply have to be updated after this PR is merged.

Implements and closes #37800, implements and closes godotengine/godot-proposals#2114.

This PR adds a new Time singleton for working with time. Some of these methods are moved out of _OS in core_bind (which is the same as the exposed OS but is not the same as the internal OS), and some are new methods (including those requested by proposal 2114).

Compared to the current master, this code:

  • Is a better abstraction from a user's perspective, since time isn't really related to operating systems.
  • Is a lot cleaner overall. Things are more clearly named, there are more comments, parameters start with p_ now, etc.
  • Has better documentation, with clear statements of what standards are followed and what is/isn't supported.
  • Has better error messages for when invalid values are passed.
  • Has many test cases, vs the current code which doesn't have any test cases.
  • Can handle arbitrarily large times, supports up to billions of years in the past or future.
  • Has more features, especially converting with ISO 8601 date and time strings.

There are many new methods added by this PR to suit many use cases. They handle 3 formats: the Unix timestamp, a datetime dictionary, and ISO 8601 date and time strings (without timezone, as it's not stored in the Unix timestamp). Not all are strictly necessary as they can be built from the others, but not nearly as efficiently (going from dictionary -> Unix timestamp -> string and vice versa requires calculating the Unix timestamp, going from Unix timestamp -> dictionary -> string and vice versa requires constructing a dictionary, and going from Unix timestamp -> string -> dictionary and vice versa requires constructing and decomposing a string and doing extra calculations to find the weekday if that's desired). Some can be removed if desired, but I suggest keeping them all as there is very little code duplication - the heavy lifting is abstracted to macros at the top of time.cpp.

In addition to converting time, several of the methods are for getting time information from the operating system, in which case they call OS - the internal one, the previously exposed methods were in _OS. Remember that these methods were already "duplicated" before (_OS and internal OS), they've just moved (so it's now Time and internal OS).

Open question: Timezones and DST are not handled. Should they be? It would add complication to the code and require the user to supply more information: what the timezone is, whether DST is active (can't have an "auto" because different countries have DST at different times, except it is possible only when getting the current time from the OS but this part is already handled), and whether the time should be converted back/forth/or not. It might be simpler for the end user to require users to manually add/subtract/append/trim the timezone before/after converting as necessary.

Open question: Every single method starts with get_. It might make sense to just exclude this prefix.

Testing and feedback would be appreciated.

@CsloudX
Copy link

CsloudX commented May 27, 2021

I think should a Date, a Time and a DateTime class. like Qt
All class has >,<, == etc operators.
All class has a current() method.
All class not a singleton.

This classes very useful for me.

@aaronfranke
Copy link
Member Author

aaronfranke commented May 27, 2021

@CsloudX Definitely worth considering, as that's kinda what JS and Qt do. I don't think we need 3 of them though.

However, for comparison purposes, you can just use the Unix timestamp which is an int. <, >, ==, etc works. For getting the current time, there is Time.get_unix_time_from_system() (in 3.x it's OS.get_unix_time).

@LikeLakers2
Copy link
Contributor

Open question: Timezones and DST are not handled. Should they be? It would add complication to the code and require the user to supply more information: what the timezone is, whether DST is active (can't have an "auto" because different countries have DST at different times, except it is possible only when getting the current time from the OS but this part is already handled), and whether the time should be converted back/forth/or not. It might be simpler for the end user to require them to add/subtract/append/trim the timezone before/after converting as necessary.

Timezones are hard, so we should probably let the OS handle that if at all possible. Otherwise, I would let the user handle it -- or alternatively, don't handle it at all.

@EricEzaM
Copy link
Contributor

Haha, I knew that was the Tom Scott video before clicking it.

I don't love how this uses so many massive macros. They could all easily be converted to methods which return a struct with year, month, day, h, m, s, etc, or int (i.e. in YMD_TO_DAY_NUMBER) which would help readability IMO.

@aaronfranke
Copy link
Member Author

aaronfranke commented May 27, 2021

@EricEzaM That would indeed make sense for some of them like YMD_TO_DAY_NUMBER and VALIDATE_YMDHMS, I think I got carried a way a bit with macros, I'll change it if others agree but as-is it's pretty elegant because we don't even need to pass a list of arguments. However, for the ones that need to return multiple fields I don't really want to make so many structs that will only be used a few times. For EXTRACT_FROM_DICTIONARY the code to extract the information from the struct isn't too much of an improvement over extracting from the dictionary manually.

@qarmin
Copy link
Contributor

qarmin commented May 27, 2021

I added workaround to CI, so it should pass now without any negative impact of this changes.

@dalexeev
Copy link
Member

dalexeev commented Jun 1, 2021

I think there should be a DateTime class (extends Reference) to work with datetime instances like in many other languages.

@Calinou
Copy link
Member

Calinou commented Jun 1, 2021

I think there should be a DateTime class (extends Reference) to work with datetime instances like in many other languages.

I think it's better to leave this to a separate PR (and proposal). Otherwise, we risk making this PR huge and rejected in the end.

@aaronfranke aaronfranke force-pushed the it-is-time branch 2 times, most recently from 1c792ed to 91a68f8 Compare June 9, 2021 18:27
@akien-mga akien-mga merged commit ac73059 into godotengine:master Jun 12, 2021
@akien-mga
Copy link
Member

Thanks!

@aaronfranke aaronfranke deleted the it-is-time branch June 12, 2021 21:09
@AndreaCatania
Copy link
Contributor

Looking at the function uint64_t get_ticks_msec() const; I've noticed that it's type was changed from uint32_t to uint64_t, there was any issue or why it's more preferable to use 64 bits in this case?

@Calinou
Copy link
Member

Calinou commented Jun 26, 2021

Looking at the function uint64_t get_ticks_msec() const; I've noticed that it's type was changed from uint32_t to uint64_t, there was any issue or why it's more preferable to use 64 bits in this case?

This was likely done to prevent an overflow if you have a Godot process that runs for more than ~49.7 days (which is possible with a dedicated server).

@fire

This comment was marked as resolved.

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