News:

Simutrans.com Portal
Our Simutrans site. You can find everything about Simutrans from here.

[CODE] incorrect for statements in private car routing save data

Started by Phystam, February 18, 2020, 06:48:06 AM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

Phystam

There is an error in simcity.cc:2385 and :2405,

for (uint32 i = 0; i++; i < 2)
{
  uint32 private_car_routes_count = private_car_routes[i].get_count();
  file->rdwr_long(private_car_routes_count);
...

Please fix it, but probably this correction causes a incompatibility of saved data...

freddyhayward

I had a look at this and fixed it in my travel-time-congestion branch. It did indeed break all saves since the private car routes update, but I fixed this by running the old code on saves from that version.

Sirius

What does this code actually do?
Run until the uint32 becomes false aka 0, thus when it overflows after 2^32 iterations??
And then? Is attempts to access each index, where each index above 2 is out of range?
I would expect this to 1. grow the save quite a lot and 2. cause unexpected stuff in the code, where an immediate segfault would even be the best case, so I am wondering this didn't cause such huge issues yet and I guess I am missing something.

Phystam

Actually, this route count was not saved. When checking the loop condition first, the i is set as 0(false), since i++ is calculated after checking. If it were ++i, the increment would be calculated before checking, and it would cause an infinite loop.

freddyhayward

Phystam is right: stepping through on gdb, the for loop condition would fail on the first time, every time. what it was meant to do was save both tables, the processed and non-processed routes.

Sirius

Ah sure, obviously it's 0 at start xD

So fixing the code and using it only in later versions should work and would be the cleanest.
Effectively, we don't save references to private car routes stored all arond the world, thus we might want to perform a cleanup, resetting all private routes around the whole world when loading a game from that specific version.
That might also explain strange behavior in some places on stephenson-siemens where private cars were driving very strange routes.

Quote from: Phystam on February 21, 2020, 09:33:09 AMIf it were ++i, the increment would be calculated before checking, and it would cause an infinite loop.
Actually not true. We would get false at overflow (where we likely won't arrive due to segfaults)

Phystam


freddyhayward

Updated, since your suggestion is far more sensible than what I did. However from testing in debug mode, the problem with routing seems to be that in the effort to save performance, the routes are updated very conservatively -- effectively not at all apart from when two towns are linked by road for the first time. Remember the chorborough bypass that no one was using except in heuristic mode? I built a road tunnel linking All Petingstone and Peache, which effectively linked All Petingstone to all other mainland towns for the first time. After waiting and waiting, all the new routes *from* All Petingstone to the other towns went via the chorborough bypass, however none of the existing congested routes through chorborough were modified, and routes *towards* All Petingstone were never even created.

Sirius

Sounds buggy. From my understanding routes should be frequently recalculated, no matter any situations.

jamespetts

I should note that this code affects city storage of private car routes only, which I am currently working on abolishing in order to deal with the excess memory consumption issue.
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

The abolition of city storage of private car routes has now been deployed in the master branch and is in the latest nightly build. I believe therefore, that this bug is no longer relevant.
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.