yairm210 / Unciv

Open-source Android/Desktop remake of Civ V
Mozilla Public License 2.0
8.5k stars 1.58k forks source link

Nuke base terrain percent failing #10249

Closed hackedpassword closed 12 months ago

hackedpassword commented 1 year ago

I'm testing WMD's, finding that applying percentages to TerrainFeature's works fine, but when applying percentages to base terrains, the result is 0% for any specified value.

    {
        "name": "Grassland",
        "type": "Land",
        "food": 2,
        "movementCost": 1,
        "RGB": [0, 163, 0],
        "uniques": [
//          "[98]% Chance to be destroyed by nukes",
        ],
    },

For now I have to comment this line out, to trigger default 50% chance, otherwise the base tile is immune to receiving fallout, and it appears improvements are then also.

In the Z2 mod currently, I have uncommented all nuke chances. Damage seems to be applied where it belongs but no fallout.

Including a map file shortly to demo. How: left side of the map under the bridge there's a nuclear sub carrying a nuke, power of 3. Should be positioned to test destroy anything.

hackedpassword commented 1 year ago

ICBM x4 city scenario.txt

Nuke scenario is setup. Lots of WIP don't mind that.

hackedpassword commented 1 year ago

Potentially offending code might be somewhere around here: https://github.com/yairm210/Unciv/blob/f812c828e5e1fe569644451f38d72a643e8d44c3/core/src/com/unciv/logic/battle/Nuke.kt#L257

SomeTroglodyte commented 1 year ago

5 lines up. if (chance <= 0f || (!isGroundZero && Random.Default.nextFloat() >= chance) continue I think. Idea is, skip fallout entirely if you see the Unique say 0 chance, but do the normal "random but ground zero gets 100%" when it's non-zero.

Good catch.

hackedpassword commented 1 year ago

The logic is starting to materialize, and it looks funky. I'll have to try specifying 0 as the chance and see how that reacts.

SomeTroglodyte commented 1 year ago

:see_no_evil: "to base terrains" - you can't destroy a base terrain - empty tiles are not allowed. You could define a mod with the only base terrain being "void" and have all actual land be terrain features, maybe. Then you could nuke away one of those and be left with void. ...But wait, can a feature change impassable to passable? Think not.

hackedpassword commented 1 year ago

But fallout still is, should be, applied as a TerrainFeature. This is normal behavior. Except the unique fizzles the chance of fallout to 0 for non-TerrainFeature's.

SomeTroglodyte commented 1 year ago

I see. Yes that logic doesn't take into account someone might add that unique to a base terrain. Doesn't make sense, and the result neither. Fallout is treated as dependent of the destroyable terrain, not independently. If you declare destroyability, you only get Fallout if the destroy happens, never Fallout on top of that feature. I think because Civ5 does so. I know I analyzed the DLL sources for that.

What you want is Fallout chance control for base terrains. Yup, maybe not a bad thing.

hackedpassword commented 1 year ago

Screenshot_20231006-102826_Unciv.jpg

Screenshot_20231006-102919_Unciv.jpg

Wait, how is it not common knowledge that base terrains, without improvements or TerrainFeature's, is not "destroyable", in the context of applying fallout? Did I miss something?

Take a look at these shots. Desert base terrain is destroyed. I get the base terrain is immutable being baked in, but the perception is that it's destroyed because the TerrainFeature is applied.

Aha, yes, I believe I've found where my perception deviates from the intended design. I looked back over G&K's use of chance of being destroyed by nukes and upon study, the use here is limited to TerrainFeature's that would conceptually have vertical occupancy. So it would appear the docs' Applicable to: Terrain isn't quite accurate.

Most definitely believe the docs represent how a modder should be able to control fallout probability via unique. Bug - > Feature Request, or, docs update. :)

SomeTroglodyte commented 1 year ago

Destroy can't make Grassland no longer be Grassland, but it can convert a Grassland+Jungle to Grassland+Fallout, just semantics what "destroyed" means. For me, definitely something different from "unusable due to additional properties".

Like a murderer and a lover understanding different things as "destroying someone"

Applicable to: Terrain isn't quite accurate

Again - semantics. Terrain and applicable as in technically allowed, and technical declarative validation cannot differentiate between BaseTerrain and TerrainFeature - they're wrapped in the same type of object. Yes, a little regrettable but nothing we can do with the current architecture, not in a clean and succinct way. Which conditionals work where is even less precise.

The docs you're referring to are actually machine generated, and as such can only express those rules the validator can check. We pass through some help strings AFAIK for parameter types but not individual Unique types - maybe that could be a simple way. But that data structure has extreme pressure to stay as slim as possible.

hackedpassword commented 1 year ago

convert a Grassland+Jungle to Grassland+Fallout

I'm kinda hung up on that nukes still apply fallout to naked base terrains 50%of the time by default.

different from "unusable due to additional properties"

You play Unciv; as a player: what are your expectations?

has extreme pressure

The architecture of Unciv with its 'potato' philosophy is highly commendable, a marvel of coding discipline.

So what do you think? Would control over the existing 50% base terrain destruction be a valid unique modifier?

SomeTroglodyte commented 1 year ago

Would control over the existing 50% base terrain destruction be a valid unique modifier

NO! It would be a nice new UniqueType!!! :zany_face:

hackedpassword commented 1 year ago

try specifying 0 as the chance

fyi did try, and the reaction is no fallout on base terrain, basically saying if specified regardless of value, you get none.

SomeTroglodyte commented 1 year ago

Anyway, this is a modding feature request - see pinned issue.

And in the context it would be much more important to un-hardcode the "Fallout" itself - which feature a nuke applies. like UniqueType.AppliesFallout("Spreads [terrainFeature] within its blast radius with [percentage] chance per tile", UniqueTarget.Unit) - how to express that best? How to document the percentage only applies to tiles without a destroyable TerrainFeature and not on ground zero either? A chance conditional already exists - but that would not make it clear implementation decides per tile.

hackedpassword commented 12 months ago

Allow percent chance of wiping TerrainFeature's and Improvements from a tile replaced by Fallout.

3242