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

[Phystam]

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

Code Select Expand


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.

[Mariculous]

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.

[Mariculous]

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.

If 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]

Almost inifinte loop xD

[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.

[Mariculous]

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.

[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.