Experiment - refactoring IncreaseDate

Forum for technical discussions regarding development. If you have a general suggestion, problem or comment, please use one of the other forums.

Moderator: OpenTTD Developers

Post Reply
brandonr
Engineer
Engineer
Posts: 8
Joined: 10 Jan 2012 00:58

Experiment - refactoring IncreaseDate

Post by brandonr » 10 Jan 2012 09:02

Hi all, I'm new to the forum and new to OpenTTD. I've been studying the code for a few weeks and I noticed that a lot of game features are compiled directly into the game loop, with no way to turn them off or provide alternatives without lots of configuration flags or recompiling. For example, there is DisasterDailyLoop which runs on every new day, even if disasters are disabled. It checks _settings_game.difficulty.disasters, which is easy enough to change, but aside from changing that flag (or multiple flags) or installing a patch that changes disaster behavior, it is always tied into the daily loop. A better example is the code that marks town views dirty in case there was a snowline change, regardless what scenario you are running. To me that is a big warning sign.

I thought I would try to make the loops more abstract by refactoring some of the loop functions into an array of function pointers. Here's my current diff, which shouldn't change any game behavior at all. I'm using OSX, so the build/configure changes may not work for other systems, daily.h and daily.cpp are new files that need to get compiled in.

Feedback/comments/suggestions? There is plenty to do here, I am still tinkering.
Attachments
dailyloop-refactor.diff
Hopefully this will work
(4.78 KiB) Downloaded 98 times

User avatar
planetmaker
OpenTTD Developer
OpenTTD Developer
Posts: 9295
Joined: 07 Nov 2007 22:44
Location: Sol d

Re: Experiment - refactoring IncreaseDate

Post by planetmaker » 10 Jan 2012 11:24

Improvements to game speed are always welcome.

Do you have any testing data which prove one way or the other being advantegous?

TrueBrain
OpenTTD Developer
OpenTTD Developer
Posts: 1312
Joined: 31 May 2004 09:21

Re: Experiment - refactoring IncreaseDate

Post by TrueBrain » 10 Jan 2012 12:28

planetmaker wrote:Improvements to game speed are always welcome.

Do you have any testing data which prove one way or the other being advantegous?
This is very unlikely to be faster (instruction-wise). On the other hand, it also won't be much slower. But I also don't think that is his point, to make the game faster. More to make it more readable / sane.

What I like about this idea is, that it makes everything a bit more dynamic. It needs a few more refactoring to be more C++ (abstract class which gets added, which has a member which does the gameloop, for example, or even where you can specify DailyLoop, MontlyLoop, etc). It might just make the code more readable. But I am not sure yet what I like more.

Regarding your patch itself, there are some silly statements. First, _daily_calls for sure can be a dynamic array (a *_daily_calls, as you called it). But as we are programming in C++ here, it is much better to use a std::list (or any of his friends, I am guessing a std::vector would do? Not sure). There is no need to do this the C way.

Also, if you would be to go C++ style with classes, you can make the ctor automatically register itself to the dailyloop if needed, avoiding the AddDailyCall calls ;)

Just my 2 penny.
The only thing necessary for the triumph of evil is for good men to do nothing.

Simn
Engineer
Engineer
Posts: 15
Joined: 05 Jan 2012 19:43

Re: Experiment - refactoring IncreaseDate

Post by Simn » 10 Jan 2012 12:50

I'm new to OpenTTD development as well, but I had a quick look at your diff file. It seems that you initialize your daily callbacks in the main function, i.e. at program start. This defeats the purpose as all callbacks are immediately added, even the conditional ones like the disaster loop you mentioned. Your implementation provides no way of removing a previously added callback, so there is no way of reacting to a change, e.g. when disasters are turned on or off in the difficulty settings. For what you intend to do, you would have to observe changes to the disaster setting and add/remove the callback accordingly.

This would reduce branching in the daily loop, which is always a good thing. However, it is countered by an additional for loop (which the compiler cannot inline because _daily_calls_count is unknown) and additional dereferencing. It might still be beneficial to performance, you'd have to test that, but it's probably not worth the hooking into settings change.

As a side note, the order of commands in DisasterDailyLoop() seems strange. Shouldn't _settings_game.difficulty.disasters be checked first? This saves the pointless decrement operation if nothing else.

User avatar
Expresso
Tycoon
Tycoon
Posts: 1739
Joined: 09 Aug 2004 00:14
Location: Gouda, the Netherlands

Re: Experiment - refactoring IncreaseDate

Post by Expresso » 10 Jan 2012 14:57

Why not have all events (daily, monthly and yearly) pre-planned at the start of the year. You'd end up with a linked list of items to do which can simply be iterated through. At the end of the list an event could be attached to fill the new year. That way you don't need to figure out what needs to happen each day or whether or not a disaster occurs today, because that is already known.

brandonr
Engineer
Engineer
Posts: 8
Joined: 10 Jan 2012 00:58

Re: Experiment - refactoring IncreaseDate

Post by brandonr » 10 Jan 2012 16:16

TrueBrain wrote:
planetmaker wrote:First, _daily_calls for sure can be a dynamic array (a *_daily_calls, as you called it).
Give it a shot. It gives a compiler error, performing arithmetic on a function pointer is not allowed. I wrestled with this quite a bit, and though there is probably some truly horrible way to force it, it would be easier in the long run to wrap all the function calls inside objects.

brandonr
Engineer
Engineer
Posts: 8
Joined: 10 Jan 2012 00:58

Re: Experiment - refactoring IncreaseDate

Post by brandonr » 10 Jan 2012 17:03

Simn wrote:I'm new to OpenTTD development as well, but I had a quick look at your diff file. It seems that you initialize your daily callbacks in the main function, i.e. at program start. This defeats the purpose as all callbacks are immediately added, even the conditional ones like the disaster loop you mentioned. Your implementation provides no way of removing a previously added callback, so there is no way of reacting to a change, e.g. when disasters are turned on or off in the difficulty settings. For what you intend to do, you would have to observe changes to the disaster setting and add/remove the callback accordingly.
Even though the callbacks are set in the main loop, that doesn't prevent them from being subsequently changed - just reset the array and fill it up with new callbacks. I didn't do it here, it is just a demo, but it is now theoretically possible where it wasn't before.

TrueBrain
OpenTTD Developer
OpenTTD Developer
Posts: 1312
Joined: 31 May 2004 09:21

Re: Experiment - refactoring IncreaseDate

Post by TrueBrain » 10 Jan 2012 17:18

brandonr wrote:
TrueBrain wrote:
planetmaker wrote:First, _daily_calls for sure can be a dynamic array (a *_daily_calls, as you called it).
Give it a shot. It gives a compiler error, performing arithmetic on a function pointer is not allowed. I wrestled with this quite a bit, and though there is probably some truly horrible way to force it, it would be easier in the long run to wrap all the function calls inside objects.
Throughout the code of OpenTTD it is used many times, so the problem is at your end ;)

Code: Select all

typedef void (*LoopFP)();
LoopFP *_daily_calls;

void Test() {
  printf("Testn");
}

void main() {
  _daily_calls = malloc(sizeof(LoopFP) * 12);
  memset(_daily_calls, 0, sizeof(LoopFP) * 12);
  _daily_calls[0] = &Test;
  for (i = 0; i < 12; i++) {
    printf("%pn", _daily_calls[i]);
    if (_daily_calls[i] != NULL) _daily_calls[i]();
  }
}
This will most likely compile and work. Sometimes I love being a C programmer.

That said, this will not be accepted in OpenTTD any more, as we are C++ now. Using a std:: variant will be mandatory in these cases.
The only thing necessary for the triumph of evil is for good men to do nothing.

brandonr
Engineer
Engineer
Posts: 8
Joined: 10 Jan 2012 00:58

Re: Experiment - refactoring IncreaseDate

Post by brandonr » 10 Jan 2012 17:38

TrueBrain wrote:This will most likely compile and work. Sometimes I love being a C programmer.
_daily_calls = malloc(sizeof(LoopFP) * 12) says: invalid conversion from ‘void*’ to ‘void (**)()

Care to try again?

Simn
Engineer
Engineer
Posts: 15
Joined: 05 Jan 2012 19:43

Re: Experiment - refactoring IncreaseDate

Post by Simn » 10 Jan 2012 17:51

Expresso wrote:Why not have all events (daily, monthly and yearly) pre-planned at the start of the year. You'd end up with a linked list of items to do which can simply be iterated through. At the end of the list an event could be attached to fill the new year. That way you don't need to figure out what needs to happen each day or whether or not a disaster occurs today, because that is already known.
A linked list would contain mostly nothing and waste unnecessary memory. What would work is a std::map<Date, IDateEvent> which can be used to schedule events for certain days. IDateEvent would define an execute() method which is called by the day loop on the appropriate day, and could be implemented e.g. by a Disaster class. To enable multiple events on the same day, defining IDateEvent as a composite pattern would work, or using a std::map<Date, std::vector<IDateEvent> >.

This is just software theory though, I lack the knowledge of OpenTTD architecture to say if it's actually applicable and useful.

TrueBrain
OpenTTD Developer
OpenTTD Developer
Posts: 1312
Joined: 31 May 2004 09:21

Re: Experiment - refactoring IncreaseDate

Post by TrueBrain » 10 Jan 2012 19:05

brandonr wrote:
TrueBrain wrote:This will most likely compile and work. Sometimes I love being a C programmer.
_daily_calls = malloc(sizeof(LoopFP) * 12) says: invalid conversion from ‘void*’ to ‘void (**)()

Care to try again?
Lol. Tone down the attitude, is my first advise. And like said: this is C code. Not C++. C++ needs a cast. But that you could have figured out yourself.
The only thing necessary for the triumph of evil is for good men to do nothing.

brandonr
Engineer
Engineer
Posts: 8
Joined: 10 Jan 2012 00:58

Re: Experiment - refactoring IncreaseDate

Post by brandonr » 16 Jan 2012 20:38

Here is a revised version of the patch, which wraps each function calls in its own functionoid class. This allows you to pull out some of dependencies in date.cpp, since only the classes need to know where to find those symbols, and those classes can live somewhere else (here they live in daily.*).

This only modifies OnNewDay, but the same technique would apply to OnNewMonth and OnNewYear. StandardDay() is used to initialize the current set of daily actions, but in theory any module could manipulate the daily loop.
Attachments
dailyloop04.diff
patch for trunk (r23814 or so)
(5.64 KiB) Downloaded 72 times

brandonr
Engineer
Engineer
Posts: 8
Joined: 10 Jan 2012 00:58

Re: Experiment - refactoring IncreaseDate

Post by brandonr » 25 Jan 2012 20:52

Have you ever wanted to advance just one tick while the game is paused? Or advance several ticks without having to wait for the screen to draw each frame? Here's a little console function that will do that. Experimental, of course. Compile at your own risk.
x_tick.diff
x_tick console command
(850 Bytes) Downloaded 75 times
Limited to a max of 99 ticks.

Code: Select all

] x_tick 
] x_tick 1
] x_tick 99

brandonr
Engineer
Engineer
Posts: 8
Joined: 10 Jan 2012 00:58

Destroy All Trees!

Post by brandonr » 01 Feb 2012 18:29

The trees are just a problem. When you try to plant trees they are random after the first one. And the loop to grow tree tiles and the other loop that spawns new tree tiles don't have any configuration options, they are either on or off. But the devs aren't interested.

Until then, if you are brave enough to patch and compile, here is another console command, x_destroy_all_trees. It destroys all trees currently on the map. You can run it from the main menu, or while playing a game, or while editing a scenario. You could even load a saved game, run it, and resave the game.
x_console.diff
x_tick and x_destroy_all_trees
(1.34 KiB) Downloaded 60 times
This also includes the x_tick command from the previous post.

Post Reply

Return to “OpenTTD Development”

Who is online

Users browsing this forum: Google [Bot] and 4 guests