Simutrans-Experimental 10.0 - pre-release testing

[Carl]

Yes, I get almost identical times to yours in debug.

[jamespetts]

Very, very odd. I'm not sure that I can quite work out how to track an error that is present in release but not debug builds. Any ideas anyone (Inkelyad, for example)?

[jamespetts]

Hmm - one thing that you could try is saving a game in the debug version after you have given it enough time to accumulate the timing values than loading it in a release build and seeing whether the values are maintained, or whether it eventually reverts to the fallback?

Edit: Incidentally, I have now been able to reproduce this issue. The trouble is, I can't debug the release version. This is a bizarre and difficult problem indeed.

[Carl]

Hmm - one thing that you could try is saving a game in the debug version after you have given it enough time to accumulate the timing values than loading it in a release build and seeing whether the values are maintained, or whether it eventually reverts to the fallback?

On initial load of the debug game in the release version, the correct timing values are overwritten -- with one exception. The time from Frankenthal branch to St Johann branch is retained at 8:36 mins -- the broadly correct value. This appears to be the only retained value. Even the reverse journey (St Johann to Frankenthal branch) has reverted to the fallback level of 12:30 mins.

After a path refresh, even this value reverts back to the fallback mechanism.

This is indeed bizarre -- what are the principal differences between the debug and release version that could case this kind of problem?

[jamespetts]

The only significant differences are: (1) compiler optimisations; (2) nothing in "#ifdef DEBUG... #endif" blocks are compiled, and (3) asserts are not called. However, (2) and (3) can be ruled out, as there is no relevant code in #ifdef DEBUG blocks, and I have not used any assertions. Compiler optimisations can sometimes cause code with undefined behaviour that works by chance in a non-optimised build to stop working, but I don't think that I've written anything with undefined behaviour, either, and I'm not sure how to go about testing whether I have.

Edit: Your test, incidentally, narrows down the problem to the new code in path_explorer.cc, as that is the only relevant code called at the very start of a game.

[jamespetts]

Update: I have made a very minor change, but not had chance to test it yet. There is a small chance that this might fix the problem - I should be grateful if this could be tested.

[Carl]

The central issue is, I'm afraid, not fixed in this version. The Debug version still gives the correct times while the Release version does not.

However, there is one significant change: a Debug-savegame loaded in the Release version *does* display the correct times upon loading. But the incorrect fallback times return after a path refresh.

[Carl]

Here's a potential lead: a problem in the debug version.
http://dl.dropbox.com/u/61716/journeytimes3.2.sve

This game is saved in the latest debug version.

At Warendorf External station we're told that both of its destinations will take 6:48 mins to travel to despite the fact that these destinations are a couple of kms apart on a linear line. This anomaly has remained despite several path refreshes. (Note that the other line on this map seems to be functioning normally.)

[Milko]

Hello

Very, very odd. I'm not sure that I can quite work out how to track an error that is present in release but not debug builds. Any ideas anyone (Inkelyad, for example)?

It can be an uninitialized variable?

Giuseppe

[inkelyad]

It can be an uninitialized variable?

Uninitialized elements in container or unexpected temporary variable, i think.
I suppose I can run Simutrans through Valgrind.

Edit:
Result for debug version:

Code Select Expand


==27940== Conditional jump or move depends on uninitialised value(s)
==27940==    at 0x8289110: vehikel_besch_t::set_scale(unsigned short) (vehikel_besch.h:647)
==27940==    by 0x82755C8: karte_t::set_scale() (simworld.cc:1822)
==27940==    by 0x8275019: karte_t::karte_t() (simworld.cc:1788)
==27940==    by 0x823B4CE: simu_main(int, char**) (simmain.cc:981)
==27940==    by 0x82C0EF8: main (simsys_s.cc:682)
==27940==
==27940== Conditional jump or move depends on uninitialised value(s)
==27940==    at 0x8289147: vehikel_besch_t::set_scale(unsigned short) (vehikel_besch.h:651)
==27940==    by 0x82755C8: karte_t::set_scale() (simworld.cc:1822)
==27940==    by 0x8275019: karte_t::karte_t() (simworld.cc:1788)
==27940==    by 0x823B4CE: simu_main(int, char**) (simmain.cc:981)
==27940==    by 0x82C0EF8: main (simsys_s.cc:682)
==27940==

max_loading_time_seconds and min_loading_time_seconds are used before initialisation.

Edit2:
One more suspicious place:

Code Select Expand


simhalt.cc: In member function 'uint16 haltestelle_t::get_average_waiting_time(halthandle_t, uint8) const':
simhalt.cc:1174: warning: taking address of temporary


[jamespetts]

Inkelyad,

uninitialised variables are unlikely to be the cause of this trouble, since they are usually equally problematic in debug builds as release builds. The first location shows that there are cases (namely, when a non-Experimental pakset is read) where the minimum and maximum loading time values are not initialised; I have now fixed that on the 10.x branch, but this will not affect the issue in question.

The second issue identified, line 1174 of simhalt.cc is not something that can affect this issue, since it is related to the code for waiting times, not journey times, and is in any event not an error:

Code Select Expand


if(&waiting_times[category].get(halt.get_id()) != NULL)


The address of the temporary is taken only to check whether it is NULL or not and is not further used, so no problems should result.

Carl's leads are helpful, and I shall look into those further. Thank you both for your helpful input on this!

[inkelyad]

Here's a potential lead: a problem in the debug version.
http://dl.dropbox.com/u/61716/journeytimes3.2.sve

At Warendorf External station we're told that both of its destinations will take 6:48 mins to travel to despite the fact that these destinations are a couple of kms apart on a linear line. This anomaly has remained despite several path refreshes. (Note that the other line on this map seems to be functioning normally.)

I can't reproduce this. My version (debug and release, compiled on Debian 6.0.2 32 bit) give me 6:30 and 6:48 mins.

[Carl]

That's what I get upon loading in the release version. After a path refresh, though, it reverts to 2:30 and 5:36.

[inkelyad]

The address of the temporary is taken only to check whether it is NULL or not and is not further used, so no problems should result.

It is temporary variable. it was created while expression was computed. Its address can't be NULL.

I think it should be written as

Code Select Expand


if (waiting_times[category].is_contained(halt.get_id())


[inkelyad]

That's what I get upon loading in the release version. After a path refresh, though, it reverts to 2:30 and 5:36.

In my version there is no such revert. Tricky bug.

[prissi]

Uninitialized variables are initialized to a certain value in debug builds (pointer for instance to 0x7dddddddd in MSVC); to find out that those are actually unitialized. In release build they could rather contain random stuff.

[jamespetts]

I have found an issue using Carl's latest test game, and pushed a fix for it; can people re-test? I am not sure whether this solves the original problem (it is possible), but it certainly solves a problem.

[Carl]

Thanks James. Your suspicions were correct, as this version fixes that one oddity but doesn't seem to impact upon the main issue.

(By the way, I'm not sure that the "loading time range" fix has had the desired effect yet. At a distance_per_tile of 40, trains seem to always take 1:06 mins to load, regardless of the amount of people joining/leaving the service.)

[jamespetts]

Carl,

thank you for testing - I shall have to look into the (rather difficult) principal issue further. As to loading times; do the loading times in practice correspond to the loading times displayed in the depot?

[Carl]

As to loading times; do the loading times in practice correspond to the loading times displayed in the depot?

Yes, they do -- and having looked in the depot I've found the root of this issue, and it's not so much of a concern as I had thought:

-- All non-Experimental vehicles display the 0:20--1:00 range that you're looking for.

-- Vehicles I've built using the Experimental MakeObj -- but which don't have a loading time range specified in the .dat yet -- aren't subject to this default. Rather, most display a range of 01:10--01:10 (or occasionally 0:35-0:35). Note that many of these have *no* loading time specified in their .dat files.

---
Edit:
I've just tested this out with Standard vehicles. While vehicles are loading for the correct times, what's displayed on-screen is not quite right. See the following save: http://dl.dropbox.com/u/61716/loadingtimes.sve

The vehicle here displays a funny mix of loading times. Sometimes it will say "Loading 17:00", but it will leave after one minute of this time has passed. Similarly, sometimes it will say "Loading 3:30", but leave long before it reaches zero. I take it it's the displayed times that are mistaken here. This is a little difficult to explain -- I hope that the save is self-explanatory!

[jamespetts]

Carl,

thank you for that. The difference between vehicles built with Standard and Experimetnal makeobjs is not a bug: it has to work that way to maintain compatibility with older Experimental vehicles built using the original "loading_time" specification. Standard vehicles are not subject to such a constraint.

The second part, however, is a bug, and I shall have to look into that when I get a chance. Thank you for your report.

[Milko]

Hello

I have problems opening the files. rar, I can have the addon of Carl in zip format?

Giuseppe

[Carl]

Here it is: http://dl.dropbox.com/u/61716/carladdons4.zip

[jamespetts]

Carl,

I think that I've fixed the loading time error - can you re-test? The other issue is rather more tricky, and will need furhter investigation.

[Milko]

Hello

I'm testing http://dl.dropbox.com/u/61716/journeytimes3.2.sve

I have built sim-exp using all update to the code 31/08

I'm looking the first stop of the line 1 (Hoyersweda outer station). In init.png you may look travel time to second (Hoyersweda branch station) stop and to third stop (Hoyersweda external station), times appear ok.

On game loading the train is between third and fourth stop.

When the train reach first stop of line 1 the travel time is not updated and rimain 9:36, it's correct...

When the train reach second stop of the line travel time is updated, but is not correct (look second.png, travel time 17:18, the double)

It appear that when a train reach the first stop of a line the travel time is not applied to the correct point of the line and not resetted.

If you look the second stop when the train reach the first stop the travel time from second to one don't change.

May be the same for the last stop of the line?

Giuseppe

[jamespetts]

Some diagnostic progress in relation to the main problem: on loading a game saved in the release build in a debug build, it is apparent that the average journey time records are all empty, which is why the fallback mechanism is used. The average objects are present in the hashtable at the correct locations, but the values are zero, as if they had been incorrectly reset. Loading in debug will slowly restore the averages, but loading, running, and saving again in release will clear them again. I wonder whether this is a problem associated with resetting the averages after being read, especially as the minor change that I made to the code that had some effect was relating to that part of the code. I shall investigate further.

Giuseppe - are you using the debug or release build?

[jamespetts]

I have found a possible cause of this problem, and have pushed a fix - can people re-test?

[Milko]

Hello

Giuseppe - are you using the debug or release build?

I use MingW and the procedure I described in one of my previous post ... Sorry but I do not know if this is a debug version or release.  

EDIT:
I enclose the report on the compilation using MingW; is reported something about "-SYNC_VECTOR".
I do not understand if something important ....

Giuseppe

[jamespetts]

Giuseppe,

-SYNC-VECTOR is a new preprocessor definition added to Standard, which, when enabled, changes the sync steppable objects collection from an slist to a vector, which, according to the commit comment, can increase speed by up to 5%, and is also network safe. I am not sure why MinGW is reporting that it is not recognised - perhaps because it is not present in those particular files?

[Carl]

I have found a possible cause of this problem, and have pushed a fix - can people re-test?

James,

Initial testing suggests that the bug has indeed been fixed in the Release version!  

The lines in the test maps are now displaying journey times which are (a) commensurate with point-to-point times rather than fallback times, and (b) are in line with how long travel takes according to the GUI clock.

Many thanks!

[jamespetts]

Aha - excellent!

[Milko]

-SYNC-VECTOR is a new preprocessor definition added to Standard, which, when enabled, changes the sync steppable objects collection from an slist to a vector, which, according to the commit comment, can increase speed by up to 5%, and is also network safe. I am not sure why MinGW is reporting that it is not recognised - perhaps because it is not present in those particular files?

EDIT2:
I'll try to see if the problem also exists in the standard version. If the problem is shared I will insert a message in the forum of the standard version.
"SYNC_VECTOR" does not produce reports compiling the standard version ...

EDIT:
Enclosed you will find sim-exp with the latest bug fixes by James
http://simutrans-germany.com/files/upload/simutrans-experimental_0109.zip

Giuseppe

[wlindley]

I have done a 'git pull' and am on commit a52bf63b71f56c7143017deaa0ff07bc6429aa21 ... 64-bit linux.  Using Pak128.Britain-Ex-0.8 ... everything worked up until yesterday's build and now I get:

Code Select Expand

$ ./simutrans-experimental
Use work dir /home/bill/simutrans/sim-exp/
Reading low level config data ...
FATAL ERROR: loadsave_t::rdwr_str()
expected "<i16>", got "</ei>"

FATAL ERROR: loadsave_t::rdwr_str()
expected "<i16>", got "</ei>"
Aborted (core dumped)

[jamespetts]

WLindley,

you will need to delete settings-experimental.xml (or settings-experimental-debug.xml if you are using a debug build).

[Carl]

I've just observed another vector crash which I assume only affects larger maps:


vector_tpl<T>::[]
class koord3d: index out
of bounds: 13 not in 0..-1