yairm210 / Unciv

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

Feature request: Allow natural wonders to not change the terrain. #11689

Open denismattos opened 1 month ago

denismattos commented 1 month ago

Before creating

Problem Description

Unciv expects all natural wonders in Terrains.json to contain a turnsInto attribute Since 4.8.15, when the ruleset validator became more stringent, the absence of the attribute or a value throws a red error.

I believe Unciv should allow a natural wonder to not change the terrain where it's placed.

Related Issue Links

No response

Desired Solution

The simplest solution, I believe, would be for Unciv to allow turnsInto to accept either a null string or a new string (maybe nochange) as an instruction to not change the terrain.

Alternative Approaches

I'd really like to try to implement this myself (I believe that's the reason open source code is great), if I knew enough JS, but, alas, this is not the case, ATM. If the devs haven't had the time to implement this when I manage to study enough JS, I may try it myself and push.

Additional Context

I ran into this issue when updating an old mod. The original modder didn't provide a value for turnsInto in a natural wonder. It may have been a mistake or a choice, but, regardless, he/she most likely didn't even consider this, because the check was just skipped, back then. I can't test this on current Unciv versions, but, IIRC, this ended up causing graphical errors, so I understand the importance of the ruleset validation.

Nevertheless, I believe this option to not change the terrain should be available. Sometimes, the kind of terrain where a natural wonder occurs IRL isn't easily classifiable. I'd argue the very fact that occursOn allows for a list of strings demonstrates the devs understood this, too. The wonder's placement in world generation is constrained by the values in occursOn, but, otherwise, random. I would argue it's only logical for the game to allow the same randomness to also determine the base terrain.

SomeTroglodyte commented 1 month ago

study enough JS

If you mean JS = JavaScript then - there's none in here. Not. a. single. line. Which is goooood.

Other than that, seems easy and why-not to me. Modder would have to do much more work in the Tileset part, but - hey - if they go down that route they should know how to handle TileSetConfigs. Syntax: leave attribute out. Effect: BaseTerrain stays put, Validator accepts... Can't see any untoward consequences off the top of my head.

SomeTroglodyte commented 1 month ago

Not. a. single. line. Which is goooood.

Oops, forgot one or three files...

SomeTroglodyte commented 1 month ago

Oh my, that was me in #10355 that made it mandatory, everything else is already set for it being optional :see_no_evil:

SomeTroglodyte commented 1 month ago

... not quite everything ....

SomeTroglodyte commented 1 month ago

Mod excerpt:

    {
        "name": "Aggtelek",
        "type": "NaturalWonder",
        "occursOn": ["Hill","Mountain"],
        "uniques": [
            "Must be adjacent to [5] to [6] [Land] tiles",
            "Must be adjacent to [2] to [5] [Hill] tiles",
            "Must be adjacent to [0] [Desert] tiles",
            "Must be adjacent to [0] [Tundra] tiles",
            "Must be adjacent to [0] [Snow] tiles",
            "Neighboring tiles except [Water] will convert to [Hill] <with [50]% chance>",
            "Neighboring tiles except [Water] will convert to [Forest] <with [50]% chance>",
            "Neighboring tiles except [Water] will convert to [Mountain] <with [20]% chance>",
            (...)

Result: image

But I'm still not 100% sure about some of the code behind that.

denismattos commented 1 month ago

If you mean JS = JavaScript then - there's none in here. Not. a. single. line. Which is goooood.

My bad. I'm new to programming and thought the usage of .json implied JS. Your comment prompted me to look it up and I learned it's an agnostic format and the game's actually mostly written in Kotlin.

denismattos commented 1 month ago

Other than that, seems easy and why-not to me. Modder would have to do much more work in the Tileset part, but - hey - if they go down that route they should know how to handle TileSetConfigs. Syntax: leave attribute out. Effect: BaseTerrain stays put, Validator accepts... Can't see any untoward consequences off the top of my head.

Glad to read that. 🙂 I don't presume to understand how easily something could be implemented, but it seems my intuition was correct.

Sad part is: I'm not sure when – if ever – I'm going to be able to study Kotlin (I'm a beginner and I'm already too old and out-of-time to be diverging my efforts away from what I need to study.), so, apparently, I won't be able to collaborate, after all. 😕

denismattos commented 1 month ago

Oh my, that was me in #10355 that made it mandatory, everything else is already set for it being optional 🙈

I thought I'd read your name before, somewhere. 😄

Thanks for your attention and – in advance, to you and/or other devs – for fixing this, whenever it's possible, in the future. 🙏🏻 I've already implemented a provisory solution, in the meantime.

SomeTroglodyte commented 1 month ago

because the game changed

No it didn't. It just went from not noticing game-breaking and potentially crashing mistakes until too late - to detecting them.

denismattos commented 1 month ago

went from not noticing game-breaking and potentially crashing mistakes until too late - to detecting them

Yes, I read the release notes. I even said above that I understand the importance of the ruleset validation.

I'd still argue that's a change in the game: implementing a stricter check for an attribute.

But I'll amend the README to be more clear.

SomeTroglodyte commented 1 month ago

Always read anything I write suspecting I'm not entirely serious. None of this really affects 42, so it's all good and to be taken lightly. "not a single line of jabbaskrip" was meant as a little joke for instance.

So - don't let me discourage you. kotlin is easy - low entry threshold as in, writing correctly working (but klunky and inefficient to the eyes of the expert) code is easy. It's also easy to write elegant and efficient code that a beginner can't possibly understand. Finding the middle ground is what's hardest about it... And in these respects it's waaaaaay better than JS. But then it's a compiled language, and not a kludge tweaked and tweaked again...

denismattos commented 1 month ago

But then it's a compiled language, and not a kludge tweaked and tweaked again...

I'm a beginner and even I know how much of a clusterfuck JS is, today. 😅