Add: [AI/GS] Missing water related functions and objects #8390
Conversation
SamuXarick
force-pushed
the
ai-gs-missing-marine-stuff
branch
from
f0204fb to
eb92c1a
Compare
|
Maybe add regression checks for added AITile functions |
SamuXarick
force-pushed
the
ai-gs-missing-marine-stuff
branch
3 times, most recently
from
5980f9e to
0e519ef
Compare
|
This is the new area where the water, river, canal, coast tests are being done for regression. The old area was too simple. |
| GetAge(): 0 | ||
| GetAge(): 1 |
There was a problem hiding this comment.
Why are these (and the following) apparently unrelated changes to do with age necessary?
There was a problem hiding this comment.
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.
SamuXarick
force-pushed
the
ai-gs-missing-marine-stuff
branch
from
0e519ef to
cd7d72a
Compare
SamuXarick
force-pushed
the
ai-gs-missing-marine-stuff
branch
from
cd7d72a to
de5b07c
Compare
SamuXarick
force-pushed
the
ai-gs-missing-marine-stuff
branch
from
de5b07c to
dce883d
Compare
src/script/api/script_tile.hpp
Outdated
| /** | ||
| * 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. |
There was a problem hiding this comment.
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? :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
SamuXarick
force-pushed
the
ai-gs-missing-marine-stuff
branch
from
dce883d to
d83b204
Compare
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.