Merge-from-standard -- accounting merger is working now

[neroden]

James, I think I've finally got the accounting code from standard merged correctly.

This is on the merge-from-standard branch.    Anyway, you may want to merge this into your 112-x-private-car-merge branch.

The accounting change touched enough different subsystems that there were a lot of little bugs to stamp out.

I like my new credit limit system.  But you may think it is simply too generous on credit (it allows a great deal of credit for highly profitable companies). That can be fixed without changing the underlying system, by changing the percentage of profit which is allowed to go to interest coverage.

Currently (on my branch), the player is allowed to borrow until half his profits (based on profits from the last 12 months) are dedicated to interest payments, and doesn't go bust until all his profits are dedicated to interest payments *and* his net worth is negative.

[jamespetts]

Thank you very much - that's most helpful. I shall have to merge/test this when I get back home.

I wonder whether it might be worthwhile to have the credit percentages for insolvency and for credit limit customisable in simuconf.tab?

[neroden]

Thank you very much - that's most helpful. I shall have to merge/test this when I get back home.

I wonder whether it might be worthwhile to have the credit percentages for insolvency and for credit limit customisable in simuconf.tab?

I think it's worth considering.  I have implemented two separate computations for credit limit: one based on assets and one based on profits.  The player gets the better of the two.  I think that the customizable options should be:
(1) interest coverage level (expected interest cost as a percentage of expected profits) at which the player should not be allowed to borrow more
(2)  interest coverage level (expected interest cost as a percentage of expected profits) at which the player should be declared hopelessly insolvent and shut down

However, I am not sure how to figure the asset-based credit limit into this; it is a different thing conceptually.  I do not think it should be affected by the same configuration options.

[neroden]

OK, there are still some subtle but obnoxious bugs.  It looks a lot better but now it's generating some troublesome "fake revenue" from vehicles which are standing still. I'm going to work on it a bit more.

---
OK, I think I've got it working right now. 

This may also feature some performance improvement. 

I believe the bug I just fixed, which caused "duplicate unloading" of convois which were standing still, is present on the 11.x branch.  It would certainly be very performance-intensive, and more performance-intensive for convois which are waiting on schedules.   Less obviously, it causes revenue to arrive when it shouldn't (an undesired benefit to all players!)

I wish I could clean up the hat_behalten code, but (unlike in some other parts of the code  ) it's not at all clear to me how to do it cleanly and efficiently.

[jamespetts]

Thank you very much for this - I shall look forward to testing this when I get home! I wonder, now that you have completed this before we have got the 11.x branch working well, whether it might be worth making this branch the basis for the next release? It would also be interesting to see whether your duplicate unloading fix helps with the performance issues identified elsewhere.

On insolvency, as you might notice, I plan to change the handling of insolvency one day, and introduce a period of administration, during which other players can buy the defunct company as a going concern. How do you think that your changes to the credit limit fit in with this plan?

[neroden]

On insolvency, as you might notice, I plan to change the handling of insolvency one day, and introduce a period of administration, during which other players can buy the defunct company as a going concern. How do you think that your changes to the credit limit fit in with this plan?

I'm not entirely sure how it will fit with this plan.  I mostly wrote it as a fairly simple interim thing so that we'd have something reasonable for now, since the previous way it was handled was frustrating and difficult to play with -- and extremely complicated to implement with the new accounting system. 

What I have now should be easy enough to change when needed.

I think it is a good interim improvement.  Worth noting: as I have it currently implemented, "cost of capital" and "credit ratings" are effectively implemented in a very simple fashion, although the cost of capital is constant.  It would now be fine to start the player with a much smaller sum of money -- just enough to pay the construction costs for minimally profitable line.   The player's credit limit will now be increasing based on the player's profitability, you see.

My handling was designed to separate out two concepts:
(1) The banks won't lend you any more money. (Soft credit limit.)
(2) You're going bankrupt and should be shut down.  (Hard credit limit.)

If you implement "bonds", then you will replace the code for #1.
If you implement a "period of administration" for sale to other creditors, then you will replace the code triggered by #2.
If you want to change the conditions under which you go into administration, then you will replace the code for #1.

I think you'll still want two "credit limits" either way, and the code for the two parts will be different, so at least I've separated them out somewhat.

[neroden]

Thank you very much for this - I shall look forward to testing this when I get home! I wonder, now that you have completed this before we have got the 11.x branch working well, whether it might be worth making this branch the basis for the next release? It would also be interesting to see whether your duplicate unloading fix helps with the performance issues identified elsewhere.

Perhaps.  There are still various subtle bugs showing up, but I *think* they're shared with the 11.x branch.  I'm not 100% sure though.  I'll start another thread about the one I'm having the most trouble with.

[jamespetts]

Thank you - that is helpful. An interim solution makes sense, I suppose. Out of interest, and mainly so that I can understand how best to develop future cost of capital and insolvency related features, what did you find frustrating about the existing system?

[neroden]

Thank you - that is helpful. An interim solution makes sense, I suppose. Out of interest, and mainly so that I can understand how best to develop future cost of capital and insolvency related features, what did you find frustrating about the existing system?

What was frustrating was the time limit on total repayment.  If you had a loan out for more than three months, your credit limit would start shrinking, and if you left a loan outstanding for a year, you'd end up with zero credit limit.  It means that I had to make all my borrowings in one big chunk, then wait doing nothing for several years until the profits paid them back, then repeat.

Actual companies keep large amounts of debt "floating" and simply service the interest.  When a bond comes due, the bond is frequently "rolled over" by issuing another bond.  Everyone's OK with this as long as the company can service the interest.  The interest eats into the available profits, so it reduces what you can build without going further into debt, but there is no "you've had a loan out for a year -- now you must not borrow any more money".

[jamespetts]

Thank you very much for your work from this. I have recently returned home, and, after installing a new hard drive and some new memory, am able to get on with some coding work again. I have started by merging your 11.x fixes branch, which compiles well - I will check the performance presently.

However, I have had some trouble with the 112.x-private-car-merge branch: I merged your from-standard and your emergency-depot-fix branches, but am unable to compile. Initially, I get an error about MAX_PLAYER_HISTORY_MONTHS being undefined in the assert in line 281 in finance.cc; when I comment out that line, I get some curious linker errors:

Code Select Expand


1>------ Build started: Project: Simutrans-Experimental, Configuration: Debug Win32 ------
1>cl : Command line warning D9030: '/Gm' is incompatible with multiprocessing; ignoring /MP switch
1>  simdepot.cc
1>c:\users\james\documents\development\simutrans\simutrans-experimental-sources\dataobj/einstellungen.h(839): warning C4244: '=' : conversion from 'uint16' to 'uint8', possible loss of data
1>simdepot.cc(801): error C2572: 'depot_t::is_suitable_for' : redefinition of default parameter : parameter 2
1>          c:\users\james\documents\development\simutrans\simutrans-experimental-sources\gui\../simdepot.h(62) : see declaration of 'depot_t::is_suitable_for'
1>  finance.cc
1>c:\users\james\documents\development\simutrans\simutrans-experimental-sources\dataobj/einstellungen.h(839): warning C4244: '=' : conversion from 'uint16' to 'uint8', possible loss of data
1>  Generating Code...
========== Build: 0 succeeded, 1 failed, 0 up-to-date, 0 skipped ==========


Any assistance appreciated!

(Incidentally, may I ask for reference: do you use GCC, MSVC++ or something else for your work on Simutrans?)

[neroden]

Thank you very much for your work from this. I have recently returned home, and, after installing a new hard drive and some new memory, am able to get on with some coding work again. I have started by merging your 11.x fixes branch, which compiles well - I will check the performance presently.

However, I have had some trouble with the 112.x-private-car-merge branch: I merged your from-standard and your emergency-depot-fix branches, but am unable to compile. Initially, I get an error about MAX_PLAYER_HISTORY_MONTHS being undefined in the assert in line 281 in finance.cc;

That was a piece of brain fog on my part.  I actually typed MAX_HISTORY_PLAYER_MONTHS.  It has now been replaced with the correct MAX_PLAYER_HISTORY_MONTHS on the emergency-depot-fix branch.

Since GCC didn't complain, apparently I'm not compiling with assertions on.  Hmm.  Maybe I should change that.

Code Select Expand


1>simdepot.cc(801): error C2572: 'depot_t::is_suitable_for' : redefinition of default parameter : parameter 2
1>          c:\users\james\documents\development\simutrans\simutrans-experimental-sources\gui\../simdepot.h(62) : see declaration of 'depot_t::is_suitable_for'

Fixed this on the emergency-depot-bugfix branch; GCC is generous when you redefine the default parameter the same way as before.  Apparently MSVC++ is persnickety and requires that you only specify the default parameter once, which to be fair is what the official C++ rules require.

Any assistance appreciated!

Hope this helps. 

FWIW, since you tested this I finally fixed the "go to depot" bug where convoys got stuck in "replacing" mode.  I also fixed a funny little bug where you couldn't send a convoy to the depot if it was already standing on the depot.

(Incidentally, may I ask for reference: do you use GCC, MSVC++ or something else for your work on Simutrans?)

GCC 4.7 on Linux.  64-bit "x86_64" architecture.  I like GCC, but then I have actually contributed substantial patches to GCC.  The maintainers are on the whole very cool people. 

My debugger is GDB, which can be a bit primitive to use.  But it did allow me to track down the "stuck while replacing" bug to the go_to_depot routine, and furthermore to figure out that the convoys were trying to go to depots which did not exist, which finally allowed me to crack the bug.

[jamespetts]

Thank you very much for fixing that. Unfortunately, however, when I try to compile in MSVC++, I still get somewhat curious linker errors:

Code Select Expand

1>simplay.obj : error LNK2005: "public: bool __thiscall spieler_t::can_afford(__int64)const " (?can_afford@spieler_t@@QBE_N_J@Z) already defined in replace_frame.obj
1>simplay.obj : error LNK2005: "public: double __thiscall spieler_t::get_konto_als_double(void)const " (?get_konto_als_double@spieler_t@@QBENXZ) already defined in simintr.obj
1>.\simutrans\Simutrans-Experimental-debug.exe : fatal error LNK1169: one or more multiply defined symbols found
========== Build: 0 succeeded, 1 failed, 0 up-to-date, 0 skipped ==========

[jamespetts]

I have had another go at this: the linker errors seem to have been at my end: deleting all of the intermediate files and compiling afresh seems to have resolved this.

However, we seem to have saved game corruption on loading (the error being given as not being able to create a default line, but the corruption showing as early as line 801 of simplay.cc, with an absurdly large number being read as headquarter_level). That set of lines is very close to (and shortly after)

Code Select Expand

// save all the financial statistics
finance->rdwr( file );


which makes me think that the problem probably originates in the loading/saving of financial data. The problem is not confined to recent saves: the old demo.sve for Pak128.Britain-Ex is affected, as is Carl's latest UK map that he sent me a short while ago to test the performance.

[neroden]

I have had another go at this: the linker errors seem to have been at my end: deleting all of the intermediate files and compiling afresh seems to have resolved this.

However, we seem to have saved game corruption on loading (the error being given as not being able to create a default line, but the corruption showing as early as line 801 of simplay.cc, with an absurdly large number being read as headquarter_level). That set of lines is very close to (and shortly after)

Code Select Expand

// save all the financial statistics
finance->rdwr( file );


which makes me think that the problem probably originates in the loading/saving of financial data. The problem is not confined to recent saves: the old demo.sve for Pak128.Britain-Ex is affected, as is Carl's latest UK map that he sent me a short while ago to test the performance.

First thing to try is the problem you "solved" for me which was causing saved games to fail:

http://forum.simutrans.com/index.php?topic=11932.0

The old demo.sve for Pak128.Britain-ex had to be rescued the same way.

I believe Carl's map is probably suffering from the same problem.

Please test a save file from standard (I don't have any).  If those work, then I know the corruption is in the "experimental" end rather than the "standard" accounting end.  There's some very subtle stuff in the accounting data read and write -- mostly in ::rdwr_compatibility -- but I went through it all very carefully to tease the logic out for each version. 

What are the versions in the files which are not loading properly?

It could easily be completely screwed up by an error somewhere else, like the one you noted before where this had to be commented out in order to load properly:

Code Select Expand


if(file->get_experimental_version() >= 11 && file->get_version() >= 112003)
{
file->rdwr_long(max_axle_load);
file->rdwr_long(max_convoy_weight);
}

[jamespetts]

Thank you for the reply. The problem cannot be the same as you encountered on the linked thread, because the demo.sve game was saved in an older version of Experimental, and not a development build with a non-standard saved game format like that which you used to create the problem saved game that you reported in that thread. When you say that the old demo.sve had to be rescued in "the same way", what do you mean here - what was the method that you used to rescue it? The old demo.sve should be recognised as being in an older file format when starting and the code should not attempt to load from it values that were only saved in more recent versions.

As to loading saved games from Standard (at least some of the older ones that I have on my hard drive), two out of the three that I tried briefly a moment ago crashed with various errors: the third, being a saved game in Standard pak64 from 2011, loaded, although I did not check whether there were anomalies in some of the values (which I did find in loading an old Experimental saved game, which did not crash on loading, but crashed with a division by zero error shortly afterwards because the maximum midrange tolerance had been read as 0 when it ought to have had a proper number). The nature of the crashes are different in each case.

As to the versions, there seems to be no consistent pattern: most seem to fail. A saved game from Standard 0.102.3 fails, for example, as does a very old game from 0.99.10.  The problem does not seem specific to any particular version, whether Experimental or Standard.

[neroden]

Thank you for the reply. The problem cannot be the same as you encountered on the linked thread, because the demo.sve game was saved in an older version of Experimental, and not a development build with a non-standard saved game format like that which you used to create the problem saved game that you reported in that thread. When you say that the old demo.sve had to be rescued in "the same way", what do you mean here - what was the method that you used to rescue it?

I created an executable based off your 11.x branch at the time, but with the relevant code commented out.  I loaded the demo.sve using that and then I saved it again.  The newly saved version was readable with newly compiled versions.  (Perhaps you can figure out what state your 11.x branch was in at the time.)

So I'm pretty sure the old demo.sve for experimental was in a non-standard, mislabeled file format at that time.

The old demo.sve should be recognised as being in an older file format when starting and the code should not attempt to load from it values that were only saved in more recent versions.

As to loading saved games from Standard (at least some of the older ones that I have on my hard drive), two out of the three that I tried briefly a moment ago crashed with various errors: the third, being a saved game in Standard pak64 from 2011, loaded, although I did not check whether there were anomalies in some of the values (which I did find in loading an old Experimental saved game, which did not crash on loading, but crashed with a division by zero error shortly afterwards because the maximum midrange tolerance had been read as 0 when it ought to have had a proper number). The nature of the crashes are different in each case.

As to the versions, there seems to be no consistent pattern: most seem to fail. A saved game from Standard 0.102.3 fails, for example, as does a very old game from 0.99.10.  The problem does not seem specific to any particular version, whether Experimental or Standard.

God, I hate the load/save code.

I went through very carefully trying to replicate the save-game-version logic of the code from standard *and* the logic of the code from experimental at the point where I started the merger.  I really suspect that the problem is somewhere else, not in my work.

---
Or at least not in my load-save work.  I could have introduced a memory management error somewhere else.  Have you run Dr. Memory, valgrind, or something similar?

---
OK, more questions.  When did Experimental start recording "way tolls"?  I suspect the code on experimental prior to my merger was actually wrong.  Also when did Experimental start recording interest?

---
OK, I think I found one bug in my code.  This will probably allow recent experimental save files to load.  The other save file failures are, unfortunately, different bugs and have probably been around for a while...

---
Yep.  That was it. It now loads the old demo.sve file successfully.  (The new demo.sve file still fixes a whole bunch of problems, including a mess of "stop too short" errors.)

It will probably load Carl's game too.

I know I should have made a new branch for this... but it's on the emergency_depot_bugfix branch.

Also on that branch, a major cleanup of the walking time code; I fixed *several* bugs by creating inline methods in simworld.h to encapsulate the computations.  It should also be faster now.  The logic is the same apart from the bugfixes.

So try this version. 

It won't do much good for the game from 0.99.10 or the one from 0.102.3, where I have no idea what went wrong -- I'm pretty sure my load logic copies the logic in standard very closely. (Do either of those load with the current version of standard?)  With older versions of experimental, I'm even less certain of the game logic: there's some high weirdness involved.

[jamespetts]

Thank you very much for this - much appreciated. I am testing the latest code now. One question however:

Code Select Expand


else if(  file->get_version()<=110006 ) {
+      // Experimental version 11 with old save file format
+      // May happen in files saved with some development versions, but shouldn't
       // only save what is needed
       // Experimental had WAY_TOLLS, INTEREST, CREDIT_LIMIT
       for(int year = 0;  year<OLD_MAX_PLAYER_HISTORY_YEARS;  year++  ) {
          for(  int cost_type = 0;   cost_type<21;   cost_type++  ) {
             if(  cost_type<COST_NETWEALTH  ||  cost_type>COST_MARGIN  ) {
                file->rdwr_longlong(finance_history_year[year][cost_type]);
             }
          }
       }
       for (int month = 0;month<OLD_MAX_PLAYER_HISTORY_MONTHS;month++) {
          for (int cost_type = 0; cost_type<21; cost_type++) {
             if(  cost_type<COST_NETWEALTH  ||  cost_type>COST_MARGIN  ) {
                file->rdwr_longlong(finance_history_month[month][cost_type]);
             }
          }
       }


Your comment suggests that this code segment (within the "if" condition) is executed for Experimental files, yet there is no check for an Experimental file version (if file->get_experimental_version() >= X), so this code will execute whenever the Standard version number exceeds 110006 (that is, Standard version 0.110.6). Did you possibly confuse the Experimental RC numbering with old Standard numbering? The only part of the Experimental version that is saved is the major version number (11), although a combination of Experimental and Standard saved game numbering is used in some cases where the Standard version is incremented (certain values being loaded/saved only if the Experimental version is greater than X and the Standard version is greater than Y).

The saved game code is not ideal, I do agree: it would be good if there was far more automation and abstraction. However, we are rather suck with the Standard inheritance on this one, I fear.

Incidentally, do you still need answers to the questions about way tolls? I have run Dr. Memory on Experimental recently, and it has come up only with a few very small memory leaks, mainly in Standard based code (but also in some UI code) amounting in total to a handful of kB.



Edit: Testing this, I found a compile error in Windows (at least), which I have fixed on my branch. Also, I notice some anomalies in the accounting window: the year/month numbers apart from the current year/month are not displayed (is this intentional? If so, it is a mistake, I think, as it makes the graph much harder to read), and the "margin" is denominated in the wrong units (it registers in the thousands when the percentage gives it in the tens: a margin of -34.76% is registered as -3,476). Is the code intended, do you think, not to separate maglev and narrow gauge out from trains? There does not seem to be a separate category for these things in the combination box. The history for "transported" appears not to have been preserved. We probably need separate (but similar) colours for the credit and solvency limit graphs.

Also, away from the finance window, the demo.sve game appears no longer to load: the default empty one town island is generated instead.

[neroden]

Thank you very much for this - much appreciated. I am testing the latest code now. One question however:

Code Select Expand


else if(  file->get_version()<=110006 ) {
+      // Experimental version 11 with old save file format
+      // May happen in files saved with some development versions, but shouldn't
       // only save what is needed
       // Experimental had WAY_TOLLS, INTEREST, CREDIT_LIMIT
       for(int year = 0;  year<OLD_MAX_PLAYER_HISTORY_YEARS;  year++  ) {
          for(  int cost_type = 0;   cost_type<21;   cost_type++  ) {
             if(  cost_type<COST_NETWEALTH  ||  cost_type>COST_MARGIN  ) {
                file->rdwr_longlong(finance_history_year[year][cost_type]);
             }
          }
       }
       for (int month = 0;month<OLD_MAX_PLAYER_HISTORY_MONTHS;month++) {
          for (int cost_type = 0; cost_type<21; cost_type++) {
             if(  cost_type<COST_NETWEALTH  ||  cost_type>COST_MARGIN  ) {
                file->rdwr_longlong(finance_history_month[month][cost_type]);
             }
          }
       }


Your comment suggests that this code segment (within the "if" condition) is executed for Experimental files, yet there is no check for an Experimental file version (if file->get_experimental_version() >= X),

Read more carefully.  It's something you need to learn to do when programming.

The game logic works like this:

Code Select Expand


...
else if(  file->get_version()<=110006  && file->get_experimental_version()==0  ) {
// standard
...
}
else if(  file->get_version()<=110006) {
// Experimental
}
....


[neroden]

Thank you very much for this - much appreciated. I am testing the latest code now.

You don't seem to have the latest code.  Try downloading again.  I have been uploading quite frequently.  (And I have been irresponsibly putting it all on the emergency-depot-bugfix branch.)

Also, I notice some anomalies in the accounting window: the year/month numbers apart from the current year/month are not displayed (is this intentional? If so, it is a mistake, I think, as it makes the graph much harder to read),

That's an actual bug.  I haven't figured out exactly how that part of the code works -- it was *extensively* rewritten in Standard.  Are the numbers showing up in Standard?  :-)  I haven't checked that.

and the "margin" is denominated in the wrong units (it registers in the thousands when the percentage gives it in the tens: a margin of -34.76% is registered as -3,476).

Another actual bug.  It would be good to see how Standard is handling this.

Is the code intended, do you think, not to separate maglev and narrow gauge out from trains? There does not seem to be a separate category for these things in the combination box.

Please note that only categories which actually have some activity will show up; so that my 1750s game only shows "Boat", "Road", and "Rail" options.  Once the other categories are active in your game they should show up in the drop-down list.  This is how it works in Standard now.

The history for "transported" appears not to have been preserved.

Grrr.  I think it's preserved but apparently it's not translated properly between formats.  This is an annoying bug.

We probably need separate (but similar) colours for the credit and solvency limit graphs.

Sure; I couldn't decide what colors to use.

Also, away from the finance window, the demo.sve game appears no longer to load: the default empty one town island is generated instead.

That's most likely a  bug which I found and fixed on my emergency-depot-bugfix branch yesterday (d688452933).  The search path for the demo.sve game was wrong: it will look for it in the main program directory, *not* in the pakfile directory.  That bug actually seems to be present in standard, and I'd suggest cherry-picking the fix and backporting it to standard.

[neroden]

Please, if you possibly could, report back as to how many (if any) of the display bugs are:
(1) present in standard as of github commit 394f55a372e62943bcda8eb6cba2508762b7b38d, which is the version I merged.  If so, it's not my fault.  :-) Some may have been fixed in standard after that point.
(2) still present on the emergency-depot-bugfix branch.  I've been through several rounds of fixes there.  I recently managed to actually remove a use of system floating point (if you call "pow", it's going to try to use floating point if you aren't very very careful).

[jamespetts]

Thank you - now have the latest code.

Have just checked - both the margin and the "transported" (Standard calls it "trips") bug appear to be in the most recent released version* of Standard, too. It does, however, correctly display past years (but not every year any longer - in an old saved game, I get 1885, 1883, 1881, 1879, 1877, 1875, 1873, 1871, 1869, 1867, 1865, 1863, 1861 (note that the graphs go backwards in Standard by default); the bars are still one per year). The finance related bugs from my last report are also present in the latest from your emergency depot bugfix branch.

Sadly, the demo.sve problem persists for some reason.

* I have not set myself up to compile Standard at present. However, the changes on Standard's Github mirror since the release can be seen here. The only fixes relevant to the finance window appear to be these:

https://github.com/aburch/simutrans/commit/bc1fcb6b47d56e4a5ea4e761e880db0cfe002e60
https://github.com/aburch/simutrans/commit/04acef0b6455ab09423070e20cc37807b934bc43
https://github.com/aburch/simutrans/commit/b1cb8aa80f03b975526de3592c8a787fad4983b3

None of these appear to address the margin denomination nor the history of trips/travelled.

Edit: I have now posted bug reports in Standard for the margin and trips issues.

[neroden]

* Please report the problems with year printing in Standard.  The problems we're having are very likely to have exactly the same cause as whatever is making half the year names go away in Standard, or at least to be closely related.

* I straightened out my branch names.  Currently under development: ncn-devel, merge-from-standard (based on ncn-devel), and fix-loading-screen (based on merge-from-standard).

* You unfortunately introduced a save/load bug on your branch.  This is now fixed on the ncn-devel branch.  This was causing crashes for me, but it might have had a different result for you.  I am seeing demo.sve loaded.  The copy being loaded is located in the pak directory.  Its name is all lowercase.

I'm wondering if this is a Windows/Linux filename convention difference, which would stink, because we'd have to code conditional compilation for it.

* I have also fixed another small and obnoxious bug related to factory coverage on the ncn-devel branch.

* I have started in on merging from standard again, one patch at a time (on the merge-from-standard branch).  While I was doing so I ran across some code from Standard from January which you failed to merge properly.  Importantly, this code impinges on simgraph16.cc and simgraph0.cc -- this may be the cause of the server crashes.

This is fixed on the "fix-loading-screen" branch, which also contains everything contained in the ncn-devel branch and the merge-from-standard branch.

[neroden]

It does, however, correctly display past years (but not every year any longer - in an old saved game, I get 1885, 1883, 1881, 1879, 1877, 1875, 1873, 1871, 1869, 1867, 1865, 1863, 1861

Try setting the graphs to go backwards and see what happens.

(note that the graphs go backwards in Standard by default)

Yep.  The graphs work backwards and not forwards.  This is a bug in standard.
---
I am pushing a proposed fix to this onto the graph-cleanup branch (which is a branch off of ncn-devel, so it doesn't have all the other fixes).  I don't have time to properly test it tonight; I'll try to get back to it.

[jamespetts]

Thank you for all of this help. Let me reply to the individual points in turn:

* I don't think that the year numbering in Standard is defective: the number of years' history recorded has increased in the latest versions, and giving every year individually would make the display of years too crowded. In any event, this now seems to be fixed in your loading screen fix branch. The backwards graphs thing in Standard is, I think, deliberate, and there is (now) an option to turn it off. I have no idea what the original reasoning behind this was, but it has been like that since the dawn of Simutrans. Experimental has had the graphs being the right way around on by default for a long time.

* Oops - I see my error from your fix. Thank you for that.

* I am afraid that I am still having the demo loading problem in an unchanged configuration from 11.x where the demo loads without difficulties. A Windows/Linux difference here would indeed be an unhappy one; but it is not clear to me why one should have arisen recently (or, more mysteriously, why it should work on Linux and not on Windows rather than the other way around, which rather rules out the usual suspect of case sensitivity).

* Thank you for the factory coverage fix - most appreciated.

* As to the continued merge from Standard, might I suggest that you put anything after and including the new landscapes into a separate branch, as I should prefer not to include that in the next release so as not to add too much at once, not least because we need to make design decisions about how to integrate this with physics (how much extra drag should a half/full height hill add? Should these be customisable in simuconf.tab?) and recompile paksets to work with this. Thank you for doing this, however.

* I have noticed that there seems to have been an unforunate side effect to the unit standardisation, in that the fix that Bernd made some time ago to allow the times in seconds to be smooth and count down each individual second in the convoys' remaining time displays and the time display (when set to the in-game time display), has been lost such that the displays have reverted to their previous behaviour of counting in chunks, which is somewhat unsatisfying and makes things difficult for users who want to be more exact with timings. Would you be able to restore this functionality?

* The margin issue is, I am told, working as intended on Standard: see here.

* The transport history thing has been fixed on Standard, and I have incorporated that fix on my 112.x-private-car-merge branch.

* I have yet to test with the server or find a new colour for the solvency limit: I will have a go at those presently.

[neroden]

* I don't think that the year numbering in Standard is defective: the number of years' history recorded has increased in the latest versions, and giving every year individually would make the display of years too crowded. In any event, this now seems to be fixed in your loading screen fix branch.

My fix on that branch needs to be applied to standard too.  It is attached -- please pass it on to the standard developers.

The backwards graphs thing in Standard is, I think, deliberate, and there is (now) an option to turn it off.

Yes: but if you turn it off in standard, you'll find that the graph labels have disappeared.  Until you apply my patch.   

* As to the continued merge from Standard, might I suggest that you put anything after and including the new landscapes into a separate branch,

I'm happy to.  However, I actually stopped merging earlier, before the "departure board" merger.  That is going to be a huge bear to merge -- it has to function completely differently in experimental than in standard.  There are a number of bug fixes, to background redraw and to accounting, which come after that in standard, unfortunately.

The main reason why I was pushing the merge forward was the accounting fixes... unfortunately there are several accounting errors persisting, of the "maintenance is wrong while playing and corrects itself on reload" variety.    I may try to cherrypick the accounting fixes. 

We talked about a rework of the maintenance code to be more object-oriented and work "more automatically" a few weeks back, and I think that that would eliminate all these problems in the long run. 

I have two branches running right now: ncn-devel, and "merge-from-standard".  "Merge-from-standard" has everything in ncn-devel, plus additional merging and the loading-screen-fix material (which might fix your server problem).  Ncn-devel just has the bugfixes I've been doing myself, with a somewhat earlier last merge point from standard. 

If merge-from-standard doesn't work please try ncn-devel.

I'm going to start another branch before merging the departure board code.

* I have noticed that there seems to have been an unfortunate side effect to the unit standardisation, in that the fix that Bernd made some time ago to allow the times in seconds to be smooth and count down each individual second in the convoys' remaining time displays and the time display (when set to the in-game time display), has been lost such that the displays have reverted to their previous behaviour of counting in chunks, which is somewhat unsatisfying and makes things difficult for users who want to be more exact with timings. Would you be able to restore this functionality?

I noticed this too.  The answer is a little complicated.

(1) It took me quite a while to find this, but it's fixed on the ncn-devel branch now.

(2) The thing about this is that most times are internally stored in tenths of minutes.  What would really be great would be if you changed the entire codebase to store all the waiting and journey times in seconds.      Could you do that sometime?      That would allow me to clean out a huge amount of gunk from the code.  Unfortunately it requires converting every old departures table on load, which is yucky.

* The margin issue is, I am told, working as intended on Standard: see here.

This link doesn't seem to be correct...

* The transport history thing has been fixed on Standard, and I have incorporated that fix on my 112.x-private-car-merge branch.

Yay!

* Thank you for the factory coverage fix - most appreciated.

There's a second factory coverage fix on the ncn-devel branch; I had missed something subtle, a bug which you introduced a while back while first putting in factory coverage, and this is now fixed.  There may be further problems with factory coverage.

There's also a flurry of fixes related to checking the credit limit, some of which fix things which have been wrong for a long time in experimental.  (See, I told you there were a lot of bugs.)

And I fixed a crash bug related to accounting for vehicle maintenance.

I also ran across a bug in code by Isodoro from 2010 (!!!) related to waiting times for trips to deleted stops, and fixed that.

* I am afraid that I am still having the demo loading problem in an unchanged configuration from 11.x where the demo loads without difficulties. A Windows/Linux difference here would indeed be an unhappy one; but it is not clear to me why one should have arisen recently (or, more mysteriously, why it should work on Linux and not on Windows rather than the other way around, which rather rules out the usual suspect of case sensitivity).

Well, here's the way to debug it. 

First, I just added another patch to the ncn-devel branch, commit 030a392f1a815 -- this one shouldn't actually do anything.  However, it eliminates some potential buffer overflows *and* it provides better debugging; compile and run with debugging level at least 3 and it should tell you where it's looking for the demo file.

Second thing to try.  Make a test branch, not your main branch.
First revert my commit 030a392f1a815
Then revert this commit of mine from June 9: d6884529333c98eb9 (reverting this will break demo.sve for me)
Compile and test this.
If you suddenly start loading the demo.sve file again, then it's a Windows/Linux directory layout difference.  And reverting the patch will break loading of demo.sve for me.

----
Finally, new bugs I've discovered.

If I build elevated roads within a city, they get taken over instantly by the city government.  Which is nice because they cost nothing to maintain, but not really appropriate...

I've also discovered what appears to be a couple of major bugs in mail handling. 
First, the mail type is missing entirely from the goods list!

Second, my mail revenue is always at the minimum --  basically, zero -- no matter how much I deliver.  (And this is on single vehicles, mail trucks, which means it isn't logic I changed.)  This could be related to the first error, or perhaps there's an underflow / roundoff error?
I haven't been able to figure out where to start looking for this.

[neroden]

I successfully solved the demo.sve bug on my ncn-devel branch.  This is a funny one -- the pakfile name may be either relative or absolute.  BOTH have to be handled correctly.

This is also a bug in standard; the pakfile name can be absolute there too.  The patch is attached; I'm not sure if it will apply cleanly to standard, but it should more or less.  Please forward it on.

[TurfIt]

Under what conditions are you seeing umgebung_t::objfilename as an absolute path?
The required directory structure has paks always below umgebung_t::program_dir, and hence the pak path should always be with the program dir as root. More places in the code than just the demo.sve loading concatenate these two strings.

[neroden]

Under what conditions are you seeing umgebung_t::objfilename as an absolute path?

Command line specification is one, but it's also popped up in savefile records.

(Edit: sort of spontaneously,... I couldn't tell you under what circumstances.)

The required directory structure has paks always below umgebung_t::program_dir, and hence the pak path should always be with the program dir as root. More places in the code than just the demo.sve loading concatenate these two strings.

Then they all need to be fixed.  You *cannot depend* on having relative path names and a specified directory structure.  *Ever*.

[TurfIt]

Almost every program I can think of that uses data files has required locations where they must be put. Simutrans is no different.
But, the -objects argument, and pak_file_path config entry are subject to user error by entering an absolute path and ending up with a semi functioning game. A simple check of the concatenated path closes that...

[neroden]

Almost every program I can think of that uses data files has required locations where they must be put. Simutrans is no different.

Sort of.  Almost every program that I can think of allows data files to be located whereever you want, provided you specify pathnames carefully enough in config files.  Only the config file has an absolute required pathname.  Have you been working on an inferior operating system or something?   

You cannot depend on having relative pathnames.  Ever.  Do not do that.    Why?  Because there is no reliable way to recover a relative pathname from an absolute pathname. 

You can if necessary depend on absolute pathnames, though that isn't advisable either.

But, the -objects argument, and pak_file_path config entry are subject to user error by entering an absolute path and ending up with a semi functioning game. A simple check of the concatenated path closes that...

This does not fix the problem.

Absolute pathnames for paks have popped up under other circumstances.  They seem to be saved into savefiles in some way, sometimes.  They are very easy to generate because you need to use them for most purposes.  Unless absolute pathnames are accepted, it breaks savefiles which have absolute pathnames for paks encoded in them, and that has happened repeatedly.

Demanding relative pathnames also qualifies as stupid.  You cannot force anyone to put their files where you want them to, since a user can work around it with sufficiently many "../" entries in a pathname.

Bluntly, unless you want these problems to recur, you have to change the codebase to accept absolute pathnames.

[jamespetts]

I should add that I suspect that having full pathnames embedded in saved game files, when it occasionally occurs, is a bug, although I have never been able to track it down.

[neroden]

I should add that I suspect that having full pathnames embedded in saved game files, when it occasionally occurs, is a bug, although I have never been able to track it down.

I'm sure it is a bug.  It's the sort of bug where even if we fix it, it is likely to recur, though.

[jamespetts]

That's an unfortunate sort of bug. Why do you think that it has that propensity?

[neroden]

Because it's really hard to *prevent* absolute paths.  Absolute paths are *natural* according to the way most operating systems were designed, including Windows and Linux.

It's easy to prevent *relative* paths.  Relative paths are *unnatural*.

If you are of a mind to do so, it's easy to prevent paths entirely, demanding only raw filenames (by rejecting anything with a slash or backslash in it).

But it's hard to prevent people from generating absolute paths and pushing them at you.  And the core system utilities will happily return absolute paths almost all the time.  Converting them to relative paths is impossible.

[jamespetts]

Hmm - what is the best way of dealing with filename handling in the light of that propensity?