Add to Lua library df time class

[dhthwy]

Initial work.

[dhthwy]

• Pretty sure this needs an API doc and probably changelog entry.
• Need a consensus on the name. 'Datetime' may be more befitting.
• File placement is currently 'library/lua.' Is this the best place?

[dhthwy]

No clue why the ci build is failing. Builds locally.

Make Error at /home/runner/work/dfhack/dfhack/library/git-describe.cmake:33 (message): git-describe failed: 128

Maybe I am missing some magic git command to make it work?

[myk002]

I vote for datetime.lua. The location in library/lua is good.

There seems to be an unintentional change to the googletest dependency submodule. That should get reverted

[myk002]

why does getDays return a float and partial days but getYears does not? I'm not exactly sure which style is better, but I think they should at least be consistent.

[dhthwy]

I can't think of a good reason. I lean toward making everything a float for this stuff unless it absolutely makes no sense to do so. The user can always convert to int.

[dhthwy]

I'm thinking use case for this class will want an integer most of the time. But as we know, floats to ints can lose data. So to keep it consistent we either have methods for both floats and ints? Or add a parameter to the methods to indicate which type to return. Or use floats to preserve data and let the user convert.

So assuming we keep things simple and use floats, when the user needs ints, the code will look like:

string.format("%03d-%02d-%02d",agr_time:getYears()//1, agr_time:getMonths()//1, agr_time:getDayInMonth()//1) Or math.floor instead of //1

%.0f isn't a viable substitute due to rounding up and formatting.

These points are probably all obvious. Just thought I'd mention it regardless. Also my prior experience with Lua is close to 0.

[myk002]

another option is to return the result in two integer variables, whole and part. e.g.

function Datetime:getYears()
    return self.years, self.year_tick
end

callers can ignore the second return value if they just want the whole part. Lua is flexible like that.

[ab9rf]

DF consistently manages times as a pair of integers (years, year_tick). i strongly recommend that we assiduously use this format for representing times and time intervals and eschew entirely the use of floating point for this purpose

[dhthwy]

sgtm

[myk002]

API docs would go in docs/dev/Lua API.rst under the Lua Modules section
https://github.com/DFHack/dfhack/blob/develop/docs/dev/Lua%20API.rst

[dhthwy]

This iteration may be more or less palatable, although untested, I'm sure it has bugs :)

As soon as we get the API finalized, I'll write the docs. I don't have any problem changing things up till then.

[dhthwy]

DateTime as it currently stands is no good for measuring time spans between two times. I'm debating on adding a TimeSpan class for this purpose.

e.g. if we subtract times and call getMonth(). getMonth() returns the one-based month of the year. So 1 for the first month. But for time spans, we need it to be zero based.

We can keep it to a single class, but then we have one class trying to do two separate things.

[myk002]

Yeah, I think separate DateTime and Duration classes make sense.

[dhthwy]

There was some common code amongst the classes so I added a DwarfCalendar parent where I placed the various common utility/conversion/convenience functions -- not just for the child classes, but also for potential users. It was either that or move these functions outside the classes as local module functions.

Don't have a problem changing things up again if this approach isn't going to fly.

[dhthwy]

I started writing tests for this, and then I'll do the docs. Please let me know if you think this might need any major overhaul.

If we keep the Duration class vs duration methods in DateTime, it can technically represent a date and time, and still bound by the rules and system of the dwarven calendar, this is why I made it a child of DwarfCalendar.

In library/lua: dfcalendar.lua instead of datetime.lua might avoid conflating real time with dwarf time.

DwarfCalendar can also be augmented to provide info on seasons as well as the expected caravans for the seasons, if we want that stuff.

For performance, if this might be in hot zones, I suppose we can precompute or cache some of these date calculations whenever year_tick changes. We'll want to ensure the caller can't directly mutate self.year_tick (or warn callers, in the docs, not to).

[dhthwy]

Might be better implementing this in C++ so that it can benefit all. Given the simplicity of this class in its current form, it should be trivial. Tho, I'm mostly daft these days and it's been a few years since I've touched C++.

In any case, Lua is a great lang for prototyping.

[myk002]

All the current consumers of the logic are in Lua right now, so there's no reason to port it quite yet.

[dhthwy]

I'm not sure how robust we want our argument checking to be. We could do:

ticks = tonumber(ticks) if not ticks...

[myk002]

Suggested change

	DateTime
	datetime

[myk002]

Suggested change

	The *DateTime* and *Duration* objects in this module support *addition*, *subtraction*,
	and *<, <=, >, >=, ==* equality operators
	The ``DateTime`` and ``Duration`` objects in this module support *+*, *-*,
	and *<*, *<=*, *>*, *>=*, and *==* comparison operators

[myk002]

Suggested change

	==============
	--------------

[myk002]

I think we should either provide constants (e.g. datetime.TICKS_PER_DAY_FORT and datetime.TICKS_PER_DAY_ADVENTURE) or this property should be a boolean to differentiate between the two modes. I'm leaning towards the first one, since it is more flexible.

edit: actually, looking at the examples below, it looks like this effectively is a boolean. Calling it mode (and updating the description) might be clearer.

[dhthwy]

I'm not thrilled with the ticks_per_day name. Originally ticks_per_day was going to be a mode (I believe (df.game_mode is an enum with DWARF=0, ADVENTURE=1, AND LEGENDS=3). I think you mentioned letting the tick rate be customizable (if can change rate of time in game, then tick rate in a DateTime object may need to coincide).

We can substitute ticks_per_day for mode in the docs, but keep ticks_per_day (unexposed) as an object property so that setTickRate() can change it?

[myk002]

using 1 for both values may be confusing. how about using a more representative value for year_tick, like 30423 (or some other large but valid number)?

[dhthwy]

admittedly the examples are kinda crappy. I was just being lazy.

[myk002]

this section should also explain what normalization is in this context and when it is applied.

[myk002]

this should not need to be exposed. what reason would callers have to modify year_tick directly?

[dhthwy]

Doesn't need to be exposed. Thinking was: 'you shouldn't be assigning to year_tick yourself, but if you insist then here's this function'

[myk002]

Suggested change

	Duration Class
	==============
	Duration class
	--------------

[myk002]

Suggested change

	DwarfCalendar
	=============
	DwarfCalendar class
	-------------------

[myk002]

A lot of the DateTime instance functions don't really make sense for a Duration class. Would this be better the other way? Duration as the superclass and DateTime as the subclass?

[dhthwy]

The other way works. Some calendrical functions will need hoisting to Duration. And the addDays(), addMonths(), etc ones would probably be useful for Duration as well. In fact, it's not uncommon from implementations that I've looked at, to expose duration-style functions in their date/time objects.

It's either that, or DwarfCalendar can be the super class for common calendrical logic. I don't know if the calendar is actually moddable.

Additionally, Duration wouldn't need to know about the calendar at all if it didn't include magnitudes of months and years. Just some thoughts.

Thanks for looking at this.

[dhthwy]

Okay, in case it helps someone push this feature forward, I implemented the changes - I think. It didn't take long to do. Untested and tests weren't updated to reflect yet.

• note. the local functions at the top are common to both duration and datetime classes. I was going to make a new superclass to store these, but couldn't think of a good name for it.