[NoGo] Neighbours are important

[aantono]

I will attach the patch file for my next submission, just haven't gotten around to checking out the repository and applying my changes to it.

Did you by any chance had a look at what could be wrong with the saving/loading issue, why the numbers that are being saved are different from what is being loaded back?

Thanks,
Alex

[Zuu]

main.nut:
- this.fromLoad is not needed. this.loaded_data will be non-null if a save game was loaded. So you don't need a separate control variable for this. Aditionaly, please follow the naming convention and don't use Java camel case in member variables.
- Why do you move the check of the master switch from at the beginning of Init() to after the call to Init in Start()?

town.nut:
This change is quite hackish. If you need the script to always update the text, don't hack in an else clause in the mechanism that only update the text when there is a change. Just remove the whole mechanism that avoids updating the text more than necessary if you must update the text at every ManageTown.

That said, I have kept the mechanism as it is still useful when stockpiling is disabled and then included a path for frequent text updates when stockpiling is enabled.

Code: Select all

 	if (this.prev_text_status != curr_text_status)
 	{
 		this.prev_text_status = curr_text_status;
 		this.UpdateTownText();
+	} else {
+		this.UpdateTownText();
 	}

patch changes
I had a go on fixing the save/load and change it to follow the same save/load pattern as eg. NoCarGoal. As we now save/load, we can also save the neighbours as they are quite CPU consuming to compute. Some data is not saved which is data that will get computed at next Manage. Really, some of those shouldn't be member variables as they are not used for anything in between Manage. But that is a later code improvement of the script unrelated to stockpiling.

cargo_stockpile_ratios is now saved/loaded.

Added a setting to enable/disable stockpiles

I have removed the member variable "rate" as it was totally unused. (not introduced by you)

Remaining issues
- The text output to town window now contain string parameters that are not always set. These will display random numbers in the GUI.
- Stockpile info will only show if neighbour factor is enabled and town growth has not been disabled for that specific town. At minimum stockpile info need to be shown when neighbourhood factor is disabled. Stockpiling should however probably not be displayed when stockpiling is disabled.
- this.cargo_stockpile and this.cargo_stockpile_ratios should change from array to table with town effects as keys.

stockpile2.patch

(19.81 KiB) Downloaded 130 times

Edit: I just reviewed my stockpile2.patch and saving of prev_text_status is not needed. It may save a milisecond somewhere at best which is not worth the code lines and storage used for saving it. Also my added line in Start() should obviously not be part of a clean patch.

[aantono]

Thanks for the changes, as always. I'll incorporate them into the code. Did you had a chance to see why the saving data does not match with what is being loaded?

The changes in the patch don't seem to affect that area, so it is still loading different numbers for me.

[Zuu]

Actually, I didn't test your implementation in-game. But with my patch applied on version 8, I got the same stockpile counts upon load as when it did save. (both in the log and what is displayed in the town window)

IIRC, your implementation didn't call the method to update the town text upon load. So the town window would not update until the next ManageTown at which point the stockpile might get changed due to delivery.


Also, mind that the script tries to have ManageTown called every 20-30 day for each town. The exact delay is randomized so that not all towns should get managed at the same day. A day is 74 ticks. In a default configuration each tick has 10 000 opcodes, but OpenTTD users can lower that amount if they wish. They can also decide how many towns they want in their game. Thus we cannot trust that ManageTown will occur on <= 30 day interval. In some cases the script may not run fast enough to keep up with the amount of towns in that game.

So we can be quite sure that ManageTown may end up being called twice some months and once some moths. In some cases the time between two ManageTown calls can also be more than one month.

Since the mechanism is not guaranteeing that you do not get to manage a town exactly every month (no more, no less), it may be better to use GSCargoMonitor rather than GSTown.GetLastMonthReceived. The CargoMonitor will report the amount delivered since last time you checked the cargo monitor and will thus better work in the asynchronous world of Game Scripts.


Edit: If you want to use GSCargoMonitor (which I suggest) or any API introduced in 1.3 API, a change in info.nut is required to declare that this GS needs OpenTTD 1.3 or later. Now it says that it support minimum API 1.2. Note that you will be able to run the GS in 1.3 without this change. What this change does is that the GS will not show up in 1.2 anymore and that the 1.2 compat.nut will no longer be loaded. Instead the 1.3 compat will be loaded (on trunk/1.4).

Code: Select all

	function GetAPIVersion()	{ return "1.2"; }

[aantono]

Yep, I think your way of saving data works, at least I've been monitoring things and the numbers seem to be matching.

For using GSCargoMonitor, I would probably collect the delivered cargo for all the companies in the game, right? Kinda makes sense, since the town would grow regardless who delivers cargo, it's the total consumed by the town.

For converting array stockpile to a table, do you really forse the addition of other town effect cargo types to be added to the game/engine? Cause if it is not, why bother changing, array access is faster than table, even though they are both O(1) complexity?

Can you explain where/how this happens? - The text output to town window now contain string parameters that are not always set. These will display random numbers in the GUI.

Thanks,
Alex

[Zuu]

For using GSCargoMonitor, I would probably collect the delivered cargo for all the companies in the game, right? Kinda makes sense, since the town would grow regardless who delivers cargo, it's the total consumed by the town.

Yes, Imho we shouldn't differentiate on whom have delivered the cargo. I don't see it makes any sense in the Neighbours are important context. In some uber complex CityBuilder context it might be used, but for this script I think that would be too complex.

For converting array stockpile to a table, do you really forse the addition of other town effect cargo types to be added to the game/engine? Cause if it is not, why bother changing, array access is faster than table, even though they are both O(1) complexity?

I don't see that we will get more town effects than those that we have. However, I would guess that NewGRFs can assign a town effect to any number of cargos they want. But you would need to check the NewGRF spec or ask a NewGRF person to get a better answer on that. Thus if my guess is right, there can be up to 32 stockpiled cargo types. But I would try to verify that guess before doing too much about it. But using tables instead of arrays is a more clean solution that will easier cope with NewGRFs adding more town effect cargos.

[aantono]

I'm looking through the API and can't seem to find a mechanism of how to get a list of all companies. Any suggestions/hints/pointers?

Also, can you explain your comment from the previous post, where/how this happens? - "The text output to town window now contain string parameters that are not always set. These will display random numbers in the GUI."

[Zuu]

I'm looking through the API and can't seem to find a mechanism of how to get a list of all companies. Any suggestions/hints/pointers?

Code: Select all

for (local c = GSCompany.COMPANY_FIRST; c <= GSCompany.COMPANY_LAST; c++) {
    if (GSCompany.ResolveCompanyID(c) != GSCompany.COMPANY_INVALID) {
        // code that operate on company c
    }
}

Also, can you explain your comment from the previous post, where/how this happens? - "The text output to town window now contain string parameters that are not always set. These will display random numbers in the GUI."

For stockpile info, you use a string STR_TOWN_GOAL_STOCKPILE_INFO_GUI with constant number of parameters. Yet, the number of parameters that you set depend of the number of cargos in this.town_effect_cargos. Thus if this.town_effect_cargos is too short you will get results like shown in this screenshot:

Unnamed, 12th Jan 1950.png (43.35 KiB) Viewed 4622 times

[aantono]

Also, can you explain your comment from the previous post, where/how this happens? - "The text output to town window now contain string parameters that are not always set. These will display random numbers in the GUI."

For stockpile info, you use a string STR_TOWN_GOAL_STOCKPILE_INFO_GUI with constant number of parameters. Yet, the number of parameters that you set depend of the number of cargos in this.town_effect_cargos. Thus if this.town_effect_cargos is too short you will get results like shown in this screenshot:

That is very interesting. I always thought that the number of TE_cargos is always 5 - passengers, mail, food, goods and 'water'? Is it not always the case, does it vary by "climate" map type?

[planetmaker]

Also, can you explain your comment from the previous post, where/how this happens? - "The text output to town window now contain string parameters that are not always set. These will display random numbers in the GUI."

For stockpile info, you use a string STR_TOWN_GOAL_STOCKPILE_INFO_GUI with constant number of parameters. Yet, the number of parameters that you set depend of the number of cargos in this.town_effect_cargos. Thus if this.town_effect_cargos is too short you will get results like shown in this screenshot:

That is very interesting. I always thought that the number of TE_cargos is always 5 - passengers, mail, food, goods and 'water'? Is it not always the case, does it vary by "climate" map type?

You can have arbitrary many as it can be set via NewGRFs: http://newgrf-specs.tt-wiki.net/wiki/Ac ... 8.2C_19.29

[Zuu]

Or at least up to 32 which is the maximum number of cargo types in a game. Eg. look back what I explained earlier about this.

I don't see that we will get more town effects than those that we have. However, I would guess that NewGRFs can assign a town effect to any number of cargos they want. But you would need to check the NewGRF spec or ask a NewGRF person to get a better answer on that. Thus if my guess is right, there can be up to 32 stockpiled cargo types. But I would try to verify that guess before doing too much about it. But using tables instead of arrays is a more clean solution that will easier cope with NewGRFs adding more town effect cargos.

Edit: I'm not a NewGRF author, so I don't know the details on exactly what NewGRFs can do. But the general rule is: expect that they can alter things until it has been proven otherwise.

So, thank you planetmaker for providing a proof for my expectation that it cannot be trusted to not alter the amount of Town Effect cargos.

[aantono]

You can have arbitrary many as it can be set via NewGRFs: http://newgrf-specs.tt-wiki.net/wiki/Ac ... 8.2C_19.29

Looking at the spec it says that you can have many different cargoes fullfil one of the town growth kinds, but there seem to be only 5 possible cargo town effects => [passengers, mail, food, goods and water]. You can have "alcohol and milk both have TE_GROWTH_WATER mask, but the number of possible masks is always 5. Or am I misunderstanding something?

[planetmaker]

You can have arbitrary many as it can be set via NewGRFs: http://newgrf-specs.tt-wiki.net/wiki/Ac ... 8.2C_19.29

Looking at the spec it says that you can have many different cargoes fullfil one of the town growth kinds, but there seem to be only 5 possible cargo town effects => [passengers, mail, food, goods and water]. You can have "alcohol and milk both have TE_GROWTH_WATER mask, but the number of possible masks is always 5. Or am I misunderstanding something?

That's right. There's only 5 different kinds of town effects. But there can be 32 cargos which have a town effect - each causing one or more of each type of town effect.

[aantono]

So I think I see what's going on with the text GUI generation. Looks like in Tropical town requires all 5 cargo effects, in Arctic it requires pass, mail, goods and food, and in Temperate it wants only 3 - pass, mail and goods.

So I will make 3 different STR_ definitions, one for each.

Question: Should I make them to be in the form of
=> STR_TOWN_GOAL_STOCKPILE_INFO_GUI_TROPIC, STR_TOWN_GOAL_STOCKPILE_INFO_GUI_ARCTIC, STR_TOWN_GOAL_STOCKPILE_INFO_GUI_TEMPERATE,
or be
+ STR_3_TOWN_GOAL_STOCKPILE_INFO_GUI, STR_4_TOWN_GOAL_STOCKPILE_INFO_GUI, STR_5_TOWN_GOAL_STOCKPILE_INFO_GUI

Thanks,
Alex

[Zuu]

I'd suggest naming the strings using numbers. That way you can use the feature that you just refer to string

Code: Select all

GSText.STR_0_STOCK_PILE_INFO + this.town_effect_cargos.len()

In OpenTTD this just magically works (both in Squirrel code and in C++ code). All you need is that the strings are ordered in the language file as 0, 1, 2, 3, ... n.

Adding in climate references will just complicate it for no benefit. When NewGRFs come into play your climate stuff will quickly break.

Mind that there can be 32 cargos that provide town effects. It can probably exist multiple cargos for the same town effect. I don't know if a cargo also can provide multiple town effects. (eg. expect it until you have found proof in the NewGRF spec that it is not possible to happen) Also mind that with 32 cargos you will run into the GSText parameter count limit. I don't remember exactly which value it is, but if a NewGRF adds 32 TownEffect cargos you will not be able to display all in the TownGUI.

Also, you may want from a conceptually point want to think a round what happens if there is 2 or more cargos for the same town effect. Will they share the same stockpile? Or will there be one stockpile for each cargo? Depending on what you pick, that may reflect how you want to deal with it in the GUI.

Last, while there may be 32 town effect cargos, I find a such game unlikely. Thus you a practical solution is to figure out how many stockpiles that you can support and then perhaps apply a sorting mechanism so that the stockpiles with least (or most?) amount of cargo will drop out of the view.

[aantono]

I've been reading the NewGRF spec more and it says there that even though there could be multiple cargos that have the same town growth effect, as well as multiple different effects fulfilled by the same cargo (i.e. milk can be both "food" and "water"), the town display GUI (at least currently) will only display One cargo for each town effect, and it will be the first one (id wise) in the list.

So even though I can track different cargoes in stockpiles (have a table of tables), I won't actually need to display them all, cause the town GUI will only display one for each kind.

[aantono]

Latest update which handles the GUI rendering, as well as proper saving and loading stockpiles and cargo goals - contains various code cleanup and refactoring.

[Zuu]

From code point of view, I have reviewed your patch and it looks good.

I see though that you still uses GSTown.GetLastMonthReceived(). It will return the amount of cargo delivered for the previous calender month. Your SetCargoGoal method is called with 20 to 30 days interval usually but less often if the town has too many towns for the script to be able to keep up. I still wonder/think that your code will give unexpected results if SetCargoGoal is called twice the same month or never some month. Please convince me that I'm wrong if you think so.

[aantono]

The reason I would not want to do that is because of the notion of a timed interval of cargo delivery. In order for the town to grow, it should get a certain amount of cargos delivered PER MONTH, not per script's internal scan interval. So if the check happens twice in one month, it is ok, because we will handle the same goals twice, so the math will work out (same values added and subtracted, netting in 0). If we do incremental changes, then we can handle lower amount of cargo delivered and prevent town from growing, where indeed enough cargo would be delivered within the month timefrme. Thoughts?

[Zuu]

This is the code for updating stockpiles. Other than that, there is an if-statement to set the stockpile to zero if delivery is less than cargo_requirement. (so that you don't get negative stock piles)

Code: Select all

		// Calculate leftover cargo from previous cycle
		local cargo_received = GSTown.GetLastMonthReceived(this.id, cargo_type);
		local cargo_extra = max(cargo_received - GSTown.GetCargoGoal(this.id, cargo_type), 0);

		// Calculate new stockpile amount, adding the leftover from recent delivery, 
		//	after satisfying the existing goal
		this.cargo_stockpile.rawset(cargo_type, this.cargo_stockpile.rawget(cargo_type) + cargo_extra);

When this block of code is called twice the same month, GetLastMonthReceived will return the same amount of cargo twice. Thus you will count delivery twice into cargo_extra. The second time GetCargoGoal() will likely return a lower value than the first one.

Also if the code above only runs every second month, you will run into problems. (you will not regard all cargo delivered since last stockpile management) Fixing this is non-trivial as your solution to reduce the cargo requirement set in OpenTTD somewhat relies on that you get to manage the town at least once a month. Thus if someone lower the #oopcode setting or run a game with too many towns, the script will not be able keep up. If that is filed under "hard to fix", perhaps a warning should be given?

Thus a possible solution is: 1) Use GSCargoMonitor for handling a town being managed twice a month. That will also give less lag for stockpile updates. 2) Add some way to detect if the script runs too slow compared to the amount of towns to keep up with the minimum speed required when Stockpiles is enabled.