Updated <in [tileFilter] tiles> so that you can settle on water/mountain tiles

[Emandac]

Updated <in [tileFilter] tiles> so that the player/ai can settle on water/mountain tiles.

[yairm210]

Did you find out what was causing the crashes?

[Emandac]

Yes. The crash was cause by not checking if the settler had an unique conditional before adding addedDistanceBeweenContinents to minimalCityDistanceOnDifferentContinents.

[yairm210]

I see
So we need to unify both automation and this function to use the same calculation
That's a prerequisite for any changes

[Emandac]

I see So we need to unify both automation and this function to use the same calculation That's a prerequisite for any changes

How should I go about doing this ?

[yairm210]

Find the code in settler automation that determines locations we can settle
find the code in unit action that determines locations we can settle
Ensure that they both use the same function

[Emandac]

Do I need to change anything else to get this PR merged ?

[RobLoach]

Shouldn't it be checking the param of ConditionalInTiles here, there than down below with canSettleInTileWithUnique?

[RobLoach]

if (unique.hasModifier(UniqueType.ConditionalInTiles)) {
  // TODO: Check the condition of params[0]
}
else if (!tile.isLand || tile.isImpassible())) {
  return false
}

[RobLoach]

In addition, since the boolean getOwner() checks following are less-expensive, may be best to put that above this one.

[SomeTroglodyte]

none { - interestingly, https://kotlinlang.org/docs/coding-conventions.html#horizontal-whitespace shows the rule for lambdas, but not other blocks or fun/if/for... bodies...

[SomeTroglodyte]

Just came across...

Unciv/core/src/com/unciv/models/ruleset/unique/UniqueTriggerActivation.kt

Line 1191 in 018c756

if (tile.isCityCenter() && terrain.type != TerrainType.Land) return null

(OneTimeChangeTerrain) ... might conflict.

Sounds like some

    @delegate:Transient
    val isRulesetAllowsSettlingWater by lazy {
        ruleset.units.values.asSequence()
            .flatMap { unit ->
                unit.uniqueObjects.asSequence()
                    .filter { it.type == UniqueType.FoundCity || it.type == UniqueType.FoundPuppetCity  }
                    .flatMap { unique -> unique.modifiers }
            }
            .filter { it.type == UniqueType.ConditionalInTiles }
            .map { it.params[0] }
            .any {
                it == "Water" ||
                ruleset.terrains[it]?.type == TerrainType.Water
            }
    }

might be needed in GameInfo for use in such "land-locked" code. Though interestingly, I can't find more with global searches - except edge cases such as - retreat from land to neighboring sea-city will be forbidden, or StartNormalizer.

[Emandac]

Just came across...

Unciv/core/src/com/unciv/models/ruleset/unique/UniqueTriggerActivation.kt

Line 1191 in 018c756

if (tile.isCityCenter() && terrain.type != TerrainType.Land) return null

(OneTimeChangeTerrain) ... might conflict.

Sounds like some

    @delegate:Transient
    val isRulesetAllowsSettlingWater by lazy {
        ruleset.units.values.asSequence()
            .flatMap { unit ->
                unit.uniqueObjects.asSequence()
                    .filter { it.type == UniqueType.FoundCity || it.type == UniqueType.FoundPuppetCity  }
                    .flatMap { unique -> unique.modifiers }
            }
            .filter { it.type == UniqueType.ConditionalInTiles }
            .map { it.params[0] }
            .any {
                it == "Water" ||
                ruleset.terrains[it]?.type == TerrainType.Water
            }
    }

might be needed in GameInfo for use in such "land-locked" code. Though interestingly, I can't find more with global searches - except edge cases such as - retreat from land to neighboring sea-city will be forbidden, or StartNormalizer.

Should I put this into it's own PR or in this one ?

[Emandac]

So like this if (tile.isCityCenter() && !tile.tileMap.gameInfo.isRulesetAllowsSettlingWater) return null ?

[Emandac]

Done

[RobLoach]

While I haven't been following this closely, it looks like it checks the first conditional in tiles. I guess this assumes you wouldn't be adding multiple of these on one line.

[Emandac]

I tested something like this: Founds a new city <in [Coast] tiles> <in [Atoll] tiles> and it did work.

You can still add more conditional in tiles, but the first one must be the one the player/ai must settle on.

[RobLoach]

This also just grabs the first one it finds. What if you have multiple of them, and a subsequent one applies, while the first doesn't?

[Emandac]

I when I added puppet city unique, I made so you can't have both at the same time.

[Emandac]

Hmm, I never thought about that 😅

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[github-actions]

Conflicts have been resolved.

[Emandac]

I will be unable to continue working on the PR