Quickstone issues discussion

[jamespetts]

Further to the discussions here and here, I should be grateful for any assistance on what appears to be an issue with the way in which Experimental handles quickstones.

(I sent Dwachs a PM on the topic, and he requested that I continue the discussion in the forum, so a brief recap from that exchange: I asked whether there is a problem if hashtables contain halt IDs and temporary halthandle_t objects are created assigned to those IDs to access the underlying halts. Dwachs pointed to this code:

Code Select Expand


if(file->get_experimental_version() >= 10 || file->get_experimental_version() == 0)
{
self = halthandle_t(this, halt_id);
}
else
{
self = halthandle_t(this);
}


as being potentially problematic, in that the "else" branch will create a new ID no matter what the previous halt id was).

To answer that question firstly, the reason that I included the "else" branch was that, in earlier versions of the saved game, the halt id would not be loaded from/saved to the file, so "halt_id" here would be zero. The "else" block is never touched when loading a newer saved game, of the sort which gives the trouble.

The errors that I get when loading in 10.7 or earlier a saved game, which, when loaded in 10.8 or later, will desync when played in network mode, is the "slot (number) already taken" error. The problem of assigning a null pointer to a non-null index reported by Sdog seems rarer, although I suspect that it stems from the same overall problem.

[Dwachs]

You are right, that part of the code alone seems to look ok. Another idea: in ware_t::rdwr there is a possibility that something goes wrong with halt ids.

The code you posted above of saving station data restores the halt id from savegame under the conditions

Code Select Expand


if(file->get_version() > 110005) {
...
if(file->get_experimental_version() >= 10 || file->get_experimental_version() == 0)
{
self = halthandle_t(this, halt_id);
}
...

But the code in simware.cc assumes that halt_ids are preserved if file->get_version() > 110005 regardless of file->get_experimental_version(). So there is the possibility that halt_ids of ware_t packets point to the wrong stations if file->get_experimental_version() < 10.

A second idea would be to take such a crashing savegame and log all calls to create a halthandle (3? places in simhalt.cc).

[jamespetts]

Dwachs,

thank you very much indeed for that - the first issue was a bug, but not the one causing this problem (since all of the files would have a higher version than 110005 ex 10 in any event). I have now fixed that, however.

The second suggestion seemed to yield results: logging calls to the quickstone constructor that took a single pointer and no ID, I found that these were caused by calls to welt->get_halt_koord_index, which I was using in place of welt->lookup(pos)->get_halt(). The former method created a new halt, with a new halthandle, when pos did not return a path to a valid halt.

That, in turn, lead me to find that last_stop_pos was often not the position of a valid halt, as it should be, so I added additional code to check for this condition and set it correctly when last_stop_pos is set in the first place. Hopefully, the combination of these things will have the desired effect. I have pushed the changes to the 10.x branch.

I am staying with my parents this week-end, so do not have time for furhter testing at present, but if anyone could test the 10.x branch, I should be most grateful.