News:

SimuTranslator
Make Simutrans speak your language.

[Code v3.8] stadt_t::passagiere()

Started by knightly, May 14, 2009, 04:47:44 PM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

knightly

James, during my search for the "unexpected mail bug", I have thoroughly reviewed the code of stadt_t::step_passagiere() in your implementation, and here are the issues I have identified, in sequential order of the code :

(1)
The first thing I noticed is that, city cars are also generated if mails cannot find a route. I think this is not reasonable, as mails cannot drive a car on their own, and in reality few car owners will drive a car to a remote destination just to deliver a single piece of mail. I suppose this is a bug, instead of what you have expected. Mails should simply be lost when there is no route and convoy. So probably, the following IF condition  :


if(start_halts.get_count() > 0 || has_private_car)
{
...


should be changed to (start_halts.get_count() > 0 || (has_private_car && wtyp==warenbauer_t::passagiere))
Obviously there are other parts of code that need to be changed, so you may need to look around for them. As a rule of thumb, wherever you check for if(has_private_car), you need to change it to if(has_private_car && wtyp==warenbauer_t::passagiere).


(2)
The DESTINATION_CITYCARS code block below should be removed, as at that point it is not yet decided whether the pax packet will take a city car or not. Placing such code here will add city cars for as many times as (number of failed routes + 1), which is neither reasonable nor necessary and creates more cars around than it should.


while(!route_good && !can_walk_ziel && current_destination < destination_count)
{


#ifdef DESTINATION_CITYCARS
//citycars with destination
if(has_private_car)
{
if(start_halts.get_count() > 0)
{
halthandle_t start_halt = start_halts[0];
if(start_halt.is_bound())
{
erzeuge_verkehrsteilnehmer(start_halt->get_basis_pos(), step_count, destinations[current_destination].location);
}
}
}
#endif



(3)
In the following code, if it is determined that there is no suitable halt for the current destination, you call current_halt->add_pax_no_route(pax_left_to_do). The problem is, if you add pax to no_route before it is determined there is no route for all alternative destinations, the same pax packet will be counted more than once in the same or different start halts. It is even possible that pax is added to both happy and no_route of the same start halt, if unreachable destinations are processed before a reachable one. If this is what you intended, then of course it is not a problem, but you have to understand that double counting will exaggerate the seriousness of no_route. A simple example is an origin with only 1 start halt which happens to have 3 unreachable destinations (all have no suitable halts) -- num of pax will be added 3 times to no_route of that single start halt.


//if (ziel_count == 0)
if(ziel_count == 0 && !can_walk_ziel)
{
// DBG_MESSAGE("stadt_t::step_passagiere()", "No stop near dest (%d, %d)", ziel.x, ziel.y);
// Thus, routing is not possible and we do not need to do a calculation.
// Mark ziel as destination without route and continue.
merke_passagier_ziel(destinations[current_destination].location, COL_DARK_ORANGE);
if(start_halts.get_count() > 0)
{
halthandle_t current_halt = start_halts[0];
current_halt->add_pax_no_route(pax_left_to_do);
}
route_good = false;



(4)
Then you proceed to add private car trips if that pax has a car. This will obviously cause double-counting, for you don't know yet if the pax will finally find a good route with an alternative destination and pursue that good route. Even if it ends up with no good route, private car trip should only be added *once* per pax packet, instead of once per failed route. Similarly, the DESTINATION_CITYCARS code portion below should not be called until it is finally determined that there is no route and need to drive a private car.


if(has_private_car)
{

// No way of getting there by public transport, so must go
// by private car if available.

// One day, it would be good if this could check to see whether
// there was a road between the origin and destination towns, too.

//@author: jamespetts

set_private_car_trip(pax_left_to_do, destinations[current_destination].town);

route_good = true;

#ifdef DESTINATION_CITYCARS
//citycars with destinations
if(start_halts.get_count() > 0)
{
//"Produce road users" (Babelfish)
erzeuge_verkehrsteilnehmer(start_halts[0]->get_basis_pos(), step_count, destinations[current_destination].location);
}
#endif
}
current_destination ++;
continue;
}



(5)
Next, you check if the destination is in walking distance. The problem here is, if a destination is within walking distance, you should break out of the while loop (the loop for iterating alternative destinations) instead of continue, because the current pax packet is done.


// check, if they can walk there?
if (can_walk_ziel)
{
// so we have happy passengers
start_halt->add_pax_happy(pax_left_to_do);
merke_passagier_ziel(destinations[0].location, COL_YELLOW);
city_history_year[0][history_type] += pax_left_to_do;
city_history_month[0][history_type] += pax_left_to_do;
route_good = true;

current_destination ++;
continue;
}



(6)
Now the case where there is no route for the current destination. For the portion of code in the body of if(has_private_car), the same problem of double counting happens here. Firstly, the same problem as in point (3) above regarding no_route pax. Secondly, we have not yet determined if private car trip is really necessary, as long as we are not done with the loop for alternative destinations. If we start a private car trip now, but in the next iteration, we found a valid route, we have created unnecessary private car trips in the first place. As a rule, for each pax packet, only 1 private car trip at maximum should be initiated. The same logic applies to the block of code in DESTINATION_CITYCARS below.


else
{
if(start_halts.get_count() > 0)
{
start_halt = start_halts[0]; //If there is no route, it does not matter where passengers express their unhappiness.

start_halt->add_pax_no_route(pax_left_to_do);
merke_passagier_ziel(destinations[current_destination].location, COL_DARK_ORANGE);

}

if(has_private_car)
{
//Must use private car, since there is no suitable route.
set_private_car_trip(num_pax, destinations[current_destination].town);
#ifdef DESTINATION_CITYCARS
//citycars with destination
if(start_halt.is_bound())
{
erzeuge_verkehrsteilnehmer(start_halt->get_basis_pos(), step_count, destinations[current_destination].location);
}
#endif
}
}



(7)
Lastly, the condition when there is no start halt and *no private car*. The check of if(!has_private_car) is unnecessary, for this part of code will only execute if the pax packet has no private car. Similarly, the part of code in DESTINATION_CITYCARS and the final else body should be removed for the same reason.
            

else
{
// the unhappy passengers will be added to all crowded stops
// however, there might be no stop too

// all passengers without suitable start:
// fake one ride to get a proper display of destinations (although there may be more) ...
pax_zieltyp will_return;
//const koord ziel = finde_passagier_ziel(&will_return);
destination destination_now = finde_passagier_ziel(&will_return);

if(!has_private_car)
{
//If the passengers do not have their own private transport, they will be unhappy.
for(  uint h=0;  h<plan->get_haltlist_count(); h++  ) {
halthandle_t halt = halt_list[h];
if (halt->is_enabled(wtyp)) {
//assert(halt->get_ware_summe(wtyp)>halt->get_capacity();
halt->add_pax_unhappy(num_pax);
}
}

#ifdef DESTINATION_CITYCARS
//citycars with destination
erzeuge_verkehrsteilnehmer(k, step_count, destination_now.location);
#endif
merke_passagier_ziel(destination_now.location, COL_DARK_ORANGE);
// we do not show no route for destination stop!
}
else
{
// Otherwise, they have a car and can get to their destination.
// They are not recorded as passengers, and drop out of the system.
// (Except to be recorded as private car trips).
set_private_car_trip(num_pax, destination_now.town);
}
}
// long t1 = get_current_time_millis();
// DBG_MESSAGE("stadt_t::step_passagiere()", "Zeit für Passagierstep: %ld ms\n", (long)(t1 - t0));
}



(8 )
As a final word, there are simply too many preprocessor conditional directives and inactive code embedded in this function which make the code difficult to read. Since you have already made such extensive changes to step_passagiere(), I think it is a good idea to separate the ST STD code in step_passaiere(), and create a new function with another name, putting all your new pathing code in, much like you create a new function rebuild_connexions() instead of putting new code in rebuild_destinations(). In this way, it is easier to maintain and debug your new code, while preserving the old code intact.

jamespetts

Knightly,

thank you for all that work - that's some impressive debugging! I will have to look over all that in more detail in due course. In the meantime, as to the preprocessor directives, I'd appreciate your view on the way forward. The NEW_PATHING preprocessor directive I put in when I was first coding version 2.0, so that I could easily go back to the original if it all went wrong, and so that I could switch and compare easily between the two. Do you think that the time has now come to expunge all the old code?

The DESTINATION_CITYCARS I had better keep for the time being, as there might be some advantages of compiling with it off in some cases.
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

#2
Quote from: jamespetts on May 15, 2009, 11:17:23 PM
The NEW_PATHING preprocessor directive I put in when I was first coding version 2.0, so that I could easily go back to the original if it all went wrong, and so that I could switch and compare easily between the two. Do you think that the time has now come to expunge all the old code?

IMHO, it doesn't hurt to keep them. The only thing is to avoid embedding too many of such preprocessor conditional directives in a single function, frequently interleaving new code with inactive old code. If a function has undergone extensive change compared to ST STD code, it would be better to create 2 separate functions -- one for ST STD and one for ST EXP -- as you have done with rebuild_destinations() and rebuild_connexions().