simfab verteile waren

[hreintke]

LS,

I am investigating the behavior of distributing goods between multiple consumers from one factory. I definately came across a bug in the simfab in the verteile_waren function.

Code Select Expand


if(  !welt->get_settings().get_just_in_time()  ) {
// without production stop when target overflowing, distribute to least overflow target
const sint32 ziel_left = ziel_fab->get_eingang()[w].max - ziel_fab->get_eingang()[w].menge;
                    dist_list.insert_ordered( distribute_ware_t( halt, ziel_left, ziel_fab->get_eingang()[w].max, (sint32)halt->get_ware_fuer_zielpos(ausgang[produkt].get_typ(),ware.get_zielpos()), ware ), distribute_ware_t::compare );
// dist_list.insert_ordered( distribute_ware_t( halt, ziel_fab->get_eingang()[w].max, ziel_fab->get_eingang()[w].menge, (sint32)halt->get_ware_fuer_zielpos(ausgang[produkt].get_typ(),ware.get_zielpos()), ware ), distribute_ware_t::compare );
}
else if(  !overflown  ) {
// Station can only store up to a maximum amount of goods per square
const sint32 halt_left = (sint32)halt->get_capacity(2) - (sint32)halt->get_ware_summe(ware.get_besch());
dist_list.insert_ordered( distribute_ware_t( halt, halt_left, halt->get_capacity(2), (sint32)halt->get_ware_fuer_zielpos(ausgang[produkt].get_typ(),ware.get_zielpos()), ware ), distribute_ware_t::compare );
}


the line with the // is the original line, the two above i think should be in.
the error is the sequence of the parameters as defined in the class

Code Select Expand


distribute_ware_t( halthandle_t h, sint32 l, sint32 t, sint32 a, ware_t w )
{
halt = h;
space_left = l;
space_total = t;
amount_waiting = a;
ware = w;

but left and total are swapped.

I don't completely understand yet how storing goods and distribiution works but  I am further checking distribution and possible recalling goods. Will update this when I understand better.

If anyone has a hint or documentation just let me know

Kind regards,

Herman

[prissi]

I could think that the names are just misleading, as in the comparison in distribute_ware_t():

Code Select Expand


static bool compare(const distribute_ware_t &dw1, const distribute_ware_t &dw2)
{
return  (dw1.ratio_free_space > dw2.ratio_free_space)
||  (dw1.ratio_free_space == dw2.ratio_free_space  &&  dw1.amount_waiting <= dw2.amount_waiting);
}


Thus for stops this compares (amount-capacity)/capacity, i.e. smallest when largest amount in storage. For factories it actually compares capacity/amount. Again, this is smallest when nearly full storage. The worst are over-/underruns and division by zero problems, but other than that both will lead to the same order, imho.

[hreintke]

Prissi,

I general you might be right (I will do some debug/spreadsheet work to see how it actually behaves but especially in special cases it doesn't work. For example If a factory doesn't have any stock (menge= 0).
The ratio_free_space is calculated as

Code Select Expand


ratio_free_space = max(space_left,0) / max(space_total>>fabrik_t::precision_bits,1);

That means in case of factories with menge = 0 (in the current implementation) ratio_free_space = capacity -> always the same target will be selected instead of depending on amount_waiting on the halt.
Also the use of the space_total>>fabrik_t::precision_bits will cause some issues as ratio_free_space outcome is then also dependent on size of the factory.
I used the code below to overcome that.

Code Select Expand


    static bool compare(const distribute_ware_t &dw1, const distribute_ware_t &dw2)
{   
//        return  (dw1.ratio_free_space > dw2.ratio_free_space )
//                ||  (dw1.ratio_free_space == dw2.ratio_free_space  &&  dw1.amount_waiting <= dw2.amount_waiting);

        if (dw1.ratio_free_space > (dw1.ratio_free_space *1.1))
        { return true;}
        else
        {
            return (dw1.amount_waiting <= dw2.amount_waiting);
        };
           
};


Implementing both the initial patch (swapping parameters) and the one above gave me nice spreading of goods for both receiving factories.

Will continue to work on this.

Herman

[TurfIt]

You can look somewhere around r3760 and r5030 for the changes I'd made fixing things once... Since 5030 something seems to have broken again, but I've not had a chance to dig back into this code...
Description of the 3760 changes: http://forum.simutrans.com/index.php?topic=5919.msg57121#msg57121
And 5030: http://forum.simutrans.com/index.php?topic=8809.msg82001#msg82001

Forcing the comparisons equal with the ratio_free_space equation allows the index_offset to give the intended round robin distribution. Should work for both full and empty fabs/stops.
Note also the differing behaviours with JIT on and off. IIRC, with it off, the distribution does depend on the factory size - fill them up on a % full basis.

From the old development forum:

Was pretty sure I had i giving round robin service to all attached stations as long as the destination station was the same. But like I said, back to square 1 investigating this...

The offset you introduced does not have an effect in the situation at hand. As long as there is an empty station connected to the factory this station will receive goods, and non-empty stations will not receive anything.

An option would be to introduce a constraint in the route-finding like: do not route over stations connected to the same factory (or report NO_ROUTE if the best route goes over such a station).

Which hints I never had empty stop case working to begin with.

There's also an apparent bug with the recall function which somebody hit here: http://forum.simutrans.com/index.php?topic=9564.0 But again haven't had a chance to track down what's going wrong.

Finally, your patch has floating point - absolutely not allowed in the simulation code. Multiplayer desyncs...

[hreintke]

Turfit,

Do I understand you correctly that in the code for r3760 the distribution to multiple factories was not working OK, fixed in r5030 but in later versions was broken again ?

Herman

[TurfIt]

It was r3786 where the patch was committed fixing distribution from a factory to multiple attached stations, and from a factory to multiple destination factories.
r5030: fixing an integer overflow and the just_in_time behaviour. JIT has been broken/fixed many times...


What suspect behaviour are you seeing?

[hreintke]

Turfit,

I have two factories both delivering to two customers.
1/ Steel factory delivering to a car factory and a shop
2/ Car factory delivering to two car dealers

In both cases the all customers all have no supply (eingang.menge = 0)
But the delivering factory is only "verteile waren" to one of the customers so in the halt next to the factory only goods for one destination exists. So I end up with no supply at all to one customer. I expected an even distribution between the two.

That is why I started looking and found the way of instancing the distrubute_ware_t in the dist_list_insert ordered call.
See the update I made below.

Code Select Expand


if(  !welt->get_settings().get_just_in_time()  ) {
  // without production stop when target overflowing, distribute to least overflow target
const sint32 ziel_left = ziel_fab->get_eingang()[w].max - ziel_fab->get_eingang()[w].menge;
dist_list.insert_ordered( distribute_ware_t( halt, ziel_left, ziel_fab->get_eingang()[w].max (sint32)halt->get_ware_fuer_zielpos(ausgang[produkt].get_typ(),ware.get_zielpos()), ware ), distribute_ware_t::compare );
//dist_list.insert_ordered( distribute_ware_t( halt, ziel_fab->get_eingang()[w].max, ziel_fab->get_eingang()[w].menge, (sint32)halt->get_ware_fuer_zielpos(ausgang[produkt].get_typ(),ware.get_zielpos()), ware ), distribute_ware_t::compare );
}


as the original code was hitting side effects on the usage of the max function in calculation of the ratio_free_space.

Code Select Expand


ratio_free_space = max(space_left,0) / max(space_total>>fabrik_t::precision_bits,1);


Then I still had the same issue and found that also the usage of fabrik_t::precision_bits in one side of the calculation had some side effects.

That is why I (trial) recoded the compare function to

Code Select Expand


    static bool compare(const distribute_ware_t &dw1, const distribute_ware_t &dw2)
{   
//        return  (dw1.ratio_free_space > dw2.ratio_free_space )
//                ||  (dw1.ratio_free_space == dw2.ratio_free_space  &&  dw1.amount_waiting <= dw2.amount_waiting);

        if (dw1.ratio_free_space > (dw1.ratio_free_space *1.1))
        { return true;}
        else
        {
            return (dw1.amount_waiting <= dw2.amount_waiting);
        };
           
};


Getting this in I get on each of the halts near the delivering factories an outgoing storage of 50% to each of the customers.
That is what I would expect.

I will some further investigation on the compare function as it was just a first try and understand from your posting I need to find a solution without use of floating point.

Herman

[TurfIt]

The cases you've described should've been fixed in 3786, with last_leiferziel_start.

Have you tested your changed across all possibilities? Factory to single factory, factory to multiple factories, single source station, mulitple sources, single dest, multiple dest, sources empty, source(s) full, fabs empty, fulll, JIT on, JIT off, etc... and all combinations thereof. I can't find my test games that had this all setup. It's usually one of these cases that works sub optimally.

[hreintke]

Turfit,

I am also comparing verteile_waren R3786 to current code. Have already seen some functional differences but will finish comparison and post the results when done completely.

And no, I certainly did not test all possibilities. First this is still investigation phase and not final bugfix phase and secondly, as I am only a starter in simutrans/simutrans coding, I do not (yet) have setup a full test environment.
But if you could find your testgame(s) with this setup, maybe I can use those for more extensive testing.

Herman

[hreintke]

LS,

I compared R3786 to current trunk and found the folowing

In current the in simfab, distribute_ware_t there is this code :

Code Select Expand


  1273     prissi ware = w;
  5030     turfit // ensure overfull stations compare equal allowing tie breaker clause (amount waiting)
  5030     turfit ratio_free_space = max(space_left,0) / max(space_total>>fabrik_t::precision_bits,1);
  1273     prissi }
  1273     prissi distribute_ware_t() {}
  4434     dwachs
  4434     dwachs static bool compare(const distribute_ware_t &dw1, const distribute_ware_t &dw2)
  4434     dwachs {
  4434     dwachs return  (dw1.ratio_free_space > dw2.ratio_free_space)
  5030     turfit ||  (dw1.ratio_free_space == dw2.ratio_free_space  &&  dw1.amount_waiting <= dw2.amount_waiting);
  4434     dwachs }


in R3786 there was not yet a compare function but the usage was computed as follows :

Code Select Expand


const sint32 usage = (space_left << precision_bits) / space_total;


So difference is the usage of precision_bits on space_left vs space_total


For calling the dist_list_insert current trunk code is :

Code Select Expand


  4614     prissi if(  !welt->get_settings().get_just_in_time()  ) {
  4780     prissi // without production stop when target overflowing, distribute to least overflow target
  5030     turfit dist_list.insert_ordered( distribute_ware_t( halt, ziel_fab->get_eingang()[w].max, ziel_fab->get_eingang()[w].menge, (sint32)halt->get_ware_fuer_zielpos(ausgang[produkt].get_typ(),ware.get_zielpos()), ware ), distribute_ware_t::compare );
  4434     dwachs }


In R3786 the code was

Code Select Expand


if(!welt->get_einstellungen()->get_just_in_time()) {

// distribution also to overflowing factories
if(still_overflow  &&  !overflown) {
// not overflowing factory found
still_overflow = false;
dist_list.clear();
}
if(still_overflow  ||  !overflown) {
dist_list.insert( distribute_ware_t( halt, halt_left, halt->get_capacity(2), ware ) );



So the difference is swapping the capacity and space_left parameters in the distribute_ware(...)

Changing the current back to R3786 code :

in distribute_ware_t

Code Select Expand


// ratio_free_space = max(space_left,0) / max(space_total>>fabrik_t::precision_bits,1);
ratio_free_space = (space_left << fabrik_t::precision_bits) / space_total;


and verteile waren

Code Select Expand


const sint32 ziel_left = ziel_fab->get_eingang()[w].max - ziel_fab->get_eingang()[w].menge;
                    dist_list.insert_ordered( distribute_ware_t( halt, ziel_left, ziel_fab->get_eingang()[w].max, (sint32)halt->get_ware_fuer_zielpos(ausgang[produkt].get_typ(),ware.get_zielpos()), ware ), distribute_ware_t::compare );
// dist_list.insert_ordered( distribute_ware_t( halt, ziel_fab->get_eingang()[w].max, ziel_fab->get_eingang()[w].menge, (sint32)halt->get_ware_fuer_zielpos(ausgang[produkt].get_typ(),ware.get_zielpos()), ware ), distribute_ware_t::compare );


restores the functional behaviour of distribution of goods from one factory to multiple consumers.

Herman

[TurfIt]

space_left<<precision_bits results in an integer overflow for some fabs. Hence why I changed to _total>>precision. Are you seeing underflows now?

As for the parameter swap, assuming I did that in 5030, I can't remember why; Presumably for a good reason, but if I broke things...
I'll take a look at this again tonight.

[hreintke]

Turfit,

I do see unwanted side effects from the situation now.
One factory two consumers, only one connected but no transport to the connected due to the fact that the unconnected one is on top of the dist_list.

Will also investigate more.

Herman

[TurfIt]

Can you post a savegame demonstrating the failure to distribute to multiple consumers?
I can't replicate this behaviour with any test cases I've tried. Instead, I've found another case where extra unexpected distributions are being made (goods taking a multi hop journey when a direct route exists).

[hreintke]

Turfit,

I uploaded P64_nl_flat_9-e13 where you can see the issue at the steelfactory at 490,291 and the carfactory at 302,259.

Herman

[hreintke]

Turfit,

I found where my change interferes with next steps.
A few lines further in verteile_waren there is this code

Code Select Expand


menge = min( menge, 9 + best->space_left );
// ensure amount is not negative ...
if(  menge<0  ) {
menge = 0;
}


And by swapping the parameters the value of best->space_left has changed

Herman

[TurfIt]

I can't imagine wanting play with just_in_time off, hence can't foresee how players who do think it should behave. So, how should it?

Never got any response to this; And still can't imagine wanting to play with JIT off. If one were to describe how it should behave, it can be coded. Currently, the goods are being distributed to the factory with the least amount in stock.

Please try r5902. Bug in the goods recall routine fixed.

[hreintke]

Turfit,

Didn't have time to look into it today, neithter will have coming days. But will investigate further and get back on it.

Herman