News:

SimuTranslator
Make Simutrans speak your language.

[Code v3.7] Problems in convoi_t

Started by knightly, May 12, 2009, 06:33:00 PM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

knightly

James,

For this part of code from convoi_t :



bool convoi_t::set_schedule(schedule_t * f)
{
enum states old_state = state;
state = INITIAL; // because during a sync-step we might be called twice ...

DBG_DEBUG("convoi_t::set_fahrplan()", "new=%p, old=%p", f, fpl);

if(f == NULL)
{
return false;
}

#ifdef NEW_PATHING
if(!line.is_bound() && old_state != INITIAL)
{
// New method - recalculate as necessary
halthandle_t tmp_halt;
ITERATE_PTR(fpl, j)
{
tmp_halt = haltestelle_t::get_halt(welt, fpl->eintrag[j].pos, besitzer_p);
if(!tmp_halt.is_bound())
{
// Try a public player halt
spieler_t* sp = welt->get_spieler(0);
tmp_halt = haltestelle_t::get_halt(welt, fpl->eintrag[j].pos, sp);
}
if(tmp_halt.is_bound())
{
for(uint8 i = 0; i < anz_vehikel; i ++)
{
const uint8 catg_index = fahr[i]->get_fracht_typ()->get_catg_index();
tmp_halt->reschedule[catg_index] = true;
tmp_halt->force_paths_stale(catg_index);
}
}
}

ITERATE_PTR(f, k)
{
tmp_halt = haltestelle_t::get_halt(welt, fpl->eintrag[k].pos, besitzer_p);
if(!tmp_halt.is_bound())
{
// Try a public player halt
spieler_t* sp = welt->get_spieler(0);
tmp_halt = haltestelle_t::get_halt(welt, fpl->eintrag[k].pos, sp);
}
if(tmp_halt.is_bound())
{
for(uint8 i = 0; i < anz_vehikel; i ++)
{
const uint8 catg_index = fahr[i]->get_fracht_typ()->get_catg_index();
tmp_halt->reschedule[catg_index] = true;
tmp_halt->force_paths_stale(catg_index);
}
}
}
}
#endif
...



(1)
The 1st problem is, if you try to run a convoy using schedule (not line) between 3 halts A, B & C, and after a while, remove B from its schedule, you will find that pax at B are still waiting for the convoy to go to A or C, and more people keep adding to the halt. If you check the reachable connexions, A and C are not removed from the list. Such condition persists for a few months. You will surely wonder -- in your code, you have already looped through all entries in both old and new schedules, notifying all halts to reschedule.

However, what you don't expect is, fpl == f ! If you take at look at the following code from sync_step() :



bool convoi_t::sync_step(long delta_t)
{
// still have to wait before next action?
wait_lock -= delta_t;
if(wait_lock <= 0) {
wait_lock = 0;
}
else {
return true;
}

switch(state) {
case INITIAL:
// jemand muß start aufrufen, damit der convoi von INITIAL
// nach ROUTING_1 geht, das kann nicht automatisch gehen

//someone must call start, so that convoi from INITIAL
//to ROUTING_1 goes, which cannot go automatically (Google)
break;

case FAHRPLANEINGABE:
// schedule window closed?
if(fpl!=NULL  &&  fpl->ist_abgeschlossen()) {

set_schedule(fpl);
...



You can see that the last line in the quoted code, that fpl is passed as argument to set_schedule(). Thus, you are actually working on 2 equivalent schedules.


(2)
The 2nd problem is, you only perform force_paths_stale(c) on the involved halts (i.e. added halts, deleted halts and halts that remain untouched). I think it is a very good attempt to restrict connexion and path reconstruction by notifying only the involved halts. However, connexions and paths are different in nature.

Connexions are local in nature, because they only record directly reachable halts from a single halt only. If the schedule of a registered line has one halt added or removed, only the involved halts need to be notified for update.

But for paths, they are global in nature, in that they are affected by any transport-related changes anywhere on the map. For instance, assuming there are 10 halts named from A to J, and among the various transport lines, one serves only A, B, C, D & E. Later, if that line adds F to its schedule, basically all halts have to recalculate their paths, since there is no easy way to isolate the effect of such change. You cannot selectively modify only those paths that can benefit from such change, because you don't know which path will benefit from the change until you perform path search again from scratch. If you don't notify G, H, I & J to recalcuate their paths, they will not be able to take advantage of the change.

So, the proper way is to notify involved halts to rebuild connexions, but notify all halts on the map to rebuild paths. Of course, both can be restricted to the ware type entailed, as you have been doing right now.


3)
Finally, a small suggestion. I can see that you loop through all vehicles of the convoy to update the halts, for 2 times. I just think, maybe it's worthwhile to pre-calculate a mini-list of supported ware types, so that you can reuse it from time to time. Similar thing has already been done for lines. That would help when there are more longer convoys.


BTW, will you expand this system to incorporate lines as well?

jamespetts

Knightly,

thank you for all this - more very useful analysis (and hard work no doubt!). May I ask for clarification on that last question - to which system in particular are you referring?
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.

knightly

Oh, I think I should not use the word system :P

What I mean is your approach to limit updates to relevant halts instead of calling welt->set_schedule_counter() to trigger a holistic update. Since I can see that the latter approach is still used in simline.cc and simlinemgmt.cc, I am wondering if you will extend your approach to cover lines as well.