-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Zone of Control #4085
Zone of Control #4085
Conversation
Units that can move after attacking are not affected by zone of control if they move because of defeating a unit.
Ciao! I found this very detailed post about zones of control. In particular this list of special cases could be handy: Cities
Embarked Units
Naval Units vs Land Units
Units able to move after attacking
Miscellaneous
|
Thank you for the input! I'll mark this PR as a draft until I have implemented the additional special cases.
This appears to contradict the text on the wiki. The forum posts has screenshots, though, so I'll consider it to be the more trustable source. |
Oh My God |
All special cases have been implemented, so this PR is now ready. |
I'm still debating internally. I think the code looks good, I'm just not sure what the performance effect would be. I think a compromise would be to put this as an 'opt-in', the problem is that this should be per-game not per user because it affects gameplay... |
I see. Making this a game-specific setting is certainly possible, just give the word if you want me to do that. In the case that this change severely impacts performance, perhaps it could be further optimized by caching the shared neighbors for every neighbor for every tile on the map, but that might be a bit much. On another note, what would be the best way to profile the performance of |
The ideal situation would be to start with an advanced game with lots of units and comparing the nextTurn timing of the same turn, with it without this function. |
In that case, I'll wait for you to provide the file. |
The_Ottomans_-_401_turns_Lag.txt |
No worries. I'll take a look at this again later, if/when I get back to developing. |
is this outdated by now? |
This is not outdated. Zone of Control is functionally completely implemented, and I don't expect that recent changes have broken anything. This may even merge without conflicts, though I can't see that while it's a draft. What still needs to be done is an investigation of what the performance impact of this change is. I fiddled around a bit with Android Studio's profiler, but I have't quite figured it out yet. To be honest, I am currently not very interested in figuring out the profiler, but I might come back to this in a while. If anyone wants to take over this PR before then, they are free to do so! The concrete todo-list:
|
I think performance should come after rule completeness... And maybe whatever this costs could be offset by #4830... And maybe this should begin with unit tests which could also serve as temporary test bed for performance measurements. AS Profiling works with an Android target only, and I found it to be of little help. Seems aimed at Android development using an integrated UI framework, not Gdx, nor did I find how to instrument and measure selected code... The common neighbor problem could probably also be solved by something inspired by HexMath.getClockDirectionToWorldVector - but that one thinks in world coordinates. |
A testing framework I did here:(New file Unciv/tests/src/com/unciv/logic/map/ZoneOfControlTests.kt): package com.unciv.logic.map
import com.unciv.Constants
import com.unciv.logic.MapSaver
import com.unciv.logic.civilization.CivilizationInfo
import com.unciv.models.ruleset.Nation
import com.unciv.models.ruleset.Ruleset
import com.unciv.models.ruleset.RulesetCache
import com.unciv.testing.GdxTestRunner
import org.junit.Assert
import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
@RunWith(GdxTestRunner::class)
class ZoneOfControlTests {
private var ruleSet = Ruleset()
private var tileMap = TileMap()
private var goodGuys = CivilizationInfo()
private var goodUnit = MapUnit()
private var fromTile = TileInfo()
private var evilGuys = CivilizationInfo()
private var evilUnit = MapUnit()
@Before
fun initTheWorld() {
RulesetCache.loadRulesets()
ruleSet = RulesetCache.getBaseRuleset()
goodGuys.initCiv("Good Guys")
evilGuys.initCiv(Constants.barbarians)
// Sorry for the 320 char literal - simpler than constructing this world on foot
@Suppress("SpellCheckingInspection")
val worldZippedJson = "H4sIAAAAAAAAAJWSsQqDMBRFfygOmu2tHboUWmihQ+nwWjMENMpLpLYh/16DU4vCdRHEw/Hm3sSW+xMLtyYY8RSn17P9GIrCtR08aeWmb7QbfOjapF6dNPVVuKcgg0kq2MYcrA90iw/25mJE2DraC3vfsKuTin3nbbCdozhSUar39EgKgTMLoagS1I0ohh6kqPKpKwzOLIjObYJwiXpRJRxzy+YFPPoIt7Sh+E2DorDOsP6Fj0/D7g/MHIDNPwfACvSVoK9EfIgKirV0t1fjA9zSZKv1AhzSLDg6kkqD8TXWxuxbBO/pC3I2qwsUBgAA"
tileMap = MapSaver.mapFromSavedString(worldZippedJson)
tileMap.setTransients(ruleSet, false)
evilUnit = placeUnit(0, 0, "Archer", evilGuys)
goodUnit = placeUnit(1, 1, "Warrior", goodGuys)
fromTile = goodUnit.getTile()
}
private fun CivilizationInfo.initCiv(name: String) {
tech.unitsCanEmbark = true
civName = name
nation = Nation().apply {
this.name = name
cities = arrayListOf("The Capital")
}
}
private fun placeUnit(x: Int, y: Int, name: String, civInfo: CivilizationInfo): MapUnit {
val unit = ruleSet.units[name]!!.let {
it.ruleset = ruleSet
it.getMapUnit(ruleSet)
}
unit.assignOwner(civInfo, false)
unit.currentMovement = 5f
val tile = tileMap[x, y]
tile.militaryUnit = unit
tile.setUnitTransients(false)
return unit
}
@Test
fun testZocMovementCost() {
@Suppress("UNUSED_VARIABLE")
val otherEvilUnit = placeUnit(0, 2, "Archer", evilGuys)
var destinationTile = tileMap[2,2]
val moveAwayCost = goodUnit.movement.getMovementCostBetweenAdjacentTiles(fromTile, destinationTile, goodGuys)
destinationTile = tileMap[1,0]
val moveToCommonTileCost = goodUnit.movement.getMovementCostBetweenAdjacentTiles(fromTile, destinationTile, goodGuys)
destinationTile = tileMap[1,2]
val moveToOtherZocCost = goodUnit.movement.getMovementCostBetweenAdjacentTiles(fromTile, destinationTile, goodGuys)
Assert.assertTrue("ZOC rules: moving away from enemy should cost 1 but costs $moveAwayCost", moveAwayCost == 1f)
Assert.assertTrue("ZOC rules: moving within ZOC should cost all MP but costs $moveToCommonTileCost", moveToCommonTileCost > 99f)
Assert.assertTrue("ZOC rules: moving to another unit's ZOC should cost 1 but costs $moveToOtherZocCost", moveToOtherZocCost == 1f)
}
@Test
fun testZocCanReach() {
val destinationTile = tileMap[0,-1] // 2 spaces around the enemy, reachable in 4 moves
goodUnit.currentMovement = 4f
val canReachIn4 = goodUnit.movement.canReachInCurrentTurn(destinationTile)
goodUnit.currentMovement = 3f
val canReachIn3 = goodUnit.movement.canReachInCurrentTurn(destinationTile)
Assert.assertTrue("ZOC rules: Leaving and reentering ZOC possible in 4 moves", canReachIn4)
Assert.assertFalse("ZOC rules: Leaving and reentering ZOC not possible in 3 moves", canReachIn3)
}
@Test
fun measureZocMovementCostPerformance() {
val runtime = Runtime.getRuntime()
val startFreeMem = runtime.freeMemory()
val startTime = System.nanoTime()
val sumCosts = measureZocMovementCostPerformanceRunner(iterations = 50_000_000)
println("measureZocMovementCostPerformance took ${(System.nanoTime()-startTime)/1000}µs and ${(startFreeMem-runtime.freeMemory())/1024}kB")
//Assert.assertTrue("ZOC rules: Performance test should calculate a sum of 0.8*iterations", sumCosts == 40000000.0)
}
private fun measureZocMovementCostPerformanceRunner(iterations: Int): Double {
val destinationTiles = listOf(tileMap[1,0], tileMap[2,1], tileMap[2,2], tileMap[1,2], tileMap[0,1])
var iteration = 0
var sumCosts = 0.0
while (true) {
for (destinationTile in destinationTiles) {
if (iteration++ >= iterations) return sumCosts
sumCosts += goodUnit.movement.getMovementCostBetweenAdjacentTiles(fromTile, destinationTile, goodGuys)
}
}
}
} This runs in current master, and fails at all the right places. Timing baseline on my tiny NUC is around 1.8 seconds for 50M iterations of Next:
|
So the pathing thinks this is allowed and a unit with 3mp can do that in one turn, with this ZOC code in place: Anybody help setting up more complicated tests? |
Thanks for taking a look at this PR so I don't have to! :) I included a concise summary of what I believe to be the full ZoC rules as a comment in As for your last post: the path should indeed be allowed. The (1,1)->(2,1)->(1,0) movement costs 2 MP (no ZoC effects here) and the final move to (0,-1) simply consumes all remaining MP, so 3 MP is indeed enough. In fact, even 2.1 MP would suffice. |
An implementation of essentially Yair's idea gave:
Source code:(New fun in HexMath.kt): /** returns a Sequence of 2 Vectors for two neighboring hexes - those they share a border with.
* Does ***not*** work if the input hexes are not neighbors or for worldWrap maps.
*/
fun commonAdjacentTiles(pos1: Vector2, pos2: Vector2): Sequence<Vector2> {
if (pos1.x > pos2.x || pos1.y > pos2.y) return commonAdjacentTiles(pos2, pos1)
val dx = pos2.x > pos1.x
val dy = pos2.y > pos1.y
return when {
!dx -> sequenceOf(pos1.cpy().add(-1f,0f), pos1.cpy().add(1f,1f))
!dy -> sequenceOf(pos1.cpy().add(0f,-1f), pos1.cpy().add(1f,1f))
else -> sequenceOf(pos1.cpy().add(0f,1f), pos1.cpy().add(1f,0f))
}
} The key function in UnitMovementAlgorithms calls it like this: /** Returns whether the movement between the adjacent tiles [from] and [to] is affected by Zone of Control */
private fun isMovementAffectedByZoneOfControl(from: TileInfo, to: TileInfo, civInfo: CivilizationInfo): Boolean {
// Sources:
// - https://civilization.fandom.com/wiki/Zone_of_control_(Civ5)
// - https://forums.civfanatics.com/resources/understanding-the-zone-of-control-vanilla.25582/
//
// Enemy military units exert a Zone of Control over the tiles surrounding them. Moving from
// one tile in the ZoC of an enemy unit to another tile in the same unit's ZoC expends all
// movement points. Land units only exert a ZoC against land units. Sea units exert a ZoC
// against both land and sea units. Cities exert a ZoC as well, and it also affects both
// land and sea units. Embarked land units do not exert a ZoC. Finally, units that can move
// after attacking are not affected by zone of control if the movement is caused by killing
// a unit. This last case is handled in the movement-after-attacking code instead of here.
//
// We only need to check the two shared neighbors of [from] and [to]: the way of getting
// these two tiles can perhaps be optimized.
return HexMath.commonAdjacentTiles(from.position, to.position).map {
from.tileMap[it]
}.any {
(
(
it.isCityCenter() &&
civInfo.isAtWarWith(it.getOwner()!!)
)
||
(
it.militaryUnit != null &&
civInfo.isAtWarWith(it.militaryUnit!!.civInfo) &&
(it.militaryUnit!!.type.isWaterUnit() || (!it.militaryUnit!!.isEmbarked() && unit.type.isLandUnit()))
)
)
}
} |
Yes you still have to! Teeeemwork! But thanks and thanks back - seemed a pity it slept its rose sleep down at the bottom of the open PR's.
I hoped someone would cherry-pick the most interesting Pics from that post and translate them to my testbed's placeUnit calls and a "Tile (x,y) should (not) be reachable with (n) movement points" description or even the correct asserts...
Yes! Good one. And yes - |
Indeed! If you have any finished additions for this PR, just point me to a public branch on your fork and I will add them here. Or you could open a replacement PR, either is fine with me.
Yeah, that was something I was afraid of. |
The refreshed baseline is essentially unchanged. 1.6 to 1.8 seconds, and master still fails exactly the right tests. I fear the optimization potential looks bleak. Getting world wrap in there will cost, and the small advantage might be lost... Almost not worth the effort. But... There is no world wrap code in HexMath and IMHO it should be - where else? So, isolating the relevant tricks might be a worthwhile learning experience. Status:
|
Not so faithfully, cattle is missing xD |
Awwwwww... 🤣 you could have gimped that 😭 ... I still don't know where they came from. Must be aliens in the Unciv newgame code, on the prepared map they're not. |
Or maybe... There were aliens in civ5 and they took the cattle with them 😨 Jokes aside, I have also noticed some resources generate on an empty map. I guess someone slipped in some code, when they were trying to add resource placement for custom maps. |
The 👽 insubordination is in function |
Added the knowledge gained from SomeTroglodyte's tests.
Looks like the current conclusion is that my initial implementation is surprisingly the most efficient one, but still increases the time spent in I could try a few more approaches, but I don't expect to reach significantly better performance. @SomeTroglodyte, I cannot run your testing code from #4085 (comment) because Either way, I think that this is ready to merge (if the performance impact is deemed acceptable). The addition of an in-game wiki explanation can be done in a future PR. |
Unrelated: I only just noticed that the hexagon layouts of the original Civ V and Unciv are different. That's interesting. |
I'm not sure how to fix the failing check, so I'd appreciate it if someone else took a look at it. |
It's trying to run Gradle 7.02 with the same old JVM which won't work... No idea where to tell travis which JVM to use... |
My MapSaver.kt looks like:package com.unciv.logic
import com.badlogic.gdx.Gdx
import com.badlogic.gdx.files.FileHandle
import com.unciv.logic.map.TileMap
import com.unciv.ui.saves.Gzip
object MapSaver {
fun json() = GameSaver.json()
private const val mapsFolder = "maps"
private const val saveZipped = false
private fun getMap(mapName:String) = Gdx.files.local("$mapsFolder/$mapName")
fun mapFromSavedString(mapString: String): TileMap {
val unzippedJson = try {
Gzip.unzip(mapString)
} catch (ex: Exception) {
mapString
}
return mapFromJson(unzippedJson)
}
fun mapToSavedString(tileMap: TileMap): String {
val mapJson = json().toJson(tileMap)
return if (saveZipped) Gzip.zip(mapJson) else mapJson
}
fun saveMap(mapName: String,tileMap: TileMap) {
getMap(mapName).writeString(mapToSavedString(tileMap), false)
}
fun loadMap(mapFile:FileHandle):TileMap {
return mapFromSavedString(mapFile.readString())
}
fun getMaps(): Array<FileHandle> = Gdx.files.local(mapsFolder).list()
private fun mapFromJson(json:String): TileMap = json().fromJson(TileMap::class.java, json)
} I used that to make the world setup easier and to more simply get the map into a notepad and back in. That Sorry, like that it's probably still incomplete - here's the patch I'm repeatedly using: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor nitpick that civ 5 also doesn't have working correctly:
Helicopter units specifically ignore zone of control, but the algorithm showing the tiles a helicopter unit can go does not. This can of course be implemented in a different PR, this one has been open for way to long already
Yup, to-do "Ignore zones of control" unique |
Exactly what I had in mind, and this should be very easy to implement. However, the mention of helicopters made me realize that my ZoC code does not consider free-moving air units at all. For efficiency, I assumed that non-naval means land, and non-land means naval. That means that air units currently exert ZoC only on land units and are affected only by ZoC of naval units, which is probably not right. For the base game, this only affects the Helicopter Gunship, but mods might very well add more free-moving air units. @xlenstra, @ravignir, or anyone else with the original game: could you check the following for me?
|
Another question: I could easily add an even more generic "Ignores ZoC of [unitFilter] units" unique, but that would impact performance more. Should I? |
My testing earlier revealed that Helicopter units were not affected by ZoC of land units, so I suppose it is the same for naval units and cities. I'm away from home this evening so I can't test the other cases or what happens when helicopter units are embarked. Also, helicopters are currently implemented as land units in unciv, as we only treat units that can move solely between cities (and carriers) as air units
I vote against this, checking for the existence of uniques and whether their parameters match a filter is something that has been actively removed from the movement functions for performance reasons |
This is not at all a given, since naval units and cities generally have a "stronger" ZoC than land units.
I see. From looking at the code, this also appears to mean that the "air units" unitFilter value won't match helicopters, which means that anti-air units don't deal increased damage against them. That seems to be another bug. |
Unfiltered unique can be cached easily. Filtered ones - doable limiting the filter type, e.g. parsing to HashSet... Would set a precedent. |
Bring back "domain" - which would be air/land/water (space? spelunking?), and use it only for these filters, while keeping UnitMovementType for the other code... ... ...? |
Based on testing done in a voice call with @avdstaaij, we have determined that, regarding ZoC, helicopters are basically land units that are themselves not affected by any ZoC effect |
Implemented the unique and gave it to the Helicopter Gunship.
I have added an "Ignores Zone of Control" unique and given it to the Helicopter Gunship. |
I have also executed a few more performance measurements. I measured performance in two ways:
Both measurements were quite inconsistent, so I performed each a few times and recorded my rough estimation of the average of the results. Results:
This PR is ready to merge now, if this performance is good enough. Otherwise, I might try a few more implementations at some point in the future. |
You should have measured with the save in #4840 ... 👍 |
This PR implements Zone of Control (mentioned in #663), as described by https://civilization.fandom.com/wiki/Zone_of_control_(Civ5).
The normal ZoC rule is quite simple, but the exception for units that can move after attacking required an additional boolean parameter that propagates through a bunch of the movement functions.