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

Script API: more DateTime factory methods #2551

Merged

Conversation

ivan-mogilko
Copy link
Contributor

@ivan-mogilko ivan-mogilko commented Oct 11, 2024

Implements factory methods that create DateTime from full date/time, and from "rawtime".
Creating from normal date/time values may be useful to compare another DateTime object with certain "base point".
All of these may be useful to create DateTime back from previously saved values, either calendar values or rawtime.

/// Creates DateTime object from the provided calendar and, optionally, time components
static DateTime* DateTime.CreateFromDate(int year, int month, int day, int hour = 0, int minute = 0, int second = 0); 
/// Creates DateTime object from the provided raw time value (in seconds)
static DateTime* DateTime.CreateFromRawTime(int rawTime);

@ericoporto
Copy link
Member

ericoporto commented Oct 11, 2024

Uhm, so using this approach it internally saves the date in RawTime (Unix Time), so if you do something like this

  DateTime* d = DateTime.CreateFromDate(1969, 5, 22, 8, 27, 2);
  player.Say("Now it's %02d:%02d:%02d, from day %d/%d/%d", d.Hour, d.Minute, d.Second, d.DayOfMonth, d.Month, d.Year);
  player.Say("Rawtime is %d", d.RawTime);

It will show 0 for all fields and minus 1 to RawTime. I would suggest either holding the data internally in some other format and converting to RawTime if requested (on property read) or error the CreateFromDate factory method if the year is lower than 1970.

(ah, I actually thought that Unix Time could support negative numbers to go back reverse from 1970, but apparently for some reason it can't, I guess that is useful in the end because we gain -1 as meaning of invalid time)

@ivan-mogilko
Copy link
Contributor Author

ivan-mogilko commented Oct 11, 2024

Internally it always saves full list of values, regardless of how it's set. Nothing is changed there, and nothing is changed in the supported range of values.

@ericoporto
Copy link
Member

ericoporto commented Oct 12, 2024

Internally it always saves full list of values, regardless of how it's set. Nothing is changed there, and nothing is changed in the supported range of values.

I don't understand, it's giving me 0 for everything in the example I just listed, it should either error or give a warning that year below 1970 is not supported.

image

@ivan-mogilko
Copy link
Contributor Author

ivan-mogilko commented Oct 12, 2024

Yes, it does not work for anything below year 1970, because it fails to initialize "raw unix time".
At first I thought to save date even in that case, but then reconsidered, because that creates an inconsistent struct.
So, in a way, everything is based on whether it may be represented by "raw time", if that's what you meant by "internally saves the date in RawTime". But it's been always like that, nothing changed in that regard, there's no new "approach".

I can add a warning if the datetime initialization failed.

EDIT: documentation for C++11 std::chrono::system_clock also mentions that it begins counting starting from 1970:
https://en.cppreference.com/w/cpp/chrono/system_clock
so, if a bigger range is wanted, then this struct will have to be implemented differently, allow rawtime to be invalid, or change its meaning, or replace it completely. I was not planning such change here.

@ivan-mogilko
Copy link
Contributor Author

So, I see now, the situation is worse, CreateFromTime will not work, because it uses zero year, month and day.
Somehow I thought that will work, but it won't.
My initial idea was to let use this struct for time comparison, similar to how std::chrono::time_point can be used, but it does not work like that.

@ivan-mogilko ivan-mogilko marked this pull request as draft October 12, 2024 01:36
@ivan-mogilko ivan-mogilko marked this pull request as ready for review October 12, 2024 03:12
@ivan-mogilko
Copy link
Contributor Author

ivan-mogilko commented Oct 12, 2024

I had to remove CreateFromTime, and only leave CreateFromDateTime.
Unfortunately, if we keep the DateTime struct as it is, it's not possible to use it to store arbitrary time of day without a date.

Perhaps, it may be worth to plan a different API, something similar to how C++ (or C#) deals with this, where there's a arbitrary TimePoint, and TimeSpan, and a structure is not required to be a valid calendar date. But that's something for the future.

Added year check (idk what the max year is though), and also added another warning in case ScriptDateTime failed to init.

@ericoporto
Copy link
Member

idk what the max year is though

// NOTE: subject to year 2038 problem due to shoving seconds since epoch into an int32

I tried to set 2038 and it rolled raw time to a negative number. I guess that means that 2038 should be the limit as it seems it is flipping the rawtime signal - I am not sure this is the same in all operating system implementation, I swear I used something in the past that negative unix time put me before 1970.

@ivan-mogilko
Copy link
Contributor Author

ivan-mogilko commented Oct 12, 2024

I do not see a reason to bother about dates below 1970. Because this struct is centered over 32-bit rawtime, the available range of dates will always be small, it cannot represent any arbitrary date. In its current design it's usable to compare "current time" with previously saved "current time", or compare file times, and neither of them can be below 1970.

@ericoporto
Copy link
Member

ericoporto commented Oct 12, 2024

I think I would also make the script RawTime not take negative values to ensure we are not relying on platform specifics there, since it looks like the RawTime is an unsigned int behind the scenes, at least testing the Windows builds.

I wonder if it's best to give an error message for values out of range rather than debug warn if this struct ever gets reused to support something different behind the scenes - DateTime is a good name. But this could just have a version check if this gets done, so not sure here.

I do not see a reason to bother about dates below 1970

My bother is mostly because I am trying to avoid platform specifics to appear in the user facing script interface.

@ivan-mogilko ivan-mogilko force-pushed the 362--datetimectors branch 2 times, most recently from 41d2282 to 116b9bb Compare October 12, 2024 14:35
@ivan-mogilko
Copy link
Contributor Author

After some thought, I made it to explicitly create an "invalid" DateTime object if either arguments are out of range, or it failed to initialize. AGS users don't like null pointers...

Invalid DateTime has date/time fields zeroed, and rawtime = -1.

@ivan-mogilko ivan-mogilko merged commit 66dbd33 into adventuregamestudio:master Oct 12, 2024
21 checks passed
@ivan-mogilko ivan-mogilko deleted the 362--datetimectors branch October 12, 2024 22:19
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.

2 participants