Skip to content

Update core clocks & time usage#7500

Merged
zach2good merged 20 commits intoLandSandBoat:basefrom
cocosolos:steady-clock
May 6, 2025
Merged

Update core clocks & time usage#7500
zach2good merged 20 commits intoLandSandBoat:basefrom
cocosolos:steady-clock

Conversation

@cocosolos
Copy link
Copy Markdown
Contributor

@cocosolos cocosolos commented Apr 27, 2025

I affirm:

  • I understand that if I do not agree to the following points by completing the checkboxes my PR will be ignored.
  • I understand I should leave resolving conversations to the LandSandBoat team so that reviewers won't miss what was said.
  • I have read and understood the Contributing Guide and the Code of Conduct.
  • I have tested my code and the things my code has changed since the last commit in the PR and will test after any later commits.

What does this pull request do?

This switches the server clock from the system clock to a steady clock.

Tasks: 0ms, Network: -2552ms, Total: -2551ms, Diff/Sleep: 2751ms

The system clock is susceptible to changes caused by NTP. This means that the clock can occasionally move backwards. We do not want that for measuring durations, so we use the steady clock. This clock only ever moves forward, but the actual value of the time point is meaningless and inconsistent between reboots. So we use the steady clock to measure durations while the process is running, and use the system clock to persist a point in time to the database (or send it to Lua). The system clock is also used when we need a specific real world time, such as the current UTC or JST time.

Two helper namespaces have been added. The ::timer namespace is a wrapper around the steady clock and has utilities to convert the steady time to and from UTC time, and for counting the seconds or milliseconds in a given duration. The ::earth_time namespace is a wrapper around the system clock and contains namespaces for UTC, JST, and local timezones which all have helper functions for getting time information (such as current hour, minute, second, day of the week, etc). earth_time also has helper functions to return a Unix timestamp, a Vana'diel timestamp in Earth seconds since the Vana'diel epoch, the current "game" weekday (Monday-Sunday), and a time point for the start of the next "game" week (Monday at midnight JST).

A Vana'diel clock class has been added to the ::xi namespace. This allows us to use actual Vana'diel time durations directly, down to the Vana'diel millisecond. A ::vanadiel_time namespace has been added to help working with this. It has helper functions to convert the Vana'diel time to/from Earth times/durations as well as the same time information helpers the earth_time namespaces have, as well as namespaces for moon and RSE information. Care must be taken when working with Vana'diel time and converting to Earth/timer time and durations (std::chrono::seconds() and ""s works for earth_time and timer durations, vanadiel_time uses xi::vanadiel_clock::seconds()).

With all of this in place, I went through and replaced every instance I could find of an integer being used as a time value with the actual time types. This means it is no longer necessary to manually convert between e.g. milliseconds and seconds by multiplying and dividing. Internally everything is either a time point or duration and can use the helper namespaces or the std chrono library. When an integer is needed such as when reading/writing to the database, Lua, or packets, only then should these time types be floored to the appropriate duration and converted to an integer representation.

There are 2 exceptions that were not converted. Gardening furnishing items read/write a Vana'diel timestamp from their exdata. This doesn't get saved into any kind of gardening class or struct so it's fine to just stay as an integer. The other exception is weapon/base delay. These are eventually converted into time durations, but they remain integers until after all the calculations are done. See #2502, it seems there's still some unknowns and what we have is best guess so I'm not touching that.

Other than just converting, I was able to clean up the logic in some places and made some changes where I thought could be improved. This includes rearranging how some of the hourly ticks happen (like RoE). I don't think there is any negative impact, but there is a lot here and it's always possible I missed something. I made very few changes to scripts, but did change a few things that were using the Vana'diel day (1-360) when the days since epoch is more appropriate. I also cleaned up a few quests/missions that were using extra char vars and complicated conversions to count days (VanadielTime() is basically the Vana'diel equivalent of os.time() and should be used with the vanaTime enums).

Steps to test these changes

  • (Temporarily) enable performance logging to confirm values make sense.
  • Try the !time command in game.

image

Lot's of stuff to test:

  • Vana'diel time Lua bindings (!time)
  • Status effects
  • Attack delays
  • Spells cast and recast, interrupts
  • Death homepoint time
  • Mob pathing and combat
  • Conquest (fixes 🐛 [Conquest] Tally results differ in chat & in Region map #4517)
  • /playtime
  • Abilities recast
  • Charged abilities (sic, stratagems, quickdraw)
  • /heal ticks
  • Ranged attacks
  • EXP/CP chains
  • Watchdog
  • Petskills
  • BCNM
  • Guild pattern
  • Transport/elevators
  • Usable items
  • Weaponskills/skillchains
  • Drops and treasure pool
  • Rate limiting packets (/jump)
  • Raise/weakness
  • Synthesis
  • Fishing
  • Quests/missions
  • Instances
  • xi.mob.phOnDespawn (night/daytime only spawns)
  • ???

There's probably more. It'll probably be obvious if something doesn't work. Still in the process of thoroughly testing but everything seems to be working right.

@cocosolos cocosolos force-pushed the steady-clock branch 2 times, most recently from 2818abf to 32e7ab9 Compare April 27, 2025 19:17
@cocosolos
Copy link
Copy Markdown
Contributor Author

cocosolos commented Apr 27, 2025

Clang doesn't have time zone support until clang19 😭 (only up to 18.1.8 on the runner)

Update: Linux builds fine on clang 18.1.3 so I changed the MacOS build to use clang 18.1.8 (-DCMAKE_C_COMPILER=$(brew --prefix llvm@18)/bin/clang -DCMAKE_CXX_COMPILER=$(brew --prefix llvm@18)/bin/clang) and it still fails.

@cocosolos cocosolos force-pushed the steady-clock branch 4 times, most recently from e5e89f2 to 4462d3e Compare April 28, 2025 19:33
@zach2good
Copy link
Copy Markdown
Contributor

so I changed the MacOS build to use clang 18.1.8

I know OSX is a pain in the ass and generally we run quite far behind for compilers and dependencies on all platforms, but I'd rather we stick to what is defaultly available from our CI images so we provide maximum ease of use and compatibility. Nobody wants proper C++20 and C++23 support more than I do ;_;

Copy link
Copy Markdown
Contributor

@zach2good zach2good left a comment

Choose a reason for hiding this comment

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

Great work, just some general style points about comments and const auto, but the content is all great 👍

Comment thread src/world/time_server.cpp Outdated
Comment on lines +48 to +50
earth_time::time_point jstTime = earth_time::time_point(timer::to_utc(tick));
auto jstHour = earth_time::jst::get_hour(jstTime);
auto jstWeekday = earth_time::jst::get_weekday(jstTime);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

these can all be const auto

Comment thread src/world/time_server.cpp Outdated
auto jstWeekday = earth_time::jst::get_weekday(jstTime);

// Static variable for the next tick
static earth_time::time_point nextHourlyTick = std::chrono::ceil<std::chrono::hours>(jstTime);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

static const auto

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is updated during ticks so went with static auto.

Comment thread src/world/time_server.cpp Outdated
// 4-hour RoE Timed blocks
static time_point lastTickedRoeBlock = tick - 1h;
if (CVanaTime::getInstance()->getJstHour() % 4 == 0 && CVanaTime::getInstance()->getJstMinute() == 0)
/**
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we use blocks of // comments instead of these /**/ comments please?

Comment thread src/world/time_server.cpp Outdated
Comment on lines +105 to +107
vanadiel_time::time_point vanaTime = vanadiel_time::from_earth_time(jstTime);
vanadiel_time::TOTD vanaTotd = vanadiel_time::get_totd(vanaTime);
auto vanaHour = vanadiel_time::get_hour(vanaTime);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

const auto

Comment thread src/world/world_server.h Outdated

#include "http_server.h"

static constexpr auto kTimeServerTickInterval = 2400ms;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do these need to be visible in the header? They were in an anon namespace in world_server.cpp because they were only needed there and wrapped to be inaccessible anywhere else

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I moved this so I could reference it in the time server but I ended up changing that and didn't need it so I'll move it back.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Moved this back but kept the name change so it matches the map constants.

Comment thread src/common/vana_time.h
===========================================================================

Copyright (c) 2010-2015 Darkstar Dev Teams
Copyright (c) 2025 LandSandBoat Dev Teams
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You've essentially completely rewritten this, you don't need to keep around the DSP attribution unless you want to

cocosolos added 20 commits May 1, 2025 14:33
server_clock is our internal "tick" clock and we want it to be monotonic
for duration measurements. Status effects and jug pets use system_clock
only to perform offline tick calculations. These don't have to be as
exact as tick duration measurements.
`./tools/run_clang_format.sh`
Gut timer.h and replace it with a timer namespace for steady_clock. Use
this wherever possible. VanaTime still needs work.

timer::clock (std::chrono::steady_clock)
utc_clock (std::chrono::system_clock)
Rename timer::clock::now() to just timer::now().
inline const for the start_time means we shouldn't need the init or
final functions anymore.
Move all of the JST and system time calculations out of the Vana'diel
time class. Get rid of most C-style time handling. Add some helpers for
calculating weekly reset and other game related real-world times. Probably
fixes some issues.

Still need to update status effects, recast times, and other misc things
to use proper time values. Should only be using ints when sending to Lua
or the database. Everything in core should be using durations and the
proper clock.
This includes cast and recast times, battlefields, instances, status
effect durations and tick times, respawn times, animation times etc.
Everything that is a time duration should be using a time type and not
an integer.

There are some exceptions:
- Charged abilities are misusing the recast field.
- Items use Vana'diel time for timing.
- Traverser stones, database timestamp/datetime fields, exception logging.
- Remove CVanaTime.
- Refactor world and map time servers.
- Remove option for custom Vana'diel epoch.
- !time GM/general purpose test command.
doExpiredTasks should return the actual time it takes to complete.
Clamp the result to give the network phase 50-150ms. Move the watchdog
update call to after reporting performance statistics. This way the tick
will complete _then_ either the watchdog kills the process for taking too
long, we sleep for the remainder of the tick, or just loop, reporting
degraded performance if it's in the acceptable range (<100ms).
Switch day checks to VanadielUniqueDay (a count of days since epoch).
VanadielDayOfTheYear is almost never the right function to use.
Use Vana'diel timestamp and time enums in some places instead of trying
to calculate how much time has passed by storing a bunch of variables.
Make death time and playtime use the steady timer. Remove and replace
remaining uses of ctime outside of the dedicated time namespaces.
Use chrono for (almost) everything.
We appear to be ahead 1 day on the rank of the requested guild point item.
Fix issue with AH pagination module.
Fix some clang tidy warnings.
Fix issue with escaped quotes getting picked up in Lua CI checks.
Clang isn't ready for these yet. Comment them out and use a different
approach until they're ready.
@cocosolos
Copy link
Copy Markdown
Contributor Author

Rebased to include the battleutils changes in #7506 (simple change from static_cast to chrono::floor) + requested changes. I'm gonna say this is ready enough and if anything breaks just ping me and I'll fix it (xi.mob.phOnDespawn is the only thing I'm not really sure on and that's only because I don't understand what was going on there before, but based on what I think it should be doing it should work right).

@cocosolos cocosolos marked this pull request as ready for review May 1, 2025 19:28
@zach2good
Copy link
Copy Markdown
Contributor

Nice work!

Yeet!

@zach2good zach2good merged commit c21f9de into LandSandBoat:base May 6, 2025
13 checks passed
@sruon sruon mentioned this pull request Aug 18, 2025
4 tasks
@cocosolos cocosolos deleted the steady-clock branch December 2, 2025 17:48
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.

🐛 [Conquest] Tally results differ in chat & in Region map

2 participants