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 to Lua library df time class #3907

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

dhthwy
Copy link
Contributor

@dhthwy dhthwy commented Oct 20, 2023

Initial work.

@dhthwy dhthwy marked this pull request as draft October 21, 2023 19:38
@dhthwy dhthwy force-pushed the add_library_time_class branch 2 times, most recently from 56cc90a to 13693b6 Compare October 22, 2023 15:45
@dhthwy
Copy link
Contributor Author

dhthwy commented Oct 22, 2023

  • 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
Copy link
Contributor Author

dhthwy commented Oct 23, 2023

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
Copy link
Member

myk002 commented Oct 25, 2023

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

Screenshot 2023-10-25 12 08 06 AM

library/lua/time.lua Outdated Show resolved Hide resolved
library/lua/time.lua Outdated Show resolved Hide resolved
library/lua/time.lua Outdated Show resolved Hide resolved
library/lua/time.lua Outdated Show resolved Hide resolved
library/lua/time.lua Outdated Show resolved Hide resolved
function Time:getMonths() -- >>int<< Months as age (not including years)
return math.floor (self.ticks / TU_PER_MONTH)
end
function Time:getYears() -- >>int<<
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@dhthwy dhthwy Oct 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@myk002 myk002 Oct 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sgtm

library/lua/time.lua Outdated Show resolved Hide resolved
@myk002
Copy link
Member

myk002 commented Oct 25, 2023

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

library/lua/time.lua Outdated Show resolved Hide resolved
@dhthwy
Copy link
Contributor Author

dhthwy commented Oct 30, 2023

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
Copy link
Contributor Author

dhthwy commented Oct 30, 2023

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
Copy link
Member

myk002 commented Oct 30, 2023

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

@dhthwy
Copy link
Contributor Author

dhthwy commented Nov 3, 2023

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.

library/lua/datetime.lua Outdated Show resolved Hide resolved
library/lua/datetime.lua Outdated Show resolved Hide resolved
@dhthwy
Copy link
Contributor Author

dhthwy commented Nov 7, 2023

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
Copy link
Contributor Author

dhthwy commented Nov 8, 2023

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
Copy link
Member

myk002 commented Nov 8, 2023

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

@dhthwy dhthwy marked this pull request as ready for review November 11, 2023 20:11
Comment on lines 289 to 331
function DateTime:addTicks(ticks)
if not ticks then qerror('ticks must be a number') end
self.year_tick = self.year_tick + ticks
self:normalize()
return self
end
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@@ -3928,6 +3928,211 @@ Examples:
end
unit.body.blood_count = math.min(unit.body.blood_max, unit.body.blood_count + healAmount)

DateTime
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
DateTime
datetime

Comment on lines 3935 to 3936
The *DateTime* and *Duration* objects in this module support *addition*, *subtraction*,
and *<, <=, >, >=, ==* equality operators
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

and *<, <=, >, >=, ==* equality operators

DateTime class
==============
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
==============
--------------


:year: A year. Defaults to zero.
:year_tick: A year tick. Defaults to zero.
:ticks_per_day: The rate at which time passes in a day.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

-- the current game time.
local dateTime = DateTime.now()
-- construct and set year to 1, year_tick to 1
dateTime = DateTime{ year = 1, year_tick = 1 }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

:ticks_per_day: The rate at which time passes in a day.
Defaults to DWARF/fortress mode.
note: if ticks_per_day is given a value <= 1,
it will default to fortress mode.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 4081 to 4085
* ``dateTime:normalize()``

Ensure's year_tick is within bounds of a year, otherwise
adjusts year and year_tick accordingly. Note: should only
call this function when directly modifying year_tick.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'

Comment on lines 4102 to 4103
Duration Class
==============
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Duration Class
==============
Duration class
--------------

Comment on lines 4124 to 4125
DwarfCalendar
=============
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
DwarfCalendar
=============
DwarfCalendar class
-------------------

Comment on lines 4105 to 4106
Inherits DateTime's constructor and methods. Used for measuring the difference
between DateTime and Duration objects.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

dhthwy commented May 7, 2024

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.

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

Successfully merging this pull request may close these issues.

3 participants