Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Game script - Town growth, transport service condition #5934

Closed
DorpsGek opened this issue Mar 2, 2014 · 10 comments
Closed

Game script - Town growth, transport service condition #5934

DorpsGek opened this issue Mar 2, 2014 · 10 comments
Labels
component: AI/Game script (squirrel) This issue is related to Squirrel (Scripting language) flyspray This issue is imported from FlySpray (https://bugs.openttd.org/)

Comments

@DorpsGek
Copy link
Member

DorpsGek commented Mar 2, 2014

The_Dude opened the ticket and wrote:

I am trying to implement new GS features to my Simple City Builder script, and I get into one small trouble.

Script influences town in such way, that they need to be supplied to grow houses. I added new growth mechanism, that uses new features of setting town growth as TOWN_GROWTH_NORMAL when town is supplied and TOWN_GROWTH_NONE when not. This all works fine,
but when town has enough resources and is not serviced, the town will not grow, but the GS will think it grows, because town was supplied.

Since GS cant recognize, that the town has no transport service, it gives the user invalid information about town growth in that case. That is when town gui says correctly the town is not growing, but GS will say in goal gui the town is growing (even if I omit the growing line, the user can still be confused, if he sees the deliveries).

How it would be possible to implement some new GS feature?

Either gather the service information directly in town_cmd.cmd in
static void UpdateTownGrowRate(Town *t)
and send it to GS, like GSTown.HasService(townid),

or make some GSStation function to retrieve struct Station properties :
byte time_since_load
byte time_since_unload
, which would make it possible to decide the service status in GS.

The latter could be also useful for other interesting scripts too.

Reported version: 1.4.0-beta5
Operating system: All


This issue was imported from FlySpray: https://bugs.openttd.org/task/5934
@DorpsGek
Copy link
Member Author

DorpsGek commented Mar 3, 2014

The_Dude wrote:

Eddi was so kind to suggest a simple solution, I am posting it hereby.
It would indeed provide the information of town actually not growing to GS.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/5934#comment13129

@DorpsGek
Copy link
Member Author

DorpsGek commented Mar 9, 2014

Zuu wrote:

While this change is possible useful for you, it will break other scripts that rely on the old behaviour. (API returning the growth rate even when the goals are not met)

With the many different town growth scripts available, it is hard to know if you break any script or not with this change. Maybe you can provide a new API with the wanted behaviour and update the Doxygen comments to clarify the difference between GetGrowthRate and GetCurrentGrowthRate or similar.

Edit: The new API should be added to game_changelog.hpp and ai_changelog.hpp


This comment was imported from FlySpray: https://bugs.openttd.org/task/5934#comment13136

@DorpsGek
Copy link
Member Author

DorpsGek commented Mar 9, 2014

frosch wrote:

I don't think just exposing TOWN_IS_GROWING is a good idea. I mean you would need like 3 pages of text to explain when it is set, and when it is not set. And when it is set when you do not expect it to be set, and when it is not set when you would expect it to be set.

Better start with defining what the function should really check and what not, instead of exposing some magic bit.


This comment was imported from FlySpray: https://bugs.openttd.org/task/5934#comment13137

@DorpsGek
Copy link
Member Author

The_Dude wrote:

Zuu is right, I do realize weakness of that too simple patch.

One way could be to pass second optional argument return the days period value only when town is growing, but new function is probably cleaner.

Check included patch. It returns the boolean value depending on TOWN_IS_GROWING. Since TOWN_IS_GROWING bit is really always set when town is growing in any way possible, I do not see reason, why not to expose it. You could check for the exactly same fix as are already being checked elsewhere on the path, but what would be purpose of double checks.
I added comments to possible ways how TOWN_IS_GROWING is set, but I think it is not imporant info for script writers. When I am writing a script, I dont care for exact proccess behind the functions.

I have not added the function for AI in this patch yet, but you get the idea.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/5934#comment13138

@DorpsGek
Copy link
Member Author

frosch wrote:

The issue with TOWN_IS_GROWING is, that it also is affected by funding houses and a 1/12 chance.
I think you rather only want to check the normal growth conditions.


This comment was imported from FlySpray: https://bugs.openttd.org/task/5934#comment13139

@DorpsGek
Copy link
Member Author

The_Dude wrote:

Sure, but it still gives accurate information, that town is or is not growing.
That is the reason and the need of the new function.


This comment was imported from FlySpray: https://bugs.openttd.org/task/5934#comment13140

@DorpsGek
Copy link
Member Author

The_Dude wrote:


This comment was imported from FlySpray: https://bugs.openttd.org/task/5934#comment13141

@DorpsGek
Copy link
Member Author

Zuu wrote:

Some notes on the patch:
- In your doxygen comment you have something that looks like you intended to have a bullet list there. In order for the HTML output to show up as a list, you need to follow the Doxygen syntax for bullet lists, otherwise it will probably even ignore your manual line breaks.
- Items in the changelog is sorted alphabetically - not by date within the same version.
- "more of this conditions" => "more of these conditions" (change "this" to "these")
- In the API documentation, you should not refer to internal ENUM constants which are not exposed in the API. For example TOWN_GROW_RATE_CUSTOM is not exposed in the API. Use descriptions that relate to what is exposed to API users or use plain English if you need to refer to internal logic not exposed in the API. It is better here to refer to existing APIs for how a GS set growth rate. This will make the references clickable and improve understanding of the API.

This line in your patch will not compile:
+ static bool ScriptTown::IsTownGrowing(TownID town_id)


This comment was imported from FlySpray: https://bugs.openttd.org/task/5934#comment13142

@DorpsGek
Copy link
Member Author

The_Dude wrote:

Thanks for review. At this stage I was more interested into developers opinion on implementing that new function.

Semanthic check is not that important now. Give me some sign of approval and I can make working patch with squirrel templates, etc.

I think some API for getting real status of town growth can be useful for more script authors than me (as I tried to point out when using TOWN_GROWTH_NORMAL, in some conditions GS has no way to say that town is growing with 100% accuracy).


This comment was imported from FlySpray: https://bugs.openttd.org/task/5934#comment13143

@DorpsGek DorpsGek added flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) Goal/Game script labels Apr 7, 2018
@TrueBrain
Copy link
Member

I understand the use case, but I do not think there will be a solution that is proper and sustainable. So sorry, passing on this.

@frosch123 frosch123 added the component: AI/Game script (squirrel) This issue is related to Squirrel (Scripting language) label Apr 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: AI/Game script (squirrel) This issue is related to Squirrel (Scripting language) flyspray This issue is imported from FlySpray (https://bugs.openttd.org/)
Projects
None yet
Development

No branches or pull requests

3 participants