Factory Link Refactoring

[PJMack]

I have begun to refactor the industry linking code to improve performance with industry generation and upgrading.  Although not nearly ready for a PR, I am far enough along to announce my intentions and post my progress to github.  I will move this topic to the PR sub-forum when appropriate.

The existing code for linking contains a single vector_tpl<koord> of all suppliers and a single vector_tpl<koord> for all consumers.  Although this saves memory, it does require several recalculations for when factories are removed and added as determining whether an link is incorrect requires traversal through all existing links.  This would have made sense at the time it was first written however the feature base, typical industry density, and typical map sizes have all increased.  My solution to this is to split the vectors by ware type to allow simplification of code and reduce the number of loops needed for industry generation or removal.

I am attempting not to change any behavior with the exception of fixing bugs (such as the one where quarries are still linked to modern builders yards).  If behavior change is desirable, this refactoring should make such changes simpler to implement. 

I did find one behavior though that I cannot judge whether it was intended.  In the event that a consumer industry is missing a link, it will link to all possible industries until a the link is filled regardless of whether the new links help.  This means a market looking for meat would link to all the orchards it can find before coming across a slaughter house.

[jamespetts]

Thank you for your work on this - this is most interesting.

To answer your question, I do not think that the behaviour to which you refer in the last paragraph is intended - I suspect that it is a byproduct of the way in which the code is currently written.

[PJMack]

To answer your question, I do not think that the behaviour to which you refer in the last paragraph is intended - I suspect that it is a byproduct of the way in which the code is currently written.

In that case, a correction may be in order. 
I would also like to confirm that scripting and the API are deprecated in Extended and it is safe to disable the API access to the older lists rather than trying to implement a complex adapter which I would have no easy way of testing.

[jamespetts]

In that case, a correction may be in order. 
I would also like to confirm that scripting and the API are deprecated in Extended and it is safe to disable the API access to the older lists rather than trying to implement a complex adapter which I would have no easy way of testing.

Apologies for the delay in replying. Yes, the scripting API has never been fully integrated into Extended and has never worked in Extended. I have not fully stripped it in case anyone wanted to revive this.

[PJMack]

Yes, the scripting API has never been fully integrated into Extended and has never worked in Extended. I have not fully stripped it in case anyone wanted to revive this.

Thank You.  In that case I will not bother trying to integrate the modified function calls into the API.


Although not quite complete (I still have one more bug to fix and extensive testing), I have gotten the functionality in the branch to what it was before, plus-or-minus what was needed for bug fixes.  As mentioned previously, the bug fixes include preventing consumers from wildly attaching to all producers when missing a single good, and preventing consumers from remaining attached to the wrong producers upon upgrade  (any previously made incorrect links would be corrected on loading).  The status for wares is now preserved properly when an upgrade rearranges the goods order.  An aesthetic side effect to this branch is that the list of consumers and providers in the GUI are now in order of product type first, then distance.  An upgraded industry may have the list of wares in a different order than the same new industry.

One other issue that I did come across is the consumer_active_last_month status indicator which had only been working for the first 32 industries.  (It had been that way since 2012, a time when 32 industry links per industry would have been unheard of.)  The indicator is actually mislabeled, as it keeps track of activity within the current month and is reset monthly.  Although I did make a repair, as it is a moderately complex piece of code operating in the step function purely for the purpose of a GUI status indicator (that had not worked reliably for quite some time), I would like to remove that status indication to save on memory and performance.  Would anyone miss that status icon?

[wlindley]

Looking forward to this.

Somewhere on the factory info tabs it would be wonderful to have an "Add one new link" button.  That way, when building new factories, you could add links to only existing factories, one at a time -- instead of the "Build a chain" functionality which sometimes gets far too rambunctious at adding new farms (for example) when there are plenty of existing ones with only a single link.


Also would be most pleasing to eventually be able to (as the public player) click a button to have a factory renegotiate any unproductive links by attaching to different industries.

[Ranran(restricted account)]

Looking forward to this.

Somewhere on the factory info tabs it would be wonderful to have an "Add one new link" button.  That way, when building new factories, you could add links to only existing factories, one at a time -- instead of the "Build a chain" functionality which sometimes gets far too rambunctious at adding new farms (for example) when there are plenty of existing ones with only a single link.


Also would be most pleasing to eventually be able to (as the public player) click a button to have a factory renegotiate any unproductive links by attaching to different industries.

To make a button do what a two-point click tool does, someone would have to define a new tool, and that is a tedious task...

[Matthew]

Thank you for your contributions on this, PJMack!

One other issue that I did come across is the consumer_active_last_month status indicator which had only been working for the first 32 industries.  (It had been that way since 2012, a time when 32 industry links per industry would have been unheard of.)  The indicator is actually mislabeled, as it keeps track of activity within the current month and is reset monthly.  Although I did make a repair, as it is a moderately complex piece of code operating in the step function purely for the purpose of a GUI status indicator (that had not worked reliably for quite some time), I would like to remove that status indication to save on memory and performance.  Would anyone miss that status icon?

I am not quite sure what you mean by this. Do you mean the fact that connected industry links are highlighted in bold? For example:



If so, then I think that indicator does have value, but if it is hammering memory and performance, I it is definitely worthwhile having a discussion about whether it is worth it, preferably informed by some data.

P.S. This is the Message Window buffer completely filled by (less than?) one month's worth of Cattle Farm → Convenience Store messages.

I did find one behavior though that I cannot judge whether it was intended.  In the event that a consumer industry is missing a link, it will link to all possible industries until a the link is filled regardless of whether the new links help.  This means a market looking for meat would link to all the orchards it can find before coming across a slaughter house.

If your patch would stop that behaviour, then it will be even more welcome! But 'only' refactoring the code would be progress in itself, so please keep wrestling that spaghetti in any case.

[PJMack]

To make a button do what a two-point click tool does, someone would have to define a new tool, and that is a tedious task...

I have no plans regarding manual factory adjustments by the public player.

I am not quite sure what you mean by this. Do you mean the fact that connected industry links are highlighted in bold? For example:



If so, then I think that indicator does have value, but if it is hammering memory and performance, I it is definitely worthwhile having a discussion about whether it is worth it, preferably informed by some data.

The status indication I am referring to are the meat icons in some of the suppliers listed.

This is the Message Window buffer completely filled by (less than?) one month's worth of Cattle Farm → Convenience Store messages.

[...]

If your patch would stop that behaviour, then it will be even more welcome! But 'only' refactoring the code would be progress in itself, so please keep wrestling that spaghetti in any case.

I did fix that behavior within this patch.

[Ranran(restricted account)]

The status indication I am referring to are the meat icons in some of the suppliers listed.

I did fix that behavior within this patch.

I think this was mentioned before. In standard, it is represented by a brown pile. It is just inherited from standard.
https://forum.simutrans.com/index.php/topic,20020.msg188231.html#msg188231

[PJMack]

I think this was mentioned before. In standard, it is represented by a brown pile. It is just inherited from standard.
https://forum.simutrans.com/index.php/topic,20020.msg188231.html#msg188231

This is indeed the same thing.  The icon indicates that wares have shipped (e.g. departed from their origin) since the month began.  Because it only worked with 32 destinations from the origin factory, larger maps with several cross connections resulted in a large number of false negatives.  I was able to repair this by setting the width of the bit field dynamically, however this increases the complexity of the code as well as memory usage.

[PJMack]

This stage of refactoring is complete and passed all of my testing.  I created a PR and moved this forum topic.

[prissi]

At which point is there a speedup? I looked through the patch, since I considered it for standard as well, but in principle all the lookups are just in separate functions, but the overall lookup amount seems no much affected.

Also I would suggest to use the regular save game version bump instead introducing the subversion for saving.

[PJMack]

At which point is there a speedup?

The primary speedup is in the disconnect_supplier and disconnect_consumer functions which is where in addition to disconnecting when a valid koord is given, the functions verify that there is at least one link for each good type and attempt to make a correction if needed.  These are called with for each factory monthly as well as well as for each factory when increase_industry_density is called, totaling up to five times per factory at the start of each month plus once for each factory effected by a closure.  These functions originally had to traverse all links to determine which goods are missing whereas now only the wares need to be traversed to determine which goods have an empty list of links.

The secondary source of speedup is in the verteile_waren and increase_industry_density functions where some of the for loops now only traverse the links for the relevant ware rather than traversing all links and filtering within the loop.  the verteile_waren functions is called for each ware for each factory during the step function and the increase_industry_density function is called up to four times at the start of each month.

As you mentioned, some of the lookups have been rearranged and moved, some of which as a side effect to the aforementioned changes, however some changes are purely to ease code readability and maintenance with a neutral effect on performance.

Unfortunately, some of the lesser used functions and code did become more complex.  Reading and writing to the file now takes a little longer to maintain compatibility.  Upgrading industries is now much more complex, however this is mostly due to a bug fix.  Industries were loosing track of the number of goods in transit upon upgrading to an industry with a different order of wares.

I considered it for standard

If you are still considering this for standard, I should warn you that this may break API compatibility.  As discussed early, the API in extended is deprecated but the remnants of the code still in place.  As such, I simply commented out the dead API code for extended, but if that same code is not dead yet in standard, then an adapter function may be necessary.

Also I would suggest to use the regular save game version bump instead introducing the subversion for saving.

There have been some recent issues with version bumps in PRs as discussed in other forum threads.  Such issues not only include merge conflicts that slip through unnoticed (e.g. where git merges automatically though incorrectly), but savefiles from branches can become unopenable after a merge with master.  Although having a subversion number is not the most ideal solution, it does decrease the likely-hood of repeating those issues. 

[prissi]

At least in standard, any patch must be able to save also to a previous version. Hence you could save the latest savegame even as 99.17 version (although of course losing a lot of information).

That way a bump is only done on release, the code of a patch just prepares for it.

[PJMack]

That would be something to consider though it would add a step to merging.  In this particular instance, the state information is purely aesthetic, however some PRs are more major and states would need to be saved and loaded in branch versions in order to test them properly, including verification that balance has not been adversely effected.

[jamespetts]

Apologies for the delay in integrating this - it has been too hot to work much on Simutrans lately and I have also been preoccupied with other matters.

However, I have now briefly tested this and integrated it. Thank you very much for your work on this.