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

@SamuXarick SamuXarick commented Dec 16, 2020

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 Dec 22, 2020
@SamuXarick SamuXarick force-pushed the ai-gs-missing-marine-stuff branch from f0204fb to eb92c1a Jan 9, 2021
@glx22
Copy link
Contributor

@glx22 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 Jan 10, 2021
@SamuXarick
Copy link
Contributor Author

@SamuXarick SamuXarick commented Jan 10, 2021

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.

GetAge(): 0
GetAge(): 1
Copy link
Member

@LordAro LordAro May 1, 2021

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

Copy link
Contributor

@nielsmh nielsmh May 1, 2021

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 SamuXarick force-pushed the ai-gs-missing-marine-stuff branch from 0e519ef to cd7d72a Jun 6, 2021
@SamuXarick SamuXarick force-pushed the ai-gs-missing-marine-stuff branch from cd7d72a to de5b07c Aug 12, 2021
@SamuXarick SamuXarick force-pushed the ai-gs-missing-marine-stuff branch from de5b07c to dce883d Aug 30, 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

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

@FLHerne FLHerne Sep 14, 2021

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

@SamuXarick SamuXarick Sep 14, 2021

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

@FLHerne FLHerne Sep 14, 2021

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
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants