Skip to main content
Topic: [Code v6.1] On step_p****agiere() (Read 4465 times) previous topic - next topic

[Code v6.1] On step_p****agiere()

James,

I have reviewed the whole of step_p****giere() when I located and fixed the bugs mentioned in the other thread. However, I have some questions with certain portions of your code.

(1)

Code: [Select]
				if(route_good == good)
{
pax.arrival_time = welt->get_zeit_ms();

// All p****engers will use the quickest route.
start_halt = start_halts[best_start_halt];

pax.set_origin(start_halt);

// Now, decide whether p****engers would prefer to use their private cars,
// even though they can travel by public transport.
uint16 distance = accurate_distance(destinations[current_destination].location, pos);
if(has_private_car)
{
//Weighted random.
uint8 private_car_chance = simrand(100);
if(private_car_chance < 1)
{
// If the chances are zero, skip all the rest of this code. This
// will speed up processing, and enable reversion to older behaviour.
goto public_transport;
}

//Check first that car journey is within time tolerance.

// As the crow flies distance. This is very much an approximation - *but*
// we use the standard speedbonus speed (for 'buses), which are generally
// slower than cars, so, very approximately, it should balance correctly.
// TODO: (Long-term) get the accurate road distance between each town
// and have a speedbonus.tab entry for private cars.
const uint16 car_distance = accurate_distance(k, destinations[current_destination].location);
const sint32 car_speed = welt->get_average_speed(road_wt) > 0 ? welt->get_average_speed(road_wt) : 1;
const uint16 car_minutes = (((float)car_distance / car_speed) * welt->get_einstellungen()->get_journey_time_multiplier() * 60.0F);

May I know why in determining the value of the variable distance, pos (position of the city) instead of k (position of the origin building) is used? If this is just a mistake, then the car_distance variable alone is enough.

Besides, isn't it necessary to scale the distances with distance_per_tile? The same issue happens with finde_p****agier_ziel() when determining distance between 2 towns :

Code: [Select]
			for(uint8 i = 0; i < 32; i ++)
{
zielstadt = welt->get_town_at(random);
distance = accurate_distance(this->get_pos(), zielstadt->get_pos());
if(distance <= max_distance && distance >= min_distance)
{
break;
}

(2)

For this portion of code :

Code: [Select]
						// This is the speed bonus calculation, without reference to price.
const ware_besch_t* p****engers = pax.get_besch();
const uint16 average_speed = (60 * distance) / (best_journey_time * (1.0F - welt->get_einstellungen()->get_journey_time_multiplier()));
const sint32 ref_speed = welt->get_average_speed(road_wt) > 0 ? welt->get_average_speed(road_wt) : 1;
const uint16 speed_bonus_rating = convoi_t::calc_adjusted_speed_bonus(p****engers->get_speed_bonus(), distance, welt);
const sint32 speed_base = (100 * average_speed) / ref_speed - 100;
const float base_bonus = (float)speed_base * ((float)speed_bonus_rating / 100.0F);
//base_bonus should be 1 if the average speed is the same as the bonus speed.

In determining average_speed, may I know why best_journey_time is multiplied by (1 - journey_time_multiplier)? And why is distance multiplied by 60, but not 600 as in add_connexion()? It seems to me that your formulas are often different in different places.


(3)

In the following part of code :

Code: [Select]
					// now try to add them to the target halt
uint32 max_ware = ret_halt->get_capacity(wtyp->get_catg_index());
if(!ret_halt->is_overcrowded(wtyp->get_catg_index()))
{
// prissi: not overcrowded and can recieve => add them
if (found)
{
ware_t return_pax(wtyp, ret_halt);
return_pax.menge = pax_left_to_do;
return_pax.set_zielpos(k);
return_pax.set_ziel(start_halt);
if(ret_halt->find_route(return_pax) != 65535)
{
return_pax.arrival_time = welt->get_zeit_ms();
ret_halt->starte_mit_route(return_pax);
ret_halt->add_pax_happy(pax_left_to_do);
}
else
{
ret_halt->add_pax_no_route(pax_left_to_do);
}
}
else
{
// no route back
if(has_private_car)
{
//Must use private car, since there is no route back.
set_private_car_trip(num_pax, destinations[current_destination].town);
}
else
{
ret_halt->add_pax_no_route(pax_left_to_do);
}

}
}
else
{
// return halt crowded
ret_halt->add_pax_unhappy(pax_left_to_do);

if(has_private_car)
{
//Must use private car, since the halt is crowded.
// However, check first that car journey is within time tolerance.

// As the crow flies distance. This is very much an approximation - *but*
// we use the standard speedbonus speed (for 'buses), which are generally
// slower than cars, so, very approximately, it should balance correctly.
// TODO: (Long-term) get the accurate road distance between each town
// and have a speedbonus.tab entry for private cars.
const uint16 car_distance = accurate_distance(k, destinations[current_destination].location);
const sint32 car_speed = welt->get_average_speed(road_wt) > 0 ? welt->get_average_speed(road_wt) : 1;
const uint16 car_minutes = (((float)car_distance / car_speed) * welt->get_einstellungen()->get_journey_time_multiplier() * 60.0F);

if(car_minutes <= destinations[current_destination].tolerance)
{
set_private_car_trip(num_pax, destinations[0].town);
#ifdef DESTINATION_CITYCARS
//citycars with destination
erzeuge_verkehrsteilnehmer(k, step_count, destinations[0].location);
#endif
}
else
{
if(current_destination == 0)
{
// If p****engers cannot get to their destination by private car because the journey is too long,
// they should have a chance of having alternative destinations. This should be re-set on the *first*
// p**** only.
max_destinations = (welt->get_einstellungen()->get_max_alternative_destinations() < 16 ? welt->get_einstellungen()->get_max_alternative_destinations() : 15) + 1;
}
if(!start_halts.empty())
{
if(current_destination + 1 >= destination_count)
{
// Only record too slow if alternative destinations not canv****ed.
start_halts[best_start_halt]->add_pax_too_slow(pax_left_to_do);
}
}
}
}

There are 2 places which check whether to use private car if available. However, the first one does not check for tolerance level and does not create citycars with destination; while the second one does both. I think this is inconsistent.

Besides, for this specific part of code :

Code: [Select]
								if(current_destination == 0)
{
// If p****engers cannot get to their destination by private car because the journey is too long,
// they should have a chance of having alternative destinations. This should be re-set on the *first*
// p**** only.
max_destinations = (welt->get_einstellungen()->get_max_alternative_destinations() < 16 ? welt->get_einstellungen()->get_max_alternative_destinations() : 15) + 1;
}
if(!start_halts.empty())
{
if(current_destination + 1 >= destination_count)
{
// Only record too slow if alternative destinations not canv****ed.
start_halts[best_start_halt]->add_pax_too_slow(pax_left_to_do);
}
}

This is inside the part of code that deals with return trip when a good route has already been found and the current pax packet has already chosen to take public transport or private car to travel from origin to destination. I don't understand why you should allow an alternative destination again, for this will cause the problem of double counting -- so the same pax packet is to go from origin to 2 different places?

Besides, please don't register a case as "Too Slow" on the ground that private car does not meet tolerance level. Otherwise, the figure will be misleading to players -- they don't know how many of such cases are caused by public transport. If origin halt is overcrowded and private car does not meet tolerance level, please register as unhappy; similarly, if there is no route and private car does not meet tolerance level, please register as no route.


Knightly

Re: [Code v6.1] On step_p****agiere()

Reply #1
James,

Have you read this yet? No comment?

Knightly

Re: [Code v6.1] On step_p****agiere()

Reply #2
Knightly,

sorry for the delayed reply to this one: it did require some careful picking through the code, and I have been quite busy this week. To answer your points:

(1) thank you for that suggestion - I have simplified the code as you suggested in the first extract by using "k" (in 6.2). I cannot do that in the second extract, however, because "k" is not in scope in that part of the code. Also, accuracy is less important there. The distances are already scaled, and do not need to be scaled again.

(2) I cannot remember exactly how I reached the particular formula that I used now - it would take a long time of careful calculating and cross-checking to work out precisely why I used those figures. I do remember that I tested them all carefully for mathematical consistency, however, and in many places combined functions: for example, if I needed the output of the standard calculation multiplied by 10, I'd just multiply by 600 instead of 60.

(3) Thank you for spotting that - I have added the destination city cars to the first part (in 6.2). I have not added a tolerance check because that is dealing with return journeys, and, if p****engers have taken a trip in one direction, they will generally go home however long that it takes them!

(4) I have now made sure (in 6.2) that there are no erroneous "too slow" recordings in the way that you suggest - you will have noticed a change in the way in which the "route_good" variable works between 6.1 and 6.2 to take account of this.

I hope that this answers your queries - sorry that I couldn't give a better answer in relation to the formula.
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.

Re: [Code v6.1] On step_p****agiere()

Reply #3
James,

(1)
The distances are already scaled, and do not need to be scaled again.

I realise now that the distance in the first block has been scaled in a later formula. I would suggest the distance be scaled before ****igning to the variable distance, as that will make it clear that the distance is already scaled. Of course, the variable has to be changed to float.

As for the distance in finde_p****agier_ziel(), I really can't find any scaling before comparison against max_distance and min_distance. Maybe I am blind -- do you mind pointing me to the code where you have scaled that distance value?


(2)
I cannot remember exactly how I reached the particular formula that I used now - it would take a long time of careful calculating and cross-checking to work out precisely why I used those figures.

Here is a formula extracted from haltestelle_t::add_connexion() :

T = (D x M x K) / S

where
T = time
D = distance
M = distance_per_tile
K = 600 or 60
S = speed

Rearraging terms will give

S = (D x M x K) / T

which is not mathematically equivalent to your formula :

S = (K x D) / [T x (1 - M)]

I don't know how you can arrive at the current formula, but very likely it is wrong. Probably you have wrongly written (1 / M) as (1 - M).


(3)
I have not added a tolerance check because that is dealing with return journeys, and, if p****engers have taken a trip in one direction, they will generally go home however long that it takes them!

I agree, but then for the same reason please also remove the tolerance check here :

Code: [Select]
						// return halt crowded
ret_halt->add_pax_unhappy(pax_left_to_do);

if(has_private_car)
{
//Must use private car, since the halt is crowded.
// However, check first that car journey is within time tolerance.

// As the crow flies distance. This is very much an approximation - *but*
// we use the standard speedbonus speed (for 'buses), which are generally
// slower than cars, so, very approximately, it should balance correctly.
// TODO: (Long-term) get the accurate road distance between each town
// and have a speedbonus.tab entry for private cars.
const uint16 car_distance = accurate_distance(k, destinations[current_destination].location);
const sint32 car_speed = welt->get_average_speed(road_wt) > 0 ? welt->get_average_speed(road_wt) : 1;
const uint16 car_minutes = (((float)car_distance / car_speed) * welt->get_einstellungen()->get_journey_time_multiplier() * 60.0F);

if(car_minutes <= destinations[current_destination].tolerance)
{
set_private_car_trip(num_pax, destinations[0].town);
#ifdef DESTINATION_CITYCARS
//citycars with destination
erzeuge_verkehrsteilnehmer(k, step_count, destinations[0].location);
#endif
}
else
{
if(current_destination == 0)
{
// If p****engers cannot get to their destination by private car because the journey is too long,
// they should have a chance of having alternative destinations. This should be re-set on the *first*
// p**** only.
max_destinations = (welt->get_einstellungen()->get_max_alternative_destinations() < 16 ? welt->get_einstellungen()->get_max_alternative_destinations() : 15) + 1;
}
if(!start_halts.empty())
{
if(current_destination + 1 >= destination_count)
{
// Only record too slow if alternative destinations not canv****ed.
start_halts[best_start_halt]->add_pax_too_slow(pax_left_to_do);
}
}
}

BTW, have you removed the alternative destination chance in this code block? As I have explained, return trip should not get another destination.


Thank you very much for taking the time to investigate these issues and for your reply. :)


Knightly

Re: [Code v6.1] On step_p****agiere()

Reply #4
Knightly,

thank you very much for your helpful analysis. I have found that the average speed formula was incorrect, producing values approximately half what they should be. I have replaced the line with:

Code: [Select]
const uint16 average_speed = ((distance * welt->get_einstellungen()->get_distance_per_tile()) * 600) / best_journey_time;

which I have checked for consistency with a formula used elsewhere by converting it back into journey time and checking it against the original journey time. Save for what appear to be minor rounding inaccuracies, it works.

Thank you also for pointing out the incorrect tolerance calculation of a return trip: I have removed that.

As for the addition of an alternative destination, p****engers with private cars are generally set to have no alternative destinations as in this code:

Code: [Select]
uint8 max_destinations = has_private_car ? 1 : (welt->get_einstellungen()->get_max_alternative_destinations() < 16 ? welt->get_einstellungen()->get_max_alternative_destinations() : 15) + 1;

This makes the code more efficient, as p****engers with private cars rarely need alternatives (and never needed them at all before the introduction of journey time tolerance, which also applies to cars). The addition of the further alternative destination later in the code is necessary after the introduction of the journey time tolerance feature in order to give car drivers a possible alternative when there is no suitable route by public transport and the car journey is too long.
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.

Re: [Code v6.1] On step_p****agiere()

Reply #5
James,

(1)

So, may I know where has the distance variable been scaled before comparison with max_distance and min_distance in finde_p****agier_ziel()?

OK, I realised now that you have scaled the min and max distances when parsing simuconf.tab. I think it would be better to keep min and max distances as they are, but scale any distance to be compared instead. Although both approaches yield the same outcomes, my suggestion would be more logical and consistent -- consistent, in the sense that all calculated distances should be scaled using distance_per_tile.


(3)

The addition of the further alternative destination later in the code is necessary after the introduction of the journey time tolerance feature in order to give car drivers a possible alternative when there is no suitable route by public transport and the car journey is too long.

Please take a look again at the code block which I extracted for point (3) in my previous post. It is the part of code that deals with *return trip* when a *good route has already been found* beforehand. Regardless of whether the pax packet has taken public transport or its own private car, the trip from origin A to destination B has already been registered. Thus, even if return trip is not possible due to an overcrowded halt B, the pax packet should not be given another alternative destination C. Otherwise, the same pax packet will go from A to B and from A to C at the same time.


Knightly

Re: [Code v6.1] On step_p****agiere()

Reply #6
1. It is faster to multiply the values once when they are read from the configuration file than to multiply them many times whenever they are used, so I'd prefer to keep the current arrangement.

2. Ahh, yes; in which case, that has been deleted in any event as part of deleting the check for journey time tolerance on the return.
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.

 

Re: [Code v6.1] On step_p****agiere()

Reply #7
James

1. It is faster to multiply the values once when they are read from the configuration file than to multiply them many times whenever they are used, so I'd prefer to keep the current arrangement.

I also know that there is a little performance advantage in your current approach, but I think consistency is more important in this case and in general than the little performance gain. In fact, if not due to this inconsistency, I would not have asked the question in the first place. Anyway, it's up to you to decide which approach to adopt.

Knightly