Our NeighborsWater method is currently used for two different purposes, which should have two different logical evaluations, and which quite reasonably both think it is the method they should be calling. This is mostly on me as I added the first iteration of it, which focused purely on map graphics needs but gave it a name that was very inviting for other purposes.
As a result, right now, it looks like this:
foreach (Tile neighbor in getDiagonalNeighbors()) {
//check for whether it's a water tile
Aside from questions about whether you consider the "diagonal" tiles to be the ones that share edges or vertices (currently in the code, it means the ones that share edges), the thing that should raise eyebrows is why doesn't it check all the neighboring tiles? For important questions such as, "can a tile on this city build naval units?", we want to check all neighboring tiles.
The reason it's written the way it is currently is that for the question, "do we want to display large forest/jungle/marsh graphics, or small ones?", the relevant question is "do the neighbors that share an edge with this tile contain water? if so, use small graphics." The method was written with this definition of neighboring water in mind, and got picked up for everything else.
So in essence this is a classic equivalent of when you get corruption in your database because one part of the business uses Term A and means one thing, and another part of the business uses Term A and means something else, and IT doesn't realize that Term A can mean two different things, and re-uses the same field. Like in most of those cases, there were also a few months between the first use of NeighborsWater and the subsequent uses of it, so there was time to forget exactly what it meant in the first place.
I suggest we use the "any neighboring tile is a water tile" definition for NeighborsWater, and have the graphics uses cases use the getDiagonalNeighbors method and LINQ to figure out what they need. They're the specialized case here.
This is also a bug because if you found a city that only borders water via vertices, it cannot build Galleys; the same applies for barbarian camps that only neighbor water via vertices.
Our NeighborsWater method is currently used for two different purposes, which should have two different logical evaluations, and which quite reasonably both think it is the method they should be calling. This is mostly on me as I added the first iteration of it, which focused purely on map graphics needs but gave it a name that was very inviting for other purposes.
As a result, right now, it looks like this:
Aside from questions about whether you consider the "diagonal" tiles to be the ones that share edges or vertices (currently in the code, it means the ones that share edges), the thing that should raise eyebrows is why doesn't it check all the neighboring tiles? For important questions such as, "can a tile on this city build naval units?", we want to check all neighboring tiles.
The reason it's written the way it is currently is that for the question, "do we want to display large forest/jungle/marsh graphics, or small ones?", the relevant question is "do the neighbors that share an edge with this tile contain water? if so, use small graphics." The method was written with this definition of neighboring water in mind, and got picked up for everything else.
So in essence this is a classic equivalent of when you get corruption in your database because one part of the business uses Term A and means one thing, and another part of the business uses Term A and means something else, and IT doesn't realize that Term A can mean two different things, and re-uses the same field. Like in most of those cases, there were also a few months between the first use of NeighborsWater and the subsequent uses of it, so there was time to forget exactly what it meant in the first place.
I suggest we use the "any neighboring tile is a water tile" definition for NeighborsWater, and have the graphics uses cases use the getDiagonalNeighbors method and LINQ to figure out what they need. They're the specialized case here.
This is also a bug because if you found a city that only borders water via vertices, it cannot build Galleys; the same applies for barbarian camps that only neighbor water via vertices.