News:

Simutrans Wiki Manual
The official on-line manual for Simutrans. Read and contribute.

Bugs on -devel branch

Started by Carl, April 06, 2012, 09:38:00 PM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

wlindley

With same version as exhibits the zero-passenger buildings:  Vehicles continue to accelerate forever, leading to this:

Markohs

I just found about a c++ stetic code analyzation tool http://sourceforge.net/apps/mediawiki/cppcheck/index.php?title=Main_Page

I'll have it a look and pass over the experimental code and see if I find something relevant

HeinBloed

Thanks for helping out Markohs. :)

I had meant to post some more information about this already, which I forgot. I've actually tracked down the exact commit since when the bug was introduced, it's this one: https://github.com/jamespetts/simutrans-experimental/commit/99ec6a773849768326d843653de18a00a181507c

It definitely doesn't occur on the devel branch in the previous commit, nor on the braking-physics branch in the previous commit, and seemingly in every devel branch commit since then. Unfortunately, my C(++) skills are still far too modest to make an attempt to find something in the code there or else I would have had a look myself already.
(previously known as "tttron")

Markohs

it's still processing, takes lots of CPU...

I'm processing standard and last experimental, to compare results, so far only a few hard errors showed up (about warings, he has tons already)

Top of the image is simutrans STANDARD, the one down, is EXPERIMENTAL.


Markohs

#109
Okay, I got the results. Generated 2 outputs for each project, experimental and standard. One is a report of what cppcheck considers as pure errors, one of them has already been solved today in svn by Dwachs, even I think none of the "bugs" were critical.

Experimental throws a few extra bugs than standard, the relevant ones are:

- Possible null pointer dereference: w - otherwise it is redundant to check if w is null at line 4063, line 4077 vehicle\simvehikel.cc

having a look at the code, seems correct to me, no bug here to my eyes, since thge check_access checks if it's null before refering it. *BUT* this is one of the files modified in the problematic commit, so this *might* be the bug.

- Memory leak: password line="503" file="nettools\nettool.cc"

This is indeed a bug but it's even documentated in the code, and it's not really important.

- Null pointer dereference line="67" dataobj\livery_scheme.cc

I had it a look, and I don't really know if that's a bug or not, but I think it isn't.

The rest of errors are present already in standard and are nto really a severe problem.

I'm sorry I coudn't help more, I'll have it another look this weekend if I got time. :)

http://dl.dropbox.com/u/30024783/ccpcheck/experimental.xml
http://dl.dropbox.com/u/30024783/ccpcheck/experimental-full.xml
http://dl.dropbox.com/u/30024783/ccpcheck/standard.xml
http://dl.dropbox.com/u/30024783/ccpcheck/standard-full.xml


The full report is too verbose to be useful, but the bug you describe might be in that report, hidden somewere. ;)

EDIT: Posting here fast what in a fast overlook of the report I think it *CAN* corrupt memory

-<error verbose="%s in format string (no. 1) requires a char* given in the argument list" msg="%s in format string (no. 1) requires a char* given in the argument list" severity="warning" id="invalidPrintfArgType_s"> <location line="63" file="besch\writer\root_writer.cc"/> </error>

<error verbose="%s in format string (no. 1) requires a char* given in the argument list" msg="%s in format string (no. 1) requires a char* given in the argument list" severity="warning" id="invalidPrintfArgType_s"> <location line="101" file="besch\writer\root_writer.cc"/>

Edit2: According to Hein, we can narrow the search to this files:
   
besch/reader/vehicle_reader.cc
besch/vehikel_besch.h
besch/writer/vehicle_writer.cc
dataobj/translator.cc
gui/components/gui_convoy_assembler.h
gui/convoi_filter_frame.h
gui/halt_list_filter_frame.h
simconvoi.cc
simconvoi.h
simhalt.cc
vehicle/simvehikel.cc

jk271

I am trying to compile devel branch of simutrans experimental on Linux. Compilation is ok, but linkage not:

===> LD  build/default/simutrans-experimental
build/default/path_explorer.o: In function `path_explorer_t::compartment_t::step()':
/home/pokus/src/simutrans-experimental/path_explorer.cc:790: undefined reference to `convoi_t::get_average_journey_times()'
/home/pokus/src/simutrans-experimental/path_explorer.cc:794: undefined reference to `convoi_t::get_average_journey_times()'
/home/pokus/src/simutrans-experimental/path_explorer.cc:799: undefined reference to `convoi_t::get_average_journey_times()'
/home/pokus/src/simutrans-experimental/path_explorer.cc:892: undefined reference to `convoi_t::get_average_journey_times()'
/home/pokus/src/simutrans-experimental/path_explorer.cc:1017: undefined reference to `convoi_t::get_average_journey_times()'
collect2: ld returned 1 exit status
make: *** [build/default/simutrans-experimental] Error 1


Probably reintroduced bug from this thread:
http://forum.simutrans.com/index.php?topic=9706.0

Following patch fixes problem  (removes inline keyword causing linkage not to work):

diff --git a/simconvoi.cc b/simconvoi.cc
index 89a7071..5453982 100644
--- a/simconvoi.cc
+++ b/simconvoi.cc
@@ -6138,7 +6138,7 @@ void convoi_t::emergency_go_to_depot()
        }
}

-inline koordhashtable_tpl<id_pair, average_tpl<uint16> > * const convoi_t::get_average_journey_times()
+koordhashtable_tpl<id_pair, average_tpl<uint16> > * const convoi_t::get_average_journey_times()
{
        if(line.is_bound())
        {

jamespetts

Thank you everyone for all the work on this, and sorry that I have not hitherto been able to respond more fully: I have been rather busy this week with work. I have made the change suggested by JK to fix the compile error - thank you for that. Thank you also to Hein for finding in exactly which commit the error occurs. The error first occurring in a merge commit makes it very awkward to track down, however: I cannot imagine what I altered on the -devel branch to make the braking-physics branch code go wrong when merged with it. It does seem very peculiar indeed.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

HeinBloed

@Markohs

How did you compile the list of files which might contain the (apparent) physics bug? Based on the diff with the previous commit on the devel branch ("Merge branch 'fare-stages' into devel") and then narrowing it down to files that are at least remotely related to physics code?

Going through the list, I remembered two more aspects of the bug which shouldn't be forgotten and which might indeed point to one of those files. It's the two things I've written about here and here. Since the presence and content of seemingly arbitrary files in the Simutrans folder structure is apparently having an influence on the bug, it could be one of the vehicle loading routines that's broken. Maybe the fact it doesn't happen on Windows Vista/7 or XP with debug libraries could be explained by subtly different I/O routines ...? And as for my other post, those screenshots look like the braking force (and possibly other stats) could be wrong for the vehicles in question, which could be connected to the I/O too.

What makes it so awkward to track this down is that it only happens on XP (which requires James to use a VM) and that it doesn't happen with the debug libraries, so how can you really debug it ...  :-[
(previously known as "tttron")

jamespetts

I have had a thought about the static/dynamic issue of this: Hein, you compiled a statically linked version on your XP machine, and found the same (bad) results as using dynamic linking on your XP machine. From that, we concluded that static linking was not a solution.

However, it has since struck me that that might not be right. When you statically linked on your XP machine, you were probably statically linking in the same versions of the libraries as those to which you were linking dynamically, producing the same errors. The problem may well not persist if you run on your XP machine an executable statically linked on Windows 7. To test this, I have produced such an executable, which can be downloaded here: I should be grateful if you could give it a go and let me know how you get on.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

HeinBloed

Thanks for the effort James. I tested this immediately, but I'm sorry to say that it doesn't fix the bug:



(that's one of the cases where the speed display is jumping around wildly, but that doesn't really matter as another type of tram is going infinite speed in the same game and a third type is crawling)
(previously known as "tttron")

jamespetts

Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

HeinBloed

#116
I just noticed that the bug we've been talking about before here isn't fully gone yet either. I've uploaded a savegame here with which you should be able to reproduce the crash on deleting a station within walking distance of another station. You should delete "München Victorian School Stop" for that. What might be peculiar about this one is that I had previously redesigned "München Brownfriars Railway Station" so it (involuntarily, I still keep forgetting about that sometimes) ended up in walking distance, which it wasn't at first.
(previously known as "tttron")

jamespetts

#117
Hein,

thank you for your work. Bernd has been working on some fixes this week-end, which I have just pushed to -devel: one on a so far unreported bug in loading a certain version of Standard saved games, and an uninitialised variable issue in the vehicle reader code that might well have caused the reported issues with teleporting (etc.). I have just pushed the changes - would you mind testing to see whether they help?

I shall now turn to seeing if I can fix the re-report of the walking issue.

Edit: I think that I have fixed the crash - I should be grateful if you could re-test.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

HeinBloed

Buggy trams not fixed, I'm afraid. Does Bernd have access to an XP (virtual) machine to test? Otherwise, it could be very laborious for him to deal with this ...  :-[ By the way, releasing a "workaround" build just for XP which is linked to the debug libraries wouldn't be an option either, I think. I read somewhere on a Microsoft site that one is not allowed to ship the debug libraries with a program. Whether that applies to a statically linked build too, I don't know. But seeing the performance detriments, which are not very bad though, I guess this would never be an option anyway ... I've only used this as a resort to play on my running game a little.

Oh, and if you're reading this Bernd - there's also still bug No. 6 present in the latest code.

The station removal crash (as displayed in my savegame) is gone though, thank you. :)
(previously known as "tttron")

jamespetts

Bernd tells me that he doesn't have access to an XP machine for testing, but I'm not sure whether he's able to set up a virtual machine. At least one bug is gone!
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

Bernd Gabriel

Hein,

I didn't try to fix bug 6 yet.

And I don't have access to Win XP or Win 7 Professional.

But if it is correct, that both branches were okay before the merge, but the merge result isn't, then I'm sure, that I will find some more fishy differences.
The journey is the reward!

HeinBloed

I just found another bug ... (or at least I think it is one, and it doesn't happen with 10.11 either)

It can be observed in the savegame I've already uploaded before. All the trains are slowing down a little at free signals, but only on diagonal tiles, which means they don't reach full speed there if the signal density is sufficiently high (4 in my case). However, it doesn't happen exlusively on diagonal tiles but also on straight tiles in apparently rare cases. If you follow a train going from Buxtehude to Döbeln, you'll notice that it slows down two or three times on the straight section behind Mühlacker somewhere near a signal and when going downhill.

I'll also renew my assertion that travelling times on circular lines with alternate directions turned on are still buggy. Granted this is based on a savegame carried over from 10.11 (the same as linked above), but if you check any of the Berlin inner city stations, the travelling times to all the stops on the line have gone up a huge lot and seem to have approached the old buggy time of a trip to the most distant stop again. Which means in particular that it reportedly takes way longer than an hour to reach an adjacent station only 1-2km away ...

And while there is a little variation of the pax travelling times still, the mail travelling times are all uniformly on an interval of about 5 minutes around 1:35h-1:40h.

I just don't have the time at the moment to start a new game from scratch with the devel executable and continue it until reaching such a developed stage, only to verify that it happens in that situation too. I must admit that the whole process is also getting a little frustrating because I seem to be the only one right now and the number of open bugs isn't really going down.  :-[
(previously known as "tttron")

jamespetts

Thank you for the reports. The slowing down for signals issue might be related to the current bug preventing the slowing down for corners working properly, which Bernd is looking into, but I shall see if I can look into that, too, as it might not be related.

The issue with the reverse circular routes I shall look at again. You say that the journey times seem to increase over time; can you elaborate a little on the behaviour, and in particular what you consider the start point to be (i.e., when they start increasing, and what they are at the beginning)? Knowing that might materially assist in finding the bug.

Sorry that you're feeling frustrated - part of the issue is that many of the bugs are physics related, which I cannot sensibly investigate, as I do not know how the physics works in detail. Bernd, who wrote the code, is investigating, but his free time is limited, so, whilst we are waiting for that to be fixed, I am spending my Simutrans time on pakset work and on fixing any non-physics bugs that might be reported.

Your work to track down bugs is very much appreciated, however!
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

Bernd Gabriel

Most likely the slowing down before signals is related to the physics.

I assume that, the convoys must start braking, because the signal is not yet green.

I didn't check the block reservation code, but I think there must be an extension in experimental, that considers the braking distance.

The physics code gives a 10% safety difference, thus it starts braking 1100m before the end position, if the braking distance is 1000m. If the block reservation does not calculate the same way, the convoy has to slow down to a braking distance of 909m.

BTW: While trying to fix the slope issue, I'm watching another curious braking at the bottom of a 2 tile slope to almost 0 km/h.
The journey is the reward!

HeinBloed

Thanks to both of you for looking into this. I must admit that I may be a little impatient at the moment due to some stress in life. But I always appreciate the work you put into Experimental.  :)

@James

That's the problem with circular line issue, it seems to be so difficult to pinpoint where things start going awry unless one would invest a lot of time to monitor it ...  :-[ However, since mail is apparently completely unfazed by the changes you've implemented so far (cf. my savegame, both Berlin and Hamburg), could that give a clue maybe? Insofar as a problem with mail only carries over to pax too and screws those times in the long run too?
(previously known as "tttron")

Bernd Gabriel

#125
A convoy slows down before a signal, if the pre-calculated route ends at the signal. To avoid braking, precalcultion of the next partial route should be done before braking is required. James, if you will give it a try, please notice that convoys start braking 10% before the minimum braking distance to avoid abrupt stops due to too roughly rounded calculations.

A reason, why this can be observed mostly on diagonal routes, could be, that maybe the route calculation thinks in tiles only, but should consider the shorter diagonals?


Actually slope "drag" is considered at the end of a tile. And due to the "Cumulative drag for hills" the friction lags another 1-2 tiles. James, what was the intention of the "Cumulative drag for hills"?

I'm trying to move slope consideration to the beginning of the tile...
I moved the slope consideration to the beginning of the tile and temporarily switched back to "Simple drag for hills".
The journey is the reward!

jamespetts

Bernd,

thank you very much for looking into this. I was not aware of the 10% thing, so the method for checking when signals need to determine whether they are to clear is not based on the 10% figure, which I expect is what is causing the problem. Shall I fix that, or would you like to do it?

The additional slope drag was added to make hill climbing more realistic - previously, hills did not have anything like enough of an effect on acceleration/speed to give players a realistic incentive to avoid them, and this seemed to be the most effective way of dealing with that. Specifically, I wanted to simulate different steepnesses of hill, which is not possible directly in Simutrans, by assuming that multiple hill tiles in succession means a steeper hill, which is as good an approximation as one can get, I think. If you are able to move the calculation of the drag to the beginning of the tile, that would very much help one of the reported issues.

Hein,

when you write that mail is unfazed by the changes, do you mean the reverse circular route bug still exists in its original form for mail?
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

Bernd Gabriel

James,

could you please add the 10% lookahead to the block reservation?

Thanks, also for clarification about "Cumulative drag for hills".


The journey is the reward!

jamespetts

I have had a go with the 10%: I think that I have got this right. Can people test to make sure? Thank you all very much!
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

HeinBloed

Slowing down at signals is not fixed ...

As for the circular routes, can you think of a reason why a game carried over from 10.11 could show a different behaviour than a newly started game? Are there any variables which could preserve wrong travelling times forever? I'm asking because I just can't reproduce it in a new devel branch toy game, but as I said, I can't start a full new game at the moment.
(previously known as "tttron")

Carl

Testing the latest version, I can also reproduce the "unnecessary braking for signals on diagonal track" bug. It's odd that the behaviour on diagonal track should be different from on straight track.

Bernd Gabriel

#131
If a route for a convoy is pre-calculated only up to the next waypoint, the convoy brakes to stop at the waypoint until the next partial route is pre-calculated.

That's why my test-bus stops at the bottom of a slope  ::(

I'm going to see, if I can avoid braking before signals ...
I could avoid braking before signals ...

Now waggon_t::ist_weg_frei() uses the same convoy_t::calc_min_braking_distance() method that convoi_t::calc_acceleration() uses. It includes the 10% safety margin.
And waggon_t::ist_weg_frei() considers diagonal tile distances correctly now by using the route_infos convoi_t::calc_acceleration() does as well.

My test-bus of course still stops at waypoints, as the above fix is in waggon_t  ::(
I'm going to see, how I can help the bus ...
The journey is the reward!

jamespetts

Bernd,

thank you very much for your work - that is helpful. Shall give it a test when I get a chance. In the meantime, may I ask why you have disabled the cumulative drag for hills?
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

Bernd Gabriel

I switched it off for testing. I found it easier to test with immediate drag effect.
Feel free to remove the added line and activate it again.
The journey is the reward!

jamespetts

Bernd,

thank you very much for your work on this. I have now integrated this into -devel (and reverted the disabling of cumulative drag for hills). Can people test whether this fixes it?
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

jamespetts

As to the circular routes, the difficulty is that it is extremely difficult to isolate any particular cause in a game with a large number of different routes, as it is almost impossible to identify when the code is stopped at a breakpoint whether any particular route is being used, which is why I always test with one route games. Have you checked whether amending the schedules will cause the timings to revert to a sensible level?
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

Carl

There doesn't seem to be any more unnecessary braking on diagonals in the latest version. Thanks Bernd!

jamespetts

Thank you for testing, Carl - most helpful!
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

HeinBloed

@James

I'll give amending of schedules a try later today.
(previously known as "tttron")

jamespetts

#139
Hein,

as to the weird XP bug with the physics - you mentioned that the result was affected by whether you had a cityrules.txt file in the directory, is that right? Can you confirm whether this remains the case, and whether it works correctly on an entirely clean install?

Edit: Can you also re-test the circular route thing with the latest version of -devel? I am wondering whether the issue that afflicted docks might have had some relevance here.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.