Meaning of brown pile display of the factory info window

[Ranran(retired)]

I'm wondering how to handle this display in the new GUI. 
I haven't changed this decision function is_active_lieferziel(), but it behaves differently than I expected.

What I expect is if the two factories are properly connected by a line or convoy with the correct goods category.
Actually it doesn't seem to be.

People playing on the server will often see this route disappearing even though the route exists.
For that I checked if it was connected many times. And the player finds this pile untrustworthy. 


Examples of other abnormalities that can be confirmed
Open this saved game
1) Check out the bakery near Sparrowpike Central Stop. It shows a mountain that is connected to the windmill, but only the passenger line runs at that bus stop. 
I have never set up a freight line at that bus stop.


2) Check out the colliery and coal marchant connected by rail. This is the trap I used to fall in. 
Although there is a cargo attribute, the cargo is not carried because the capacity is 0. But it is fooled by the brown pile. (´・ω・｀)


What does this brown pile represent?

[jamespetts]

I have looked at this briefly. The code in question for this is as follows:

Code Select Expand


bool fabrik_t::is_active_lieferziel( koord k ) const
{
assert( lieferziele.is_contained(k) );
return 0 < ( ( 1 << lieferziele.index_of(k) ) & lieferziele_active_last_month );
}


"lieferziel" is German for "delivery target" according to Babelfish.

This code is unchanged from Standard, as can be observed here.

Its only use, both in Standard and Extended, is in the GUI - it does not have any effect on any simulation parameters. All that it does in Extended is determine whether to display this symbol.

The code is very odd: it uses bitwise logic in what should be a basic GUI method that is not at all computationally intensive and does not use any bitfields.

What it seems to check is whether the index of the passed co-ordinate, when bit-shifted one to the left and combined using a bitwise & with the variable lieferziele_active_last_month, is greater than zero.

In turn, the variable lieferziele_active_last_month is only used in this particular GUI function, although it has been loaded and saved since Standard version 112002. There is no code comment explaining this variable.

This variable, in turn, is set in only one place (aside from some places where it is reset to 0), and that is void fabrik_t::verteile_waren(const uint32 product).

The line is:

Code Select Expand

// add as active destination
lieferziele_active_last_month |= (1 << lieferziele.index_of(best_ware.get_zielpos()));

This appears to be set if any goods are sent to their destinations. Again, there is bitshifting of the index of the target in the vector, the purpose of which I do not understand at all.

In short, I suggest that this system shows nothing useful that I can discern and, unless a Standard developer who understands this can explain what this is actually for, I suggest that this logic be replaced with something more meaningful: this is only used in the UI and does not affect any simulation logic.

[Mariculous]

Okay, that seems quite odd in the first place. Let's see what's going on:

Code Select Expand

return 0 < ( ( 1 << lieferziele.index_of(k) ) & lieferziele_active_last_month );

1. shift the one by the retrieved index to the left.
  That means 0b1<< 3 = 0b1000 = 8
  It's not 1<< 3 = 1<<0b11 = 0b110 = 6 That's not correct! Forget about this line!

In any case, this seems to be very dangerous! 1 is an int, i.e. it's signed.Left-shifting signeds is well-defined, as long as it's not shifted to or beyond the sign bit.
So if we can ensure our retrieved index is never larger than - whatever the number of bits in our target environment - we are fine. Otherwise, anything can happen.

2. &-combine the result from step 1 with lieferziele_active_last_month, which is according to its name, the active delivery targets of the last month.
That means, the 1 we set in the previous step will remain, if there is a 1 in lieferziele_active_last_month, otherwise all bits will be 0.

3. return true if that one bit was retained, otherwise false.

Code Select Expand

lieferziele_active_last_month |= (1 << lieferziele.index_of(best_ware.get_zielpos()));
The same considerations about undefined signed shifting behavior applies here again.
Here we shift the one by index to the left again, that means the bit at that position indicates that the delivery target was active last month.
Now we |-comine that information into the lieferziele_active_last_month bitfield.
That means, we set the bit of the lieferziele_active_last_month  bitfield at the location of the obtained index to 1.

Apart from the undefined behavior thing and bad code readability, this seems fine to me. We should assert that we do never run into undefined behavior here.

[jamespetts]

Another problem here might be if we have more than 32 connected industries.

[Mariculous]

Another problem here might be if we have more than 32 connected industries.

That's exactly what the undefined behavior is about
Though, to be preciese, 32 is already too large (as that would shift the one to the sign bit, which is undefined behavior) and to be very preciese, it depends on the target systems size of int, it might as well be 16 or 64 bits on some target architechtures.

[jamespetts]

I am not even sure that the intended behaviour of this method is particularly useful - it is still not clear to me what it is supposed to be doing.