Transport Tycoon Forums

The place to talk about Transport Tycoon
It is currently Mon Sep 01, 2014 11:26 pm

All times are UTC




Post new topic Reply to topic  [ 14 posts ] 
Author Message
PostPosted: Tue Jan 10, 2012 9:02 am 
Offline
Engineer
Engineer

Joined: Tue Jan 10, 2012 12:58 am
Posts: 8
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:
File comment: Hopefully this will work
dailyloop-refactor.diff [4.78 KiB]
Downloaded 75 times
Top
 Profile  
 
PostPosted: Tue Jan 10, 2012 11:24 am 
Offline
OpenTTD Developer
OpenTTD Developer
User avatar

Joined: Wed Nov 07, 2007 10:44 pm
Posts: 8269
Location: Sol d
Improvements to game speed are always welcome.

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

_________________
Image
OpenTTD: manual | online content | translations | Wanted contributions and patches
#openttdcoop: blog | wiki | public server | DevZone | NewGRF web translator
DevZone - home of the free NewGRFs: OpenSFX | OpenMSX | OpenGFX | Swedish Rails | OpenGFX+ Trains|RV|Industries|Airports|Landscape | NML


Top
 Profile  
 
PostPosted: Tue Jan 10, 2012 12:28 pm 
Offline
OpenTTD Developer
OpenTTD Developer

Joined: Mon May 31, 2004 9:21 am
Posts: 1254
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.


Top
 Profile  
 
PostPosted: Tue Jan 10, 2012 12:50 pm 
Offline
Engineer
Engineer

Joined: Thu Jan 05, 2012 7:43 pm
Posts: 15
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.


Top
 Profile  
 
PostPosted: Tue Jan 10, 2012 2:57 pm 
Offline
Tycoon
Tycoon
User avatar

Joined: Mon Aug 09, 2004 12:14 am
Posts: 1011
Location: Gouda, The Netherlands
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.


Top
 Profile  
 
PostPosted: Tue Jan 10, 2012 4:16 pm 
Offline
Engineer
Engineer

Joined: Tue Jan 10, 2012 12:58 am
Posts: 8
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.


Top
 Profile  
 
PostPosted: Tue Jan 10, 2012 5:03 pm 
Offline
Engineer
Engineer

Joined: Tue Jan 10, 2012 12:58 am
Posts: 8
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.


Top
 Profile  
 
PostPosted: Tue Jan 10, 2012 5:18 pm 
Offline
OpenTTD Developer
OpenTTD Developer

Joined: Mon May 31, 2004 9:21 am
Posts: 1254
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:
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.


Top
 Profile  
 
PostPosted: Tue Jan 10, 2012 5:38 pm 
Offline
Engineer
Engineer

Joined: Tue Jan 10, 2012 12:58 am
Posts: 8
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?


Top
 Profile  
 
PostPosted: Tue Jan 10, 2012 5:51 pm 
Offline
Engineer
Engineer

Joined: Thu Jan 05, 2012 7:43 pm
Posts: 15
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.


Top
 Profile  
 
PostPosted: Tue Jan 10, 2012 7:05 pm 
Offline
OpenTTD Developer
OpenTTD Developer

Joined: Mon May 31, 2004 9:21 am
Posts: 1254
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.


Top
 Profile  
 
PostPosted: Mon Jan 16, 2012 8:38 pm 
Offline
Engineer
Engineer

Joined: Tue Jan 10, 2012 12:58 am
Posts: 8
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:
File comment: patch for trunk (r23814 or so)
dailyloop04.diff [5.64 KiB]
Downloaded 48 times
Top
 Profile  
 
PostPosted: Wed Jan 25, 2012 8:52 pm 
Offline
Engineer
Engineer

Joined: Tue Jan 10, 2012 12:58 am
Posts: 8
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.

Attachment:
File comment: x_tick console command
x_tick.diff [850 Bytes]
Downloaded 52 times

Limited to a max of 99 ticks.

Code:
] x_tick 
] x_tick 1
] x_tick 99


Top
 Profile  
 
 Post subject: Destroy All Trees!
PostPosted: Wed Feb 01, 2012 6:29 pm 
Offline
Engineer
Engineer

Joined: Tue Jan 10, 2012 12:58 am
Posts: 8
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.

Attachment:
File comment: x_tick and x_destroy_all_trees
x_console.diff [1.34 KiB]
Downloaded 36 times

This also includes the x_tick command from the previous post.


Top
 Profile  
 
Display posts from previous:  Sort by  
Post new topic Reply to topic  [ 14 posts ] 

All times are UTC


Who is online

Users browsing this forum: lostwizard and 2 guests


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot post attachments in this forum

Search for:
Jump to:  

Powered by phpBB © 2000-2013 phpBB Group

Copyright © Owen Rudge/The Transport Tycoon Forums 2001-2013.
Hosted by Zernebok Hosting.