Simutrans-Experimental 10.0 - pre-release testing

[Milko]

Hello

I have another problem loading Pak96comic 0.4.9 using Exp10.0.

Using 9.12 there are no problem.

Giuseppe

[jamespetts]

Carl,

what were the conditions in which that occurred? Can you upload a saved game?

Giuseppe,

the problem appears to be with the saved game demo.sve - can you delete demo.sve in the pakset folder and try it again?

[Milko]

Hello

Deleting demo.sve all is ok.

But I have a similar problem using Pak German Exp - 20382.

demo.sve all seem to create loading problems with version 10.0

Giuseppe

[Carl]

what were the conditions in which that occurred? Can you upload a saved game?

No special conditions -- the crash simply occurred while I was running the following (9.x) game in the background, checking the travelling times fix. It took perhaps a quarter or half a month from loading for the crash to occur.

Savegame: http://dl.dropbox.com/u/61716/UKJuly.sve -- it's a big one, i'm afraid
I think this is the right addons folder: http://dl.dropbox.com/u/61716/carladdons2.rar

Many thanks!

[jamespetts]

Hmm, I can't reproduce this with the debug build, I'm afraid. Can you re-check with the SYNC_VECTOR option disabled?

[Carl]

Hmm -- I can't reproduce the crash, either. Very strange! I'll let you know if this comes up again.

[Dwachs]

-SYNC-VECTOR is a new preprocessor definition added to Standard ...

you should have written

-DSYNC-VECTOR

in the config.* file

[Carl]

Oh dear -- it seems I may have jumped the gun a little in thinking that the journey-times bug was fixed.

As far as I can work out, the situation is as follows: the latest release version is can read point-to-point average speeds from the hashtable (which is why I thought the problem was fixed on the previous test maps), but it cannot write them.

Here's a new map, created in the latest version, in which point-to-point journey times haven't kicked in. I guess you'll be able to verify in debug whether the hashtable is empty or not: http://dl.dropbox.com/u/61716/journeytimes3.5.sve

Further evidence comes from the previous test map, journeytimes3.2. Although the point-to-point journey times are correctly displayed upon loading, they never change from month to month. Normally I'd expect to see random variation of 6/12 seconds here and there, but there is none. This suggests that the release version is only reading, and not writing, point-to-point average speeds from the hashtable.

[jamespetts]

Carl,

thank you very much for this report, and apologies most sincere for not having reverted to you: I have been especially busy at work this week and have had very little time for Simutrans, unfortunately.

I have managed to do some testing this evening, however, and have found that the problem manifests in the id_pair representing the pair of two halthandle ids used as the hashtable key (the x being the destination and the y the origin): on loading the debug version, all of the y values in the pairs are zero. There are no id_pairs with y-values of anything other than zero. This is corrected when the debug version is run, and a full complement of id_pair values are generated.

Testing further, and using testing code to trigger the built-in error messages in place of the debugger in the release build, it appears that no pairs are put in to the hashtables with a y-value of zero. However, clearly, they are coming out with a value of zero. This is odd. I have not had time to test further yet, but I should be grateful if anyone has any ideas on this issue in the interim.

[sdog]

Just a thought on load times: At a trains route during normal stops only a part of the pax unloads, at terminal stops, all unload. Perhaps it would be sensible to double (or more) load times on terminal stops to reflect this?

[Milko]

Hello

I was trying the pak96c, I noticed that cities are created (while leaving the default settings and no timeline) are all large and with few inhabitants. All buildings generated are small, have not been generated, high-density buildings.
If I use the standard version instead of the buildings are of varying density and smaller cities are created equal in population.
pak96c has not been developed for experimental but it seems to me that in any case "experimental" has a rather strange behavior as he does not consider the medium and high density buildings.

Giuseppe

[jamespetts]

Sdog,

this should be accounted for by the fact that the new system measures the actual number of passengers (etc.) loading/unloading.

Giuseppe,

when using a non-Experimental pakset, you should increase the "rennovation_percentage" in cityrules.tab to prevent this.

Edit: The problem reported by Carl is especially tricky and bizarre. See the latest testing code in simconvoi.cc in the 10.x branch - the second of the fatal error calls is triggered in release but not debug builds. The id_pair values are inserted into the hash table, and, on insertion, the numbers are correct. In debug builds, they are retrieved from the hashtable correctly. However, in release builds, all of the y (but not the x) values are immediately read as zero whenever they are removed from the hashtable. This is rather beyond me, I am afraid: does anyone have any idea(s) as to what this might be?

[sdog]

oh, didn't notice that new feature, should have read the thread more thoroughly, instead of only browsing it.

is the load time specified in the dat file the maximum load time if all pax would alight and a full load get picked up?

[infernalmachine]

Hi.  I spent a while looking at the code, and I noticed a possible reason for the problem.

But first I should say I've never seen simutrans code before, I haven't coded in 15 years, and I never quite "got" templates in c++.  So if this is a silly suggestion, I'll understand   

My first guess was that y is being bit-shifted into zeroness, either accidentally when you retrieve it, or possibly when you create the hash number.

In the tpl directory your template for koordhashtable_tpl has the hash function:

Code Select Expand

static uint32 hash(const key_t key)
    {
uint32 hash = key.y << 16;
hash |= key.x;
return hash;
    }

Now, assuming that key.x and key.y are both uint16, it struck me that key.y might need to be typecast as uint32 before being bitshifted, or else risk becoming (uint16) 0 before you store it in hash and then or in key.x.  If that happened then hash is just equal to (uint32) key.x

So you'd only be dealing with the first 64K possible entries in the hash table for both lookup and updating.  And you wouldn't get junk back on lookup, because you have to hash your present and past location ids before you can access what's stored in the table. The expected matrix would effectively be just a vector.

Don't know how a debugger could hide that, though.... Unless it casts everything under 32 bits as 32-bit?

Anyway, as I say, I'm very rusty on when typecasting is needed, so this may be a silly suggestion and that code may be working as intended.

ps Does every line have its own hashtable? And the expected max size is the square of the number of stops on the line?

[isidoro]

I don't think there is a problem with that.  Automatic integer promotion should take place.  Please see, for instance:
https://www.securecoding.cert.org/confluence/display/seccode/INT02-C.+Understand+integer+conversion+rules

[infernalmachine]

Okay.  Thanks for that.  You convinced me.  So key.y is turned into sint32 before the bit shift. 

I guess I should try to find out where key.x and key.y values are retrieved from the the whole key.  The link you gave does warn about potential problems when going from unsigned to signed ints.

Beyond that, I'm stumped.

[jamespetts]

Infernal Machine,

thank you very much for your suggestion and looking into the code in this way - that is very kind. Thank you also Isidoro for your feedback. I am wondering whether the problem is that I am putting objects of the class "id_pair" into a koordhashtable, which is designed for objects of the class "koord". "Koord" objects have two sint16 members, whereas id_pair objects have two uint16 members. I notice that there is discussion above about difficulties when converting between signed and unsigned integer types in this context. Might that be an issue, do people think? If so - how would a hashtable class for pairs of unsigned integers work differently to one for signed integers?

Edit: This does not appear to be the problem - when I substitute koord objects for the id_pair objects, the exact same problem still occurs. This is rather bizarre.

[jamespetts]

After much testing, I think that I might have fixed this latest issue with the average journey times (although the cause is still somewhat opaque to me) - can people re-test?

[Carl]

It seems that the bug is fixed!

Since I jumped the gun in saying it was fixed last time, this time I started an entirely new test map using the latest version. After leaving it to run for a few months the stops display the correct journey times -- times that could not have been given by the fallback system.

That said, I can't get the correct times to display on the 'journeytimes3.5' map from my previous post. However, that might just be because it was saved in the previous (defective) version. The fact that this bug doesn't occur in newly-created maps suggests that it's fixed.

Many thanks!

[jamespetts]

Thank you very much for testing - much appreciated!

[Milko]

Hello James

The problems of "SYNC_VECTOR" decreased but still I believe we need to revise some things, I am attaching the report of MingW.

EDIT
When compiling: almost all files. "cc" to report the same message about "SYNC_VECTOR", not just those that show the image that I attached it.

Giuseppe

[jamespetts]

Giuseppe,

thank you for reporting this: the problem was that I had not realised that SYNC_VECTOR was already defined in the code, so the definition in the makefile produced the duplication warning. I have now removed these redefinitions, and it should now compile without these warnings.

[Milko]

Hello

Now I have this warning, in many ".cc" files. Were not there before.

Giuseppe

[jamespetts]

Don't worry about that warning - the code from Standard is full of signed/unsigned integer comparison warnings.

[Milko]

Hello

Last "nightly"...

http://simutrans-germany.com/files/upload/simutrans-experimental_1709.zip

Giuseppe

[jamespetts]

10.0 has now been released - I should like to thank very much all those who have helped with testing it.

[Carl]

Many thanks for your hard work too, James -- 10.0 is a truly excellent release!

[jamespetts]

Glad that you like it!

[Milko]

Hello

Don't worry about that warning - the code from Standard is full of signed/unsigned integer comparison warnings.

You can eliminate this warning? 

I agree that the warnings signs are weak and potentially harmful.
The problem is that if it causes so many warning signals may create confusion.
Among the many warning it is difficult to understand if some of them are important or not.
I had noticed the problem due to the fact "SYNC_VECTOR" just based on the number of warnings at compilation.

Giuseppe