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

Add: [AI/GS] Missing water related functions and objects #8390

Merged
merged 1 commit into from Sep 14, 2021

Conversation

SamuXarick
Copy link
Contributor

Allows scripts to get the costs of building lock, building canal and clearing water (sea or river in this case).
Allows scripts to check whether a tile is sea and whether a tile is river (checking whether a tile is canal was already possible).

I didn't add the costs for clearing a lock, or canal. Maybe I should, but I didn't want to overdo it.

@SamuXarick SamuXarick marked this pull request as ready for review December 22, 2020 16:18
@glx22 glx22 added the needs review: Script API Review requested from GS/AI expert label Jan 9, 2021
@glx22
Copy link
Contributor

glx22 commented Jan 9, 2021

Maybe add regression checks for added AITile functions

@SamuXarick SamuXarick force-pushed the ai-gs-missing-marine-stuff branch 3 times, most recently from 5980f9e to 0e519ef Compare January 10, 2021 15:33
@SamuXarick
Copy link
Contributor Author

Unnamed, 1954-11-25

This is the new area where the water, river, canal, coast tests are being done for regression. The old area was too simple.

Comment on lines -9102 to +9275
GetAge(): 0
GetAge(): 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these (and the following) apparently unrelated changes to do with age necessary?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A guess could be that the number of operations before the AI is suspended and another game tick simulated is reached with the new tests inserted.

glx22
glx22 previously approved these changes Sep 13, 2021
/**
* Checks whether the given tile is actually a water tile.
* @param tile The tile to check on.
* @pre ScriptMap::IsValidTile(tile).
* @return True if and only if the tile is a water tile.
* @note Returns false when a buoy was placed on the tile.
Copy link
Member

@TrueBrain TrueBrain Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick, but "was placed" is wrong tense here. Returns false when a buoy is on the tile would be more clear, I think? Mind fixing that real quick? :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does it return for various other water-like tiles?

  • Ship depots (passable by ships)
  • Water-based industries and objects (impassable)

I was going to complain that this was an unintuitive exception before realising the function already existed.

Do the newly-added IsSeaTile/IsRiverTile have the same exception? (looks like the answer is 'yes') If so, should probably note it there too.

There doesn't seem to be a function that checks for buoys, nor for 'passable water tiles' in general; would adding one be useful for AIs? I guess it would need a direction argument to make sense for coasts/depots and then be too complicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's hard to answer this without consulting the results. There are test cases in regression that checks the tiles posted in the screenshot above. Rivers weren't tested, the map doesn't contain any river. Oil Rig wasn't tested either.

Item IsWaterTile IsSeaTile IsRiverTile IsCanalTile IsCoastTile
Ship Depot yes no no no no
Buoy no no no no no
Sea Tile yes yes no no no
Lock Lower Part yes no no no no
Lock Middle Part yes no no no no
Lock Upper Part yes no no no no
Coast Half-Tile no no no no yes
Coast Full-Tile no no no no yes
Canal Tile yes no no yes no
Grass/Tree Ground Tile no no no no no
Dock on Water no no no no no
Dock on Slope no no no no no

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed table!

Looking at that, I honestly think it would be better to return true for buoys in IsWaterTile and consider the previous behaviour as a bug instead of documenting it. It makes no sense for ship depots to qualify but not buoys.

I might make a separate PR.

@TrueBrain TrueBrain added this to the 12.0 milestone Sep 14, 2021
@TrueBrain TrueBrain merged commit 37de878 into OpenTTD:master Sep 14, 2021
@SamuXarick SamuXarick deleted the ai-gs-missing-marine-stuff branch June 3, 2022 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review: Script API Review requested from GS/AI expert
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants