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

Change: Split ScriptDate:: into Calendar and Economy variants #11588

Closed
wants to merge 24 commits into from

Conversation

2TallTyler
Copy link
Member

@2TallTyler 2TallTyler commented Dec 14, 2023

Motivation / Problem

#10700 splits off economy and calendar time. #11341 makes these time units not follow each other, and #11428 allows them to tick at different rates.

It's time we break the news to Game Scripts and AI.

Description

This PR depends on and is built atop #10700.

This PR splits ScriptDate:: into ScriptDateCalendar:: and ScriptDateEconomy::, mirroring #10700's split of TimerGameCalendar and TimerGameEconomy.

All methods are duplicated, except for GetSystemTime(), which only exists in ScriptDateEconomy::. It could be split off into some new class if desired.

Internal calls to ScriptDate:: things are redirected to the correct class.

Okay, what about existing scripts? Well, the most crucial information for GS and AI is knowing how time in the game is progressing to grow towns, build new train lines, give the player new goals, etc. If calendar time can be frozen, those scripts break.

For this reason, we map all ScriptDate:: functions to ScriptDateEconomy::. More in this in Limitations.

Limitations

  • If a script looks for a specific calendar date rather than using dates only to measure the passing of time (I can't think of any examples, but a hypothetical example would be building industries based on real-world economic history) will behave strangely. They won't crash, but when OpenTTD is using real-time units, economy starts in year 1 and the script will react accordingly. I am not aware of any ways to split time without the potential of breaking scripts in this way. 😢
  • I have no idea how to test this or verify if my code is correct, since ScriptDate ignores the strong-type protection of our two time systems, and compat files are not built or verified by OpenTTD.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR touches english.txt or translations? Check the guidelines
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, game_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@2TallTyler 2TallTyler added the needs review: Script API Review requested from GS/AI expert label Dec 14, 2023
@2TallTyler 2TallTyler added this to the 14.0 milestone Dec 14, 2023
@2TallTyler 2TallTyler marked this pull request as draft December 14, 2023 23:52
Copy link
Contributor

@rubidium42 rubidium42 left a comment

Choose a reason for hiding this comment

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

I'm not so sure whether mixing calendar and game time in the API in this way is useful for backward compatibility. You are, as seen from the API, returning vastly different dates that can't be reasonably compared.

Having said that... can't (old) scripts just always get the economy date and work just fine? Except when they code something to happen on a specific date of course.
Providing access to the calendar date would be useful for those specific instances, so maybe we can do a company/async mode-type trick? If the calendar-date-mode object is at the top, it returns and expects calendar dates, otherwise it will expect economy dates.

@@ -5,13 +5,13 @@
* See the GNU General Public License for more details. You should have received a copy of the GNU General Public License along with OpenTTD. If not, see <http://www.gnu.org/licenses/>.
*/

/** @file script_date.hpp Everything to query and manipulate date related information. */
/** @file script_date_economy.hpp Everything to query and manipulate date related information. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Given date stuff is split over two files, it's definitely not 'everything' anymore.


#include "script_object.hpp"
#include "timer/timer_game_calendar.h"
#include "../../timer/timer_game_economy.h"

/**
* Class that handles all date related (calculation) functions.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would expect some documentation here (and in script_date_calendar.hpp), what is the difference between the two types of dates. Given you, as user, usually only see one I would expect this needs to be clarified for anyone writing an AI or GS.

@2TallTyler
Copy link
Member Author

2TallTyler commented Dec 15, 2023

Ah, excellent points. Yes, in compatibility mode other functions shouldn't internally reference the calendar date. I guess I can define an economy time version of these other functions in the compat files, unless there's a better way.

I don't understand the trick you propose to detect which type of date the script is looking for. I have not touched scripts much at all. 🙂

Edit: putting this here for myself later, I also need to do the AI changelog.

Outside of Real-Time Mode, these are the same date, so debugging date-based events is still possible.
@rubidium42
Copy link
Contributor

I don't understand the trick you propose to detect which type of date the script is looking for. I have not touched scripts much at all. 🙂

In scripts there are some "Mode"s that can be set. For a GS, you can set the CompanyMode which changes as which company the commands are executed. Furthermore there is an AsyncMode, which allows you to make executing commands non-blocking (but you never get the actual results of the commands, except by probing the state later). Similar to that there are TestMode and ExecMode, which allows you to create a number of commands in TestMode to see whether it's allowed without actually building, and by switching to ExecMode you can make the execute again.

Note that most modes are scopes, so when the Mode objects are removed from the stack, the state reverts back to what it was. So, in the script I can just open a new CompanyMode, run a few commands and when the function returns the old company to run as is restored.

In the context of the calendars I would imaging have a "mode" for each of the calendars, so the script can choose what it wants to read or pass to the API. It's up to the API to do the appropriate conversion when needed. However, it allows us to just default to EconomyTimeMode and when the script is aware of the differences it can read/write using CalendarTimeMode as well.
In the API this means that no backward compatibility scripts are needed, while adding support for different time modes to consider. I can imagine that some scripts want to be able to convert between the dates, which would be some new API that's added.

@2TallTyler
Copy link
Member Author

2TallTyler commented Dec 15, 2023

Ah, so your proposal is to leave ScriptDate:: as one class and have each function return a different type based on the time mode of the API call?

My current implementation matches the base game implementation of TimerGameCalendar and TimerGameEconomy as parallel classes with mirrored methods (inherited in the timers, duplicated in script API). I've been working within this model for months so it makes the most sense to me, but I recognize my bias -- and laziness, having already written the compat files. 😛 Maybe script authors would prefer a mode-based distinction. If we decided to go the mode-based route, I would need some pointers to how modes are defined and read, to do this properly.

The compat files I just pushed don't work, so let me know if I should fix them or do something else. 😃

@glx22
Copy link
Contributor

glx22 commented Dec 17, 2023

Don't forget Squirrel doesn't really have a ScriptDate[Calendar|Economy]::Date type, that's why all functions returning ScriptDate[Calendar|Economy]::Date use .base().

The scoped mode solution seems more simple for compatibility and for script authors.

@2TallTyler
Copy link
Member Author

Yes, I am coming to the same conclusion. I will do that as a separate PR to leave this intact, just in case.


/* static */ ScriptDateEconomy::Date ScriptDateEconomy::GetCurrentDate()
{
return (ScriptDateEconomy::Date)TimerGameCalendar::date.base();
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you plan to do it differently, but looks like it's using the wrong timer, and it's the same in all other functions (except for parameter types).

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, faulty (user of) find-and-replace. But yes, I'm going to do this the other way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review: Script API Review requested from GS/AI expert
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants