News:

Simutrans Tools
Know our tools that can help you to create add-ons, install and customize Simutrans.

Simutrans code

Started by ZéQuimTó, September 14, 2014, 11:16:42 PM

Previous topic - Next topic

0 Members and 2 Guests are viewing this topic.

ZéQuimTó

Hi.

Before starting to talk about simutrans code, I want to explain what made me post this topic in the first place.

As you can see in this thread, I am designing a way to ease the life of those who want to create either realistic maps, or draw the roads/cities on some image-editing software, and then import into simutrans.

The first thing I though of was: "ok, I probably just need to link my program against the objects containing load/save game functions, the classes to handle the world, and I should be good."

But no. When I tried to isolate these functions, I realised that all the simutrans code is completely entangled, and the dependencies between objects are completely "meshed" up. I ended up realising that there was some object that led to a dependency on "simmain", which was quite amusing. It meant that to create a simple tool that would only require a small subset of all that simutrans can do, I basically needed to include the whole simutrans within the tool.
I am right now implementing it as a new simutrans feature (for which I may or may not make a pull-request, depending if you think such feature would be useful or not)

Anyway, as I am implementing this feature, I came across some things in the code for which I would like your opinion. IMHO, I think that simutrans code is not "modular" enough, I think that the map is not properly isolated from the game itself.
But don't take my word for it, tell me what you think about this example:

karte_t::init
This function, that should be map-related, interferes with lot of stuff not map-related:
   It starts by muting sound  (sound-related)
   Resets or not server depending on network connectivity (network-related)
   hide pointer (GUI related)
   change viewport (GUI related)
   (then actually does world-related stuff, like generating the map, distributing trees and adding factories, etc. btw, some of these could be done in nested functions, not within the function itself (like resetting world settings, or adding industry, as it ruins readability)
   creates a loading-screen progressbar (GUI related)
   show pointer again (GUI related)
   unmute sound  (sound-related)

From google translate, I got that karte means "map", yet, it is within a file called simworld, and its class is much more than just a map class.
It turned a function that could just be to initialize a map, into something else that mutes the sound and so on and so forth..
Moreover, the map class encompasses ::interactive(), which is (from the little that I could understand) the core of the game.
Actually, this is the reason why when I tried to import only load/save functionality, the whole simutrans became a dependency.

Am I just being paranoid?


 
 



DrSuperGood

The map basically is the world kind of. Which also is the underlying element of a session.

I do agree that it is a total mess however.

As an unrelated example, factory outputs and inputs use the same class yet are functionally not the same. A result is that Simutrans allocates factory input members for factory outputs which are never used wasting memory.

Another example is that any I/O is done with some mess of conditional statements for each version. The result is the more revisions to I/O, the bigger the I/O code gets and so the slower loading will be (as these are evaluated at run time). The correct thing to do would be define some form of migration mutator for each revision, a small piece of code that takes in old formatted data and produces new formatted data that is added to some kind of table which is executed sequentially before loading old data. This would decouple version management from the I/O of individual types keeping them clean and efficient.

Personally I think that simutrans locks too many resources to a single thread. An example is that simutrans cannot be minimized (from full screen) when loading a map at certain stages. This is best seen when doing anything network related (checking games list as an example) where the process can be declared unresponsive by the OS it takes so long to update.

ZéQuimTó

Yes, I also noticed the lack of abstraction for versions and so on.
Another thing, is the fact that the map generation and map-enlargement are the same function...I would imagine it implemented separately.
Something like a simple "mapgen" function. For map enlargement, such function would be called only to generate the new area.

The more I look into the code, the more I want to clean it...but I am afraid that such would be an enourmous task, and quite useless (vs adding new functionality). Plus, current devs would like to keep it as it is, because the code might be ugly, but proven, and if it got refactored, they would have to re-learn the architecture.... I don't know...


Ters

karte_t has been cleaned up a bit lately, so just imagine what a monster it was less than a year ago. In fact, game_t would have been a more appropriate name. It's the holder for the entire game state.

Quote from: DrSuperGood on September 15, 2014, 01:02:56 AM
Another example is that any I/O is done with some mess of conditional statements for each version. The result is the more revisions to I/O, the bigger the I/O code gets and so the slower loading will be (as these are evaluated at run time). The correct thing to do would be define some form of migration mutator for each revision, a small piece of code that takes in old formatted data and produces new formatted data that is added to some kind of table which is executed sequentially before loading old data. This would decouple version management from the I/O of individual types keeping them clean and efficient.
I'm not so sure about that. Some of the conversion might depend on data loaded elsewhere and/or correcting the loaded data after everything has been loaded. I have seen that in pak loading. Transforming the data to an intermediate format will consume more memory and take more time, and I find huge maps very slow to load already (and that is not because of all the ifs, they are not that costly).

Besides, I don't find the ifs that disturbing. And it documents the variation of the file format nicely. Having umpteen conversion functions or classes, one for each version, doesn't seem very clean either. Chaining them all would be slow to run, and updating all to target the latest version would be tedious to maintain.

But I'm interested in more details as to how to do this. Support for multiple versions of a file format is something I've never found a good solution to, although I've mostly struggeled with WIP file formats, where the format can change every week.

Quote from: DrSuperGood on September 15, 2014, 01:02:56 AM
Personally I think that simutrans locks too many resources to a single thread. An example is that simutrans cannot be minimized (from full screen) when loading a map at certain stages. This is best seen when doing anything network related (checking games list as an example) where the process can be declared unresponsive by the OS it takes so long to update.
Actually, I don't think the example has anything to do with locking things to a single thread. It's just that someone didn't bother spin up a worker thread to do potentially time consuming tasks. Simutrans is capable of keeping the GUI reponsive to OS events while loading pak sets and when loading games. That's a common sin.

Markohs

#4
 About it not being modular/decoupled, I completely agree, I did some work to make this better, but certainly need a *LOT* more work. Believe me it was even worse past years. I decoupled:

- Loading screen out of simgraph/simworld, improving its even management too (to a certain point, but even games like StarCraft II suffer from some kind of lock up of event processing when loading/saving, so it's quite ok right now IMHO).
- Event management out of simworld.cc to siminteraction.cc
- Camera management out of simworld.cc to display/viewport.cc

More code has been moved out by other developers, too, iirc.

Still, have to agree with you this needs a *ton* of extra work, and it's important imho. Anyway, This idea is not popular among some of our developers, that tend to like huge source files, and not "spread" code around. I can't agree on that but on the other hand I have to agree in that code is stable, and it's working, so why changing something that's working? My guts tell me the code is wrong, and I'd happily go and refactor many parts, but it's hard to make it, because some of the refactoring could imply a degradation of performance in some cases, and it's a quite ingent amount of work, plus in this community all huge changes require consensus in developers (in wich prissi often has the last word), and this consensus is hard to reach.

About german in code (and documentation), a lot of work has been donde to make it better, again, it was much worse in the past.

TLDR version: Good luck if you try to change some of this, but our developers are quite flexible on reasoned proposals, if you got something in mind, the work will be incorporated, at least partially. :)

EDIT:
As I see it, karte_t, simworld.cc, is basically a class to *CONTAIN* the map, and all objects included in it. Plus, it handles the logic of world time stepping (map item "movement"), taht's what iirc you are referring by interactive().

iirc, I had a plan to move all this code to another class, some like map_scheduler_t(), but never managed to finish that project. There are many other things that can be taken out of that monstruous class that's karte_t.

The game also lacks of a proper game_loop_t , of the idea of sub-systems, and code isolation. It lacks a "central" part, because simmain.cc doesn't really do it (it maybe should do it), the code jumps around.

About threads: Threads didn't even exist when this game was created, it's normal it's not designed to take advantage of them, our current thread systems was designed like 2 years ago, and it's specifically designed to handle just the display.

But as I said, it's old code, it's working and performing good and it's not bad designed, having in mind how many work has been receiving. But it's quite "exotic", one of a kind. Have in mind this game worked on MS-DOS.

As I said, i'd suggest anyone interested into changing this, to make a proposal, but have in mind game speed has to be the same, or better, never worse.

EDIT2:

Quote from: DrSuperGood on September 15, 2014, 01:02:56 AM
An example is that simutrans cannot be minimized (from full screen) when loading a map at certain stages.

Again, this was worse time ago, when the loading screen was on, the game did not process events, so no minimize/resize/maximize/click could not be done on it while the screen was on.  Also, this events caused simutrans as be marked as unresponsive in Windows, for example. Now it's checked each update (not ideal, but good enough).

Ters

Quote from: Markohs on September 15, 2014, 12:03:08 PM
About threads: Threads didn't even exist when this game was created, it's normal it's not designed to take advantage of them[...]
Simutrans isn't that old. Threads have been around since at least Windows 95. Simutrans is from 1997 according to copyright.txt. However, very few used threads (explicitly) back then. Least of all for performance reasons on consumer level machines, although doing some work on the CPU while another thread did I/O was established.

Quote from: Markohs on September 15, 2014, 12:03:08 PM
Have in mind this game worked on MS-DOS
It might still do.

Other than that, you're triple posting. There are rules against that.

Dwachs

Over the years karte_t worked as a container to throw in everything that should work on global level plus even more. This is just organic growth.

I certainly agree that the structure can be improved: among other things karte_t contains
-- the map composed of tiles with some extra information on climate and water, various accessor functions, terraforming
-- the functions to generate the map
-- global lists of convoys, factories, players with their passwords
-- the game loop with a lot of timing logic.

This could be split up, ofc.
Parsley, sage, rosemary, and maggikraut.

prissi

karte_t evolved over a long time. Originally every object had a map pointer (which with nowaday millions of objects would wast tons of memory and clogging caches). So yes, thing evolve organically. Ironically the display functions are the ones with the least change even though they are so not render-compatible ... Indeed, until version 82.xx started to use SDL it run only under DOS (and could not use more than 64 MB main memory under DOS or windows 3.11). pthreads were not working on windows until the around 2006. Until then almost any processor had only a single core, and hyperthreading of pentium 4 were slower than a good single thread program.

To be honest: The core of Simutrans is not uptodate. Also doing the GUI with its own dialogues is something with these many crossplattform GUI libs one would not do today. If I would start simutrans from scratch I would think of using rather an advanced game engine (and much more supporting libraries) and just reuse the object part (and part of the passenger routing).

However, in the end separating stuff is a personaly taste. I am very oldschool, so I am ok with long functions doing a lot of work. So I do not mind the code. Other like stronger OOP approach, which is the current mainstream (for quite some time). If you come up with a clever way to separate stuff and the code is not more difficult or buggy than before, why not.

With some stuff I would probably disagree: A map generator function needs to call the progress indidator. How else should it indicate progress? But apart from that I think more rewarding would be implementation of new stuff (a list of all vehicles in the pak using the renovated scrolled lists for instance).

ZéQuimTó

Quote from: prissi on September 15, 2014, 09:19:35 PM
With some stuff I would probably disagree: A map generator function needs to call the progress indidator. How else should it indicate progress? But apart from that I think more rewarding would be implementation of new stuff (a list of all vehicles in the pak using the renovated scrolled lists for instance).

Pass a progress callback function as parameter. That is what I would do...
When I finish doing what I am doing now, I might try to sketch what I would do to karte_t, and post a proposal somewhere, so that you guys could make suggestions, and vote for it.


prissi

I personally consider a callback function as evil a goto or void pointers. Simutrans tries to use those all only for special reasons ...

A callback function could jump anywhere on the actual calltime. An OOP way would be inheritance of the callback from a class (that would be the clear way). Also normal callback functions need to be either static C code.

But it is entirely possible that I understood your intentions wrong.

ZéQuimTó

#10
You are right. Sorry, I just rushed for an answer. But the idea remains: the "progress bar" should be something abstract. Overriding a progress-function is certainly clearer. Other functions could also be provided, like "loading finished" and things similar (the ones that might be useful, oc), if an asynchronous approach is to be taken.

EDIT: However, after thinking better about it, overriding the whole karte_t might not yet be the ideal approach, because it does not make much sense to create a different super-class each time we want a different progress handler... moreover, it would mean that such class with a different handler would have a different type, which could complicate some things... I will think a bit more about this, perhaps looking for some examples.

EDIT2:
This one seems a nice way to do it. (It may look overkill, because the example is too simple)
(Source: http://stackoverflow.com/questions/3520133/object-oriented-callbacks-for-c)
Basically the idea is to use an "Interface" class, and that is the one which shall be overridden. Not the whole "User" class.
This way, it is avoided to have classes like "map_GUI_progressbar", "map_ASCII_progressbar", "map_beer_meter_progressbar"....

#include <iostream>

class Interface
{
  public:
  virtual void callback() = 0;
};

class Impl : public Interface
{
  public:
  virtual void callback() { std::cout << "Hi from Impl\n"; }
};

class User
{
  public:
  User(Interface& newCallback) : myCallback(newCallback) { }

  void DoSomething() { myCallback.callback(); }

  private:
  Interface& myCallback;
};

int main()
{
  Impl cb;
  User user(cb);
  user.DoSomething();
}


IgorEliezer

Quote from: Ters on September 15, 2014, 02:33:10 PMOther than that, you're triple posting. There are rules against that.
Ters, you probably might want to re-quote the relevant parts once the links back to those in your post ("Quote from:") are now broken.




Just a side-note, for all: while we have a 24h-timespan exception for double posts, this is not written on rock. Sometimes double-post can be forgiven if it is redeemable or necessary, that is, there is enough relevant content and good time span (~12h, the period the people usually visit the forum regularly), and/or because of urgency or public interest etc. If you can edit your post and it won't cause any loss, double-post must be avoided; forum is not a chat.

Markohs

#12
Quote from: IgorEliezer on September 16, 2014, 08:15:31 AM
Just a side-note, for all: while we have a 24h-timespan exception for double posts, this is not written on rock. Sometimes double-post can be forgiven if it is redeemable or necessary, that is, there is enough relevant content and good time span (~12h, the period the people usually visit the forum regularly), and/or because of urgency or public interest etc. If you can edit your post and it won't cause any loss, double-post must be avoided; forum is not a chat.

I'm sorry about that triple post, I was writing as ideas came to mi mind and saw things to reply. I don't really mind reading double, triple or quadruple posts, and don't really understand what's the huge problem about them since all the posts were big enough, not just simple comments. Anyway I understand rules are rules and I broke them, I'm sorry.

Btw Ters, I know threads are very old, but they were not widely used when Simutrans was born. When I said "threads didn't exist", I meant "threads were not widely used", I was abusing language, I guessed it was and evident subject. According the wikipedia the idea of threads is implemented since 1950, and implementations were available in OS/2, Windows NT and Windows 95, and many other systems. But were not so common, in 1997.

EDIT: About ZéQuimTó suggestion:

I see it more like this task whould be done decoupling the code much more and with a more OOP solution, each "long_task_t", that's a "task_t", scheduled by a "task_manager_t", if wants a visual representation, will instance a "loadingscreen_t" that will be initialized with as callback, or be a sub-class of loadingscreen_t, whatever, something in that line. Using OOP, and modern patterns. What how I shink it should be, more or less.