Border tile assignment improvements #839
Conversation
…xpansion level greater than 2. 2. Added more conditions to how a tile ownership is determined, and how/if unassigned tiles are correctly assigned to a city.
| return map.tileAt(XCoordinate + xDelta, YCoordinate + yDelta); | ||
| } | ||
|
|
||
| public Tile FindInRing(int rank, Func<Tile, bool> predicate, bool clockwise = true) { |
There was a problem hiding this comment.
Do we need this in addition to GetTilesWithinRankDistance, which returns things in spiral order?
There was a problem hiding this comment.
GetTilesWithinRankDistance returns tiles in a big fat cross style. I tried to use it, but for borders, although it's fine for the first rank, the 2nd rank doesn't include a NN, WW, EE or SS tile. This method goes around the "actual" ring around a tile. I called it ring to distinguish it from rank, which behaves differently.
I hope that makes sense.
There was a problem hiding this comment.
Makes sense, thanks! Could you put that in a comment on the method to explain how it's different and why we need it?
|
Played about 2 games of 200 rounds each, skipping animations etc, only building Temples, Libraries and whatever would give me culture points, did not come across any after the latest fix. |
C7Engine/C7GameData/Tile.cs
Outdated
| return null; | ||
| } | ||
|
|
||
| private bool IsInValidCoordinates(int dx, int dy, out Tile currentTile) { |
There was a problem hiding this comment.
Hmm, apparently adding a comment on a commit doesn't show up in the review (the GitHub code review tool suuuuuucks)
A couple of questions from my comment on the commit:
- Why "dx" and "dy" if these are absolute positions, not deltas?
- Is the method name "is in valid coordinates" or "is invalid coordinates"? Would "GetTileIfValid" work, and have the return type be Tile??
- How is this function different from map.tileAt which I believe does the tile wrapping for us
There was a problem hiding this comment.
Sorry I missed the part where map.tileAt was wrapping, to be honest I just checked that it returns a Tile.None and not a null.
Thanks for the feedback!
…cean tile with a rank larger than 2
|
I found that for the tiles that are opposite to each other and need to be filled in, Law II (Ocean tiles may only hold claims of rank 2.) was not being applied. 
Top row is the original game, bottom left what we had before and on the right what we have after this fix. I refactored the method that was calculating because there was too much duplication, really without any good reason. I had to also refactor the ResolveTileOwnershipConflict method, because we somehow have to assign null to a tile that doesn't belong to a city and the bool/out City setup made more sense to me. |
2 participants
// Ocean tiles may only hold claims of rank 2.
The current version has a bug, it should check if the tile is more than rank 2, not the city's current expansion level.
Case study: (Scenario) WWII In the Pacific
City: Manokwari (Above Australia)
Tiles: [37, 71] [38, 72] should belong to the city
You can also see this with Tokyo as well, it skips a row of tiles because its expansion level is more than 2, but the tiles themselves aren't.
I have tested this mostly with the Conquests scenarios that come with the game. There a few minor discrepancies, but I think they are temporary, as you play either game (openCiv3/original), they iron out.