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

Zone of Control #4085

Merged
merged 7 commits into from Aug 15, 2021
Merged

Zone of Control #4085

merged 7 commits into from Aug 15, 2021

Conversation

avdstaaij
Copy link
Contributor

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.

Units that can move after attacking are not affected by zone of control
if they move because of defeating a unit.
@avdstaaij avdstaaij changed the title Zone of control Zone of Control Jun 9, 2021
@r3versi
Copy link
Contributor

r3versi commented Jun 9, 2021

Ciao! I found this very detailed post about zones of control.

In particular this list of special cases could be handy:

Cities

  • Cities exert a ZOC too, even if there is no garrison. A city ZOC applies to naval and to land units. (see Fig. 6)
  • Combat units also exert a ZOC on enemy units stationed in a city. (see Fig. 7)

Embarked Units

  • Embarked combat units do NOT exert a ZOC. (see Fig. 8)
  • Land and naval units exert a ZOC on embarked units. (see Fig. 11)

Naval Units vs Land Units

  • Naval units exert a ZOC on enemy naval units. (see Fig. 1b)
  • Naval units also exert a ZOC on enemy land units. (see Fig. 9)
  • Land units do NOT exert a ZOC on enemy naval units. (see Fig. 10)

Units able to move after attacking

  • A unit that can move after attacking (e.g. a mounted or an armored unit), can kill an enemy within another enemy's ZOC and still continue moving. (see Fig. 11b)

Miscellaneous

  • Only combat units exert a ZOC: non-combat units do NOT. But non-combat units must respect the ZOC of enemy units.
  • The ZOC rule does not apply at all to air units

@avdstaaij
Copy link
Contributor Author

Thank you for the input! I'll mark this PR as a draft until I have implemented the additional special cases.

  • Combat units also exert a ZOC on enemy units stationed in a city. (see Fig. 7)

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.

@avdstaaij avdstaaij marked this pull request as draft June 10, 2021 01:17
@yairm210
Copy link
Owner

Oh My God
What a sad mechanic, to need such a decision tree.

@avdstaaij
Copy link
Contributor Author

All special cases have been implemented, so this PR is now ready.

@avdstaaij avdstaaij marked this pull request as ready for review June 13, 2021 02:44
@yairm210
Copy link
Owner

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...

@avdstaaij
Copy link
Contributor Author

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 getMovementCostBetweenAdjacentTiles? I must admit that I don't really know where to start with that, as I've never done any sort of profiling in Android Studio. Also, you mentioned running the function multiple times and checking the timing, but its performance is heavily dependent on the map situation. Would it not be more informative to simulate an entire game and check the total time spent in the function? Or should I perhaps only profile it in a situation with a zone of control?

@yairm210
Copy link
Owner

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.
I can probably send you a file like that within the next couple of days

@avdstaaij
Copy link
Contributor Author

In that case, I'll wait for you to provide the file.

@yairm210
Copy link
Owner

The_Ottomans_-_401_turns_Lag.txt
Sorry for the wait, forgot that this was waiting on me...
Uploaded as txt because Github is picky about file extension names.

@avdstaaij
Copy link
Contributor Author

No worries. I'll take a look at this again later, if/when I get back to developing.

@xlenstra xlenstra mentioned this pull request Jul 31, 2021
80 tasks
@avdstaaij avdstaaij marked this pull request as draft August 1, 2021 00:36
@ajustsomebody
Copy link
Contributor

is this outdated by now?

@avdstaaij
Copy link
Contributor Author

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:

@SomeTroglodyte
Copy link
Collaborator

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.

@SomeTroglodyte
Copy link
Collaborator

SomeTroglodyte commented Aug 11, 2021

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 getMovementCostBetweenAdjacentTiles which I guessed to be the most critical part. The giant map I set up is meant to be able to support more complex edge case tests, it looks like this (ignore the cattle, I did not place them, that's Unciv insubordination on loading the map into an actual game):

Screenshot

image

Next:

  • Import @avdstaaij 's branch, merge forward, patch in these tests...
  • Debug canReachInCurrentTurn
  • Check why getDistanceToTilesWithinTurn says (1,1)->(2,1)->(1,0)->(0,1) is allowed in 3 moves
  • Implement Yair's alternate and run tests
  • Do a unit test comparing the implementation of Yair's design with the neighbors intersection
  • Redo baseline

Current measurement: measureZocMovementCostPerformance took 8221527µs and 240124kB -> roughly 5 times baseline.
Current status: Succeeds all tests but one - using canReachInCurrentTurn to check the yellow arrow of the linked forum post's first screenshot. Could be some transients not correctly initialized...

@SomeTroglodyte
Copy link
Collaborator

So the pathing thinks this is allowed and a unit with 3mp can do that in one turn, with this ZOC code in place:
image
In the linked forum post, it seems Fig. 4 confirms this path is allowed, so my test saying a unit needs 4 MP to reach (0,-1) in this case and 3MP is not enough is wrong - 3 is sufficient, 2 is not. @ravignir , @xlenstra , anybody already subscribed - can you confirm Civ5 works that way?

Anybody help setting up more complicated tests?

@avdstaaij
Copy link
Contributor Author

avdstaaij commented Aug 11, 2021

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 isMovementAffectedByZoneOfControl. You could use that as a basis for the unit tests.

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.

@SomeTroglodyte
Copy link
Collaborator

An implementation of essentially Yair's idea gave:
measureZocMovementCostPerformance took 7866932µs and 195339kB
Old baseline was:
measureZocMovementCostPerformance took 8221527µs and 240124kB

  • Remember, the new code is not able to deal with world wrap so it must get slower, but also the old baseline was inexact as world setup was incomplete and might get slower, too. But intersecting neighbors is worldwrap-OK. I think.
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()))
                )
            )
        }
        
    }

@SomeTroglodyte
Copy link
Collaborator

Thanks for taking a look at this PR so I don't have to! :)

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.

You could use that as a basis for the unit tests.

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...

In fact, even 2.1 MP would suffice.

Yes! Good one. And yes - testZocCanReach() passes with goodUnit.currentMovement = 2.1f. Stays. But I haven't yet double-cross-checked my commonAdjacentTiles so - grain of salt.

@avdstaaij
Copy link
Contributor Author

(...) seemed a pity it slept its rose sleep down at the bottom of the open PR's.

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.

Remember, the new code is not able to deal with world wrap (...)

Yeah, that was something I was afraid of.
It might also be a good idea to throw an exception if commonAdjacentTiles is called with non-neighbors, like TileInfo.isConnectedByRiver does, as long as it doesn't impact performance too much.

@SomeTroglodyte
Copy link
Collaborator

SomeTroglodyte commented Aug 11, 2021

  • Correctness of commonAdjacentTiles proven by new unit test
  • Baseline repeated with current unit test code which now includes the exploredTiles setup
  • Teach worldwrap to commonAdjacentTiles
  • Compare measurements with wwrap capability

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:

  • Correctness of the implementation so far proven
  • Baseline - no ZOC - hammering UnitMovementAlgorithms.getMovementCostBetweenAdjacentTiles vs ZOC(neighbors) vs ZOC(commonAdjacentTiles):
    • 1.7s / 8.2s / 7.9s

@ravignir
Copy link
Contributor

faithfully!

Not so faithfully, cattle is missing xD

@SomeTroglodyte
Copy link
Collaborator

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.

@ravignir
Copy link
Contributor

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.

@SomeTroglodyte
Copy link
Collaborator

The 👽 insubordination is in functionaddConsolationPrize by... @SimonCeder , #4588 ! Woohoo, aliens, we come in pieces! Good idea that, but maybe we should pass that 'loaded from editor' flag down and deactivate the feature for those - sometime.

Added the knowledge gained from SomeTroglodyte's tests.
@avdstaaij
Copy link
Contributor Author

Looks like the current conclusion is that my initial implementation is surprisingly the most efficient one, but still increases the time spent in getMovementCostBetweenAdjacentTiles by a highly significant factor of 4.8 in @SomeTroglodyte's tests. @yairm210, it is of course up to you to decide whether this is acceptable. Like @SomeTroglodyte stated, ZoC is a core mechanic of Civ V, so it would be strange to leave it out in Unciv.

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 MapSaver.mapFromSavedString is missing - both on this branch and on master. Could you add its implementation as well so that I can use your code for performance comparisons?

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.

@avdstaaij
Copy link
Contributor Author

Unrelated: I only just noticed that the hexagon layouts of the original Civ V and Unciv are different. That's interesting.

@avdstaaij avdstaaij marked this pull request as ready for review August 13, 2021 01:16
@avdstaaij
Copy link
Contributor Author

I'm not sure how to fix the failing check, so I'd appreciate it if someone else took a look at it.

@SomeTroglodyte
Copy link
Collaborator

Android Gradle plugin requires Java 11 to run. You are currently using Java 1.8.

It's trying to run Gradle 7.02 with the same old JVM which won't work...
https://github.com/yairm210/Unciv/blob/master/gradle/wrapper/gradle-wrapper.properties
2161790#diff-40640fe1078ece83d7ea8fb67daacd77923a86d13447baf9769660b3b46f2ece

No idea where to tell travis which JVM to use...

@SomeTroglodyte
Copy link
Collaborator

SomeTroglodyte commented Aug 13, 2021

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 private const val saveZipped is kind of a preparation to make that some toggle-able option.

Sorry, like that it's probably still incomplete - here's the patch I'm repeatedly using:
MapSaver_unzipped_capability.patch.zip

Copy link
Collaborator

@xlenstra xlenstra left a 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

@SomeTroglodyte
Copy link
Collaborator

Yup, to-do "Ignore zones of control" unique

@avdstaaij
Copy link
Contributor Author

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?

  • Do helicopters exert ZoC on air units?
  • Do helicopters exert ZoC on naval units?
  • Do helicopters exert ZoC on land units?
  • Are helicopters affected by ZoC of naval units?
  • Are helicopters affected by ZoC of land units?
  • Do city centers exert ZoC on helicopters?
  • All of the above, but for embarked helicopters

@avdstaaij avdstaaij marked this pull request as draft August 13, 2021 16:32
@avdstaaij
Copy link
Contributor Author

Another question: I could easily add an even more generic "Ignores ZoC of [unitFilter] units" unique, but that would impact performance more. Should I?

@xlenstra
Copy link
Collaborator

xlenstra commented Aug 13, 2021

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?

  • Do helicopters exert ZoC on air units?
  • Do helicopters exert ZoC on naval units?
  • Do helicopters exert ZoC on land units?
  • Are helicopters affected by ZoC of naval units?
  • Are helicopters affected by ZoC of land units?
  • Do city centers exert ZoC on helicopters?

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

Should I implement "not affected by ZoC of [unitFilter] 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

@avdstaaij
Copy link
Contributor Author

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.

This is not at all a given, since naval units and cities generally have a "stronger" ZoC than land units.

Also, helicopters are currently implemented as land units in unciv (...)

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.

@SomeTroglodyte
Copy link
Collaborator

Unfiltered unique can be cached easily. Filtered ones - doable limiting the filter type, e.g. parsing to HashSet... Would set a precedent.

@SomeTroglodyte
Copy link
Collaborator

"air units" unitFilter

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... ... ...?

@xlenstra
Copy link
Collaborator

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?

* Do helicopters exert ZoC on air units?

* Do helicopters exert ZoC on naval units?

* Do helicopters exert ZoC on land units?

* Are helicopters affected by ZoC of naval units?

* Are helicopters affected by ZoC of land units?

* Do city centers exert ZoC on helicopters?

* All of the above, but for embarked helicopters

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.
@avdstaaij
Copy link
Contributor Author

I have added an "Ignores Zone of Control" unique and given it to the Helicopter Gunship.

@avdstaaij
Copy link
Contributor Author

avdstaaij commented Aug 15, 2021

I have also executed a few more performance measurements.

I measured performance in two ways:

  1. The time needed to execute getMovementCostBetweenAdjacentTiles 1 000 000 000 times in @SomeTroglodyte's testing environment (Zone of Control #4085 (comment)). In this environment (if I understand it correctly), there is no ZoC effect in 3/5 of the function calls, and there is a ZoC effect from an enemy land unit in the remaining 2/5 calls (this affects the speed of isMovementAffectedByZoneOfControl: it returns early if it determines that there is a ZoC effect). I expect that in a real game, less movements will be affected by ZoC, so isMovementAffectedByZoneOfControl will be able to return early less often, so the performance will be worse than suggested by this measurement.

  2. The time needed to execute GameInfo.nextTurn (which executes turns up to the next player turn, without drawing) 10 times, starting from the save posted in Zone of Control #4085 (comment).

Both measurements were quite inconsistent, so I performed each a few times and recorded my rough estimation of the average of the results.
Measurement 1 had a mean deviation of about 0.1 seconds. I measured at 0.1 second precision.
Measurement 2 had a mean deviation of about .5 seconds. I measured at 1 second precision.

Results:

Implementation Measurement 1 (normalized) Measurement 2 (normalized)
No ZoC 1.00 1.00
Current implementation 4.67 1.14
"Ignore ZoC" unique check first 4.73 1.14

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.

@avdstaaij avdstaaij marked this pull request as ready for review August 15, 2021 18:04
@yairm210 yairm210 merged commit 201648a into yairm210:master Aug 15, 2021
@SomeTroglodyte
Copy link
Collaborator

You should have measured with the save in #4840 ... 👍

@avdstaaij avdstaaij deleted the zone-of-control branch October 10, 2021 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants