Feature request: Enable OR when specifying multiple Tech Requirements

PLynx01 commented 1 month ago

Before creating

[X] This is NOT a gameplay feature from Civ VI, BNW, or outside - see Roadmap
[X] This is NOT a gameplay feature from Vanilla Civ V or from G&K - If so, it should be a comment in https://github.com/yairm210/Unciv/issues/4697

Problem Description

Currently, when modders are specifying multiple prerequisite Techs, all must be researched before the Technology can be researched. Civ 4 in particular has many Techs that have OR in their prerequisites, so it can't be implemented for now.

Desired Solution

I think that we can use nested lists. In that case, if at least one list of prerequisite techs is fulfilled, the technology becomes available to research, even if the other lists are not completed.

Example: In Civ 4 the early-game tech "Animal Husbandry" requires Hunting OR Agriculture https://civilization.fandom.com/wiki/AnimalHusbandry(Civ4)

The syntax would be like this:

"prerequisites": [
    ["Hunting"],
    ["Agriculture"]
]

Alternative Approaches

Alternatively we can transform the "prerequisite" type from List of Strings into an Object:

Something like this:

"prerequisites: {
    set1: ["Hunting"],
    set2: ["Agriculture"]
}

Additional Context

I posted this issue in order to make sure my PRs won't be rejected after posting them, so I am asking you for your opinion before I begin work on new feature.

SomeTroglodyte commented 1 month ago

Finally go for OR multiFilters, then it's already implemented via UniqueType.OnlyAvailable? I repeat myself: Complex nested multifilters with and or not logic are technically no problem, they're only a translation problem. I say *$§$& the translations, allow unreadable filters. It could then be the modders responsibility to make them readable by hiding the unique and providing a good comment unique instead.

But - I also got the feeling there was some work weeks or months ago that had a similar vein, only I can't find it via sources and blame... Ah, scan contributor list until one rings a bell did it: Compare closed pulls by dHannasch - that was tech prereqs too but for unit upgrades. IIRC that pulls chain felt incomplete at the time...

So, yes, good idea IMHO. But divergent solutions to similar usecases should be a no.

That said, my fist idea was to allow an anded list or ored lists under the prerequisites key without breaking backward compatibility by using custom json serialization. so A and B would be: "prerequisites":["A","B"] same as before, A or B would be: "prerequisites":[["A","B"]], A and (B or C) = "prerequisites":["A",["B","C"]], and (A and B) or C = "prerequisites":[["A","C"],["B","C"]]. In other words, a mix of classic and your first proposal. For translatability of the descriptions - same argument as above.

poc patch

```patch Index: android/assets/jsons/Civ V - Gods & Kings/Techs.json IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/android/assets/jsons/Civ V - Gods & Kings/Techs.json b/android/assets/jsons/Civ V - Gods & Kings/Techs.json --- a/android/assets/jsons/Civ V - Gods & Kings/Techs.json (revision f1c0288a43021be0a3b98be9ee00b4656aae9aac) +++ b/android/assets/jsons/Civ V - Gods & Kings/Techs.json (date 1716648008819) @@ -82,7 +82,7 @@ { "name": "The Wheel", "row": 7, - "prerequisites": ["Animal Husbandry", "Archery"], + "prerequisites": ["Animal Husbandry", ["Archery", "Pottery"]], "quote": "'Wisdom and virtue are like the two wheels of a cart.' - Japanese proverb" }, { Index: core/src/com/unciv/ui/objectdescriptions/TechnologyDescriptions.kt IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/core/src/com/unciv/ui/objectdescriptions/TechnologyDescriptions.kt b/core/src/com/unciv/ui/objectdescriptions/TechnologyDescriptions.kt --- a/core/src/com/unciv/ui/objectdescriptions/TechnologyDescriptions.kt (revision f1c0288a43021be0a3b98be9ee00b4656aae9aac) +++ b/core/src/com/unciv/ui/objectdescriptions/TechnologyDescriptions.kt (date 1715798801493) @@ -166,14 +166,17 @@ lineList += FormattedLine() lineList += FormattedLine("{Cost}: $cost${Fonts.science}") - if (prerequisites.isNotEmpty()) { + if (newPrerequisites.isNotEmpty()) { lineList += FormattedLine() - if (prerequisites.size == 1) - prerequisites.first().let { lineList += FormattedLine("Required tech: [$it]", link = "Technology/$it") } + if (newPrerequisites.size == 1 && newPrerequisites[0].size == 1) + newPrerequisites[0].first().let { lineList += FormattedLine("Required tech: [$it]", link = "Technology/$it") } else { lineList += FormattedLine("Requires all of the following:") - prerequisites.forEach { - lineList += FormattedLine(it, link = "Technology/$it") + for (set in newPrerequisites) { + if (set.size == 1) + lineList += FormattedLine(set.first(), link = "Technology/${set.first()}") + else + lineList += FormattedLine("At least one of: [${set.joinToString { it.tr() }}]", link = "Technology/${set.first()}") } } } Index: core/src/com/unciv/logic/civilization/managers/TechManager.kt IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/core/src/com/unciv/logic/civilization/managers/TechManager.kt b/core/src/com/unciv/logic/civilization/managers/TechManager.kt --- a/core/src/com/unciv/logic/civilization/managers/TechManager.kt (revision f1c0288a43021be0a3b98be9ee00b4656aae9aac) +++ b/core/src/com/unciv/logic/civilization/managers/TechManager.kt (date 1715798801453) @@ -169,7 +169,7 @@ if (isResearched(tech.name) && !tech.isContinuallyResearchable()) return false - return tech.prerequisites.all { isResearched(it) } + return tech.newPrerequisites.all { set -> set.any { isResearched(it) } } } //endregion Index: core/src/com/unciv/models/ruleset/tech/Technology.kt IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/core/src/com/unciv/models/ruleset/tech/Technology.kt b/core/src/com/unciv/models/ruleset/tech/Technology.kt --- a/core/src/com/unciv/models/ruleset/tech/Technology.kt (revision f1c0288a43021be0a3b98be9ee00b4656aae9aac) +++ b/core/src/com/unciv/models/ruleset/tech/Technology.kt (date 1716648149369) @@ -1,5 +1,8 @@ package com.unciv.models.ruleset.tech +import com.badlogic.gdx.utils.Json +import com.badlogic.gdx.utils.JsonValue +import com.badlogic.gdx.utils.SerializationException import com.unciv.Constants import com.unciv.logic.civilization.Civilization import com.unciv.models.ruleset.Ruleset @@ -8,11 +11,18 @@ import com.unciv.models.ruleset.unique.UniqueTarget import com.unciv.models.ruleset.unique.UniqueType import com.unciv.ui.objectdescriptions.TechnologyDescriptions +import kotlin.reflect.full.memberProperties -class Technology: RulesetObject() { +class Technology: RulesetObject(), Json.Serializable { var cost: Int = 0 + + @Transient + @Deprecated("Use newPrerequisites instead") var prerequisites = HashSet() + + val newPrerequisites = ArrayList>() + override fun getUniqueTarget() = UniqueTarget.Tech var column: TechColumn? = null // The column that this tech is in the tech tree @@ -53,4 +63,43 @@ && unique.conditionals[0].let { it.type == UniqueType.ConditionalTech && it.params[0] == name } fun uniqueIsNotRequirementForThisTech(unique: Unique): Boolean = !uniqueIsRequirementForThisTech(unique) + + //region Custom Json deserialization + override fun write(json: Json) { + throw UnsupportedOperationException("Serializing a Technology is not supported") + } + + /** + * Allow a mix of element types in the `prerequisites` json field - plain strings or arrays of strings + * - Map to newPrerequisites class field, which represents an `and`ed list of `or`ed items + * - After the parse, simplify into prerequisites class field for backward compatibility + * - This way, vanilla rulesets need no change, and mods can start to use `or` as needed + */ + override fun read(json: Json, jsonData: JsonValue) { + var child = jsonData.child + while(child != null) { + readField(json, child.name, child) + child = child.next + } + + @Suppress("DEPRECATION") + prerequisites.addAll(newPrerequisites.mapNotNull { it.firstOrNull() }) + } + private fun readField(json: Json, name: String, node: JsonValue) { + if (name == "prerequisites") readPrerequisites(json, node) + else json.readField(this, name, node) + } + private fun readPrerequisites(json: Json, node: JsonValue) { + if (!node.isArray) throw SerializationException("'prerequisites' is not an array") + val dummySet = HashSet() + var child = node.child + while(child != null) { + if (child.isArray) + newPrerequisites.add(json.readValue(dummySet::class.java, child)) + else + newPrerequisites.add(hashSetOf(child.asString())) + child = child.next + } + } + //endregion } ``` ... A lot of places to update code left, compiler flags them

yairm210 commented 1 month ago

This is something that pops up once in a while in different contexts. Solving it in one doesn't solve it in another. The real answer here is IMO an <or> conditional, which here would lead to only available <when [A] researched> <or> <when B researched>. Or / and logic acts as code, that is, "a or b and c or d" means "(a) or (b or c) or (d)" which may be unintuitive - but the opposite "a and b or c and d" would be unintuitive the other way round so there's no winning this one.

But this would require some work.

Changing fields for niche cases is rarely ever the answer. Custom json serialization is really the last thing we want, I really hope you're joking

yairm210 commented 1 month ago

HOWEVER - making this as a multifilter would certainly be easier. And if we want to say "damn the translations full speed ahead, let the nested multifilters flow" we can do that. If you're a mad enough modder to use nested boolean logic, then it's on you to debug it and explain it to your users, and good luck with that.

SomeTroglodyte commented 1 month ago

Custom json serialization is really the last thing we want, I really hope you're joking

Not really. With implements Serializable the json constructor registering a Serializer is obsolete, the pain less. Here, it's relatively cheap 100% backwards compatibility that doesn't even need future cleanup, can stay.

That said, I assumed the way to go will be existing Uniques anyway, which would make that json a moot point. But coding it for curiosity - that I coudn't resist. And it proved I finally got the hang of it and understood where the Gdx design choices felt so quirky.

an <or> conditional

I'd hesitate strongly before doing mandatory positional significance of conditionals. Hmmm.. Maybe if it allows 100% encapsulation inside one conditional processor class, that would be enough plusones to justify...

yairm210 / Unciv