Text only | Text with Images

The International Simutrans Forum

Simutrans Extended => Simutrans-Extended development => Topic started by: knightly on August 01, 2009, 07:54:10 am

Title: [Fixes v6.1] Tolerance level in step_p****agiere()
Post by: knightly on August 01, 2009, 07:54:10 am
James,

I have fixed 3 bugs and made some minor optimisations in step_p****agiere(). I have already uploaded my commit to Github, so please check it out when you have time.

The bugs are :

(1)

When calculating tolerance levels, the format of your formulas is simrand(max) + min. This is wrong, and may result in a figure above max. The proper format should be simrand(max - min + 1) + min. The " + 1 " is needed because simrand(X) will return random values in the range of [ 0 , (X-1) ], inclusive.


(2)

You only use a single tolerance variable to store the tolerance levels of the alternative destinations. Thus, the value stored in tolerance will always be the tolerance level for the last destination. When you are evaluating the different destinations, your code always compare journey time against the tolerance level of the last destination. I have fixed it by adding a member variable for tolerance in the destination structure.


(3)

You misplaced the break in the following code, causing the identity check among start halts and end halts to be incomplete when there are multiple start halts :

Quote
unsigned ziel_count = 0;
for (uint h = 0; h < dest_plan->get_haltlist_count(); h++)
{
   halthandle_t halt = dest_list[h];
   if (halt->is_enabled(wtyp))
   {
      ziel_count++;
      for(int i = start_halts.get_count(); i >= 0; i--)
      {
         if(start_halts.get_count() > i && halt == start_halts[ i ])
         {
            can_walk_ziel = true;
            start_halt = start_halts[ i ];
            haltestelle_t::erzeuge_fussgaenger(welt, start_halts[ i ]->get_basis_pos3d(), pax_left_to_do);
         }
      }
         break; // because we found at least one valid step ...
   }
}

And due to this bug, you added the following check, which is necessary when the abovementioned identity check is incomplete :

Quote
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];

   if(start_halt == pax.get_ziel())
   {
      // Without this, where there are overlapping stops, p****engers
      // for nearby destinations accumulate at a stop, destined for
      // that same stop, and never leave.
      can_walk_ziel = true;
      goto walk; // Not ideal, I know. Can anyone suggest anything better that does not increase overhead?
   }


I have fixed the bug and removed the check which is no longer necessary now.


Knightly
Title: Re: [Fixes v6.1] Tolerance level in step_p****agiere()
Post by: jamespetts on August 01, 2009, 09:33:15 am
Thank you very much for your fixes - most helpful. Sorry that I haven't replied to your PMs yet - have been busy. I shall have a look at those again in a minute.

As to fix no. 3, I am not sure that I fully understand what you mean: the tolerance level is the tolerance of the p****engers, not of the destinations. Thus, a single packet of p****engers with multiple destinations will have the same level of tolerance no matter whether they are going to their primary, secondary or tertiary (etc.) destination. The idea is that the tolerance level represents the overall journey time above which the p****engers no longer think travelling worthwhile at all. It would make no sense if they were prepared to travel for longer to get to their second choice destination than their first choice destination (when they would only be going to their second choice destination if their first choice destination is unreachable, or is only reachable by means that exceed their journey time tolerance).

Thank you again for your help :-)
Title: Re: [Fixes v6.1] Tolerance level in step_p****agiere()
Post by: knightly on August 01, 2009, 10:35:25 am
James,

The tolerance level is still the tolerance level of pax of course. I think you have forgotten that you have different tolerance levels for different ranges? Journey time for destinations generated with different ranges has to be compared against tolerance level of the matching range, right?

Your original code also calculates different tolerance levels for different range with randomisation. If you take a look at what I have changed, you will realise that what I am doing is in line with your original intention.

Knightly
Title: Re: [Fixes v6.1] Tolerance level in step_p****agiere()
Post by: knightly on August 06, 2009, 07:43:37 pm
James,

I can see that you have changed the code for tolerance. This is not a problem. But can you please refrain from such long and obscure program statement, such as the line below?

Code: [Select]
const uint16 tolerance = wtyp != warenbauer_t::p****agiere ? 0 : range == local ? simrand(welt->get_einstellungen()->get_max_local_tolerance()) + welt->get_einstellungen()->get_min_local_tolerance() : range == midrange ? simrand(welt->get_einstellungen()->get_max_midrange_tolerance()) + welt->get_einstellungen()->get_min_midrange_tolerance() : simrand(welt->get_einstellungen()->get_max_longdistance_tolerance()) + welt->get_einstellungen()->get_min_longdistance_tolerance();

I am sorry. I don't have the patience to decode and understand such unclear code.

Edit :

If you have reviewed my code for tolerance level, you should know that I have already added static const variables for the various tolerance levels in the beginning part of step_p****agiere(). Please make use of them, instead of repeatedly calling welt->get_einstellungen()->get_something() again and again.

Knightly
Title: Re: [Fixes v6.1] Tolerance level in step_p****agiere()
Post by: jamespetts on August 07, 2009, 09:52:36 pm
Knightly,

thank you for your comments. I have, as you suggested, changed to using your variables rather than calling the methods repeatedly. As to the long line of code: I have now broken it down into multiple lines. I have kept it semantically identical so that I can still use const, but added whitespace for clarity.
Title: Re: [Fixes v6.1] Tolerance level in step_p****agiere()
Post by: knightly on August 08, 2009, 06:39:22 am
James, thank you very much for changing it :)  It would be even nicer if you can enclose each ternary operator with parentheses.
Text only | Text with Images