From bcb26b6d2a9dfb92b1ac03216d029c1727a9001e Mon Sep 17 00:00:00 2001 From: SomeTroglodyte <63000004+SomeTroglodyte@users.noreply.github.com> Date: Thu, 3 Aug 2023 10:38:50 +0200 Subject: [PATCH] Treat remaining untyped Uniques in default rulesets (#9763) * Treat remaining untyped Uniques in default rulesets, make unit test catch them * Change untyped filtering Uniques check to Validation by inclusion in GlobalUniques instead of UniqueType.AircraftMarker * Wiki for untyped filtering Uniques * Re-include the "Who knows" of Future Tech on the Tech picker --- .../Civ V - Gods & Kings/GlobalUniques.json | 5 +- .../jsons/Civ V - Gods & Kings/Nations.json | 12 +-- .../jsons/Civ V - Gods & Kings/Techs.json | 7 +- .../jsons/Civ V - Vanilla/GlobalUniques.json | 5 +- .../assets/jsons/Civ V - Vanilla/Techs.json | 7 +- .../unciv/models/ruleset/RulesetValidator.kt | 101 +++++++++++------- .../com/unciv/models/ruleset/nation/Nation.kt | 13 ++- .../unciv/models/ruleset/unique/UniqueType.kt | 4 + .../TechnologyDescriptions.kt | 8 ++ docs/Modders/Mod-file-structure/1-Overview.md | 15 +++ .../5-Miscellaneous-JSON-files.md | 5 +- tests/src/com/unciv/testing/BasicTests.kt | 1 + 12 files changed, 121 insertions(+), 62 deletions(-) diff --git a/android/assets/jsons/Civ V - Gods & Kings/GlobalUniques.json b/android/assets/jsons/Civ V - Gods & Kings/GlobalUniques.json index 751b9a139c..1843839bc0 100644 --- a/android/assets/jsons/Civ V - Gods & Kings/GlobalUniques.json +++ b/android/assets/jsons/Civ V - Gods & Kings/GlobalUniques.json @@ -7,7 +7,10 @@ "[-33]% Strength ", "Cannot build [Settler] units ", "Rebel units may spawn ", - "[-1] Sight " + "[-1] Sight ", + + // Filtering uniques must be listed here to tell RulesetValidator they're OK despite untyped + "Aircraft" // TODO: Implement the uniques below // "[+20]% [Culture] [in all cities] ", diff --git a/android/assets/jsons/Civ V - Gods & Kings/Nations.json b/android/assets/jsons/Civ V - Gods & Kings/Nations.json index 9357fca7e5..41b188b598 100644 --- a/android/assets/jsons/Civ V - Gods & Kings/Nations.json +++ b/android/assets/jsons/Civ V - Gods & Kings/Nations.json @@ -68,7 +68,7 @@ "Paros","Elis","Syracuse","Herakleia","Gortyn","Chalkis","Pylos","Pella","Naxos","Sicyon", "Larissa","Apollonia","Messene","Orchomenos","Ambracia","Kos","Knidos","Amphipolis", "Patras","Lamia","Nafplion","Apolyton"], - "spyNames": ["Jason", "Helena", "Alexa", "Cletus", "Kassandra", "Andres", "Desdemona", "Anthea", "Aeneas", "Leander",] + "spyNames": ["Jason", "Helena", "Alexa", "Cletus", "Kassandra", "Andres", "Desdemona", "Anthea", "Aeneas", "Leander"] }, { "name": "China", @@ -246,7 +246,7 @@ "Satricum","Ardea","Ostia","Velitrae","Viroconium","Tarentum","Brundisium","Caesaraugusta","Caesarea","Palmyra", "Signia","Aquileia","Clusium","Sutrium","Cremona","Placentia","Hispalis","Artaxata","Aurelianorum","Nicopolis", "Agrippina","Verona","Corfinium","Treverii","Sirmium","Augustadorum","Curia","Interrama","Adria"], - "spyNames": ["Flavius", "Regula", "Servius", "Lucia", "Cornelius", "Licina", "Canus", "Serpens", "Agrippa", "Brutus",] + "spyNames": ["Flavius", "Regula", "Servius", "Lucia", "Cornelius", "Licina", "Canus", "Serpens", "Agrippa", "Brutus"] }, { "name": "Arabia", @@ -880,7 +880,7 @@ "Amstetten", "Bad Ischl", "Wolfsberg", "Kufstein", "Leoben", "Klosterneuburg", "Leonding", "Kapfenberg", "Hallein", "Bischofshofen", "Waidhofen", "Saalbach", "Lienz", "Steyr" ], - "spyNames": ["Ferdinand", "Johanna", "Franz-Josef", "Astrid", "Anna", "Hubert", "Alois", "Natter", "Georg", "Arnold",] + "spyNames": ["Ferdinand", "Johanna", "Franz-Josef", "Astrid", "Anna", "Hubert", "Alois", "Natter", "Georg", "Arnold"] }, { "name": "Carthage", @@ -903,13 +903,13 @@ "innerColor": [81, 0, 137], "favoredReligion": "Islam", "uniqueName": "Phoenician Heritage", - "uniques": ["Gain a free [Harbor] [in all coastal cities]","Land units may cross [Mountain] tiles after the first [Great General] is earned", - "Units ending their turn on [Mountain] tiles take [50] damage"], + "uniques": ["Gain a free [Harbor] [in all coastal cities]","Land units may cross [Mountain] tiles after the first [Great General] is earned"], "cities": ["Carthage","Utique","Hippo Regius","Gades","Saguntum","Carthago Nova","Panormus","Lilybaeum","Hadrumetum","Zama Regia", "Karalis","Malaca","Leptis Magna","Hippo Diarrhytus","Motya","Sulci","Leptis Parva","Tharros","Soluntum","Lixus", "Oea","Theveste","Ibossim","Thapsus","Aleria","Tingis","Abyla","Sabratha","Rusadir","Baecula", "Saldae"], - "spyNames": ["Hamilcar", "Mago", "Baalhaan", "Sophoniba", "Yzebel", "Similce", "Kandaulo", "Zinnridi", "Gisgo", "Fierelus"] + "spyNames": ["Hamilcar", "Mago", "Baalhaan", "Sophoniba", "Yzebel", "Similce", "Kandaulo", "Zinnridi", "Gisgo", "Fierelus"], + "civilopediaText": [{"text": "Units ending their turn on [Mountain] tiles take [50] damage"}] }, { "name": "Byzantium", diff --git a/android/assets/jsons/Civ V - Gods & Kings/Techs.json b/android/assets/jsons/Civ V - Gods & Kings/Techs.json index 07fee47ebf..a3ab86faaa 100644 --- a/android/assets/jsons/Civ V - Gods & Kings/Techs.json +++ b/android/assets/jsons/Civ V - Gods & Kings/Techs.json @@ -649,9 +649,10 @@ { "name": "Future Tech", "row": 5, - "prerequisites": ["Globalization","Particle Physics", "Nuclear Fusion", "Nanotechnology", "Stealth"], - "uniques": ["Who knows what the future holds?", "Can be continually researched"], - "quote": "'I think we agree, the past is over.' - George W. Bush" + "prerequisites": ["Globalization","Particle Physics","Nuclear Fusion","Nanotechnology","Stealth"], + "uniques": ["Can be continually researched"], + "quote": "'I think we agree, the past is over.' - George W. Bush", + "civilopediaText": [{"text": "Who knows what the future holds?"}] } ] } diff --git a/android/assets/jsons/Civ V - Vanilla/GlobalUniques.json b/android/assets/jsons/Civ V - Vanilla/GlobalUniques.json index 751b9a139c..1843839bc0 100644 --- a/android/assets/jsons/Civ V - Vanilla/GlobalUniques.json +++ b/android/assets/jsons/Civ V - Vanilla/GlobalUniques.json @@ -7,7 +7,10 @@ "[-33]% Strength ", "Cannot build [Settler] units ", "Rebel units may spawn ", - "[-1] Sight " + "[-1] Sight ", + + // Filtering uniques must be listed here to tell RulesetValidator they're OK despite untyped + "Aircraft" // TODO: Implement the uniques below // "[+20]% [Culture] [in all cities] ", diff --git a/android/assets/jsons/Civ V - Vanilla/Techs.json b/android/assets/jsons/Civ V - Vanilla/Techs.json index a9a0d285ec..7822b17b9d 100644 --- a/android/assets/jsons/Civ V - Vanilla/Techs.json +++ b/android/assets/jsons/Civ V - Vanilla/Techs.json @@ -615,9 +615,10 @@ { "name": "Future Tech", "row": 5, - "prerequisites": ["Globalization","Nuclear Fusion", "Nanotechnology"], - "uniques": ["Who knows what the future holds?", "Can be continually researched"], - "quote": "'I think we agree, the past is over.' - George W. Bush" + "prerequisites": ["Globalization","Nuclear Fusion","Nanotechnology"], + "uniques": ["Can be continually researched"], + "quote": "'I think we agree, the past is over.' - George W. Bush", + "civilopediaText": [{"text": "Who knows what the future holds?"}] } ] } diff --git a/core/src/com/unciv/models/ruleset/RulesetValidator.kt b/core/src/com/unciv/models/ruleset/RulesetValidator.kt index 3f879c39c6..6fd609788a 100644 --- a/core/src/com/unciv/models/ruleset/RulesetValidator.kt +++ b/core/src/com/unciv/models/ruleset/RulesetValidator.kt @@ -477,46 +477,7 @@ class RulesetValidator(val ruleset: Ruleset) { ): List { val prefix = (if (namedObj is IRulesetObject) "${namedObj.originRuleset}: " else "") + (if (namedObj == null) "The" else "${namedObj.name}'s") - if (unique.type == null) { - if (!tryFixUnknownUniques) return emptyList() - val similarUniques = UniqueType.values().filter { - getRelativeTextDistance( - it.placeholderText, - unique.placeholderText - ) <= RulesetCache.uniqueMisspellingThreshold - } - val equalUniques = - similarUniques.filter { it.placeholderText == unique.placeholderText } - return when { - // Malformed conditional - unique.text.count { it=='<' } != unique.text.count { it=='>' } ->listOf( - RulesetError("$prefix unique \"${unique.text}\" contains mismatched conditional braces!", - RulesetErrorSeverity.Warning)) - - // This should only ever happen if a bug is or has been introduced that prevents Unique.type from being set for a valid UniqueType, I think.\ - equalUniques.isNotEmpty() -> listOf(RulesetError( - "$prefix unique \"${unique.text}\" looks like it should be fine, but for some reason isn't recognized.", - RulesetErrorSeverity.OK)) - - similarUniques.isNotEmpty() -> { - val text = - "$prefix unique \"${unique.text}\" looks like it may be a misspelling of:\n" + - similarUniques.joinToString("\n") { uniqueType -> - var text = "\"${uniqueType.text}" - if (unique.conditionals.isNotEmpty()) - text += " " + unique.conditionals.joinToString(" ") { "<${it.text}>" } - text += "\"" - if (uniqueType.getDeprecationAnnotation() != null) text += " (Deprecated)" - return@joinToString text - }.prependIndent("\t") - listOf(RulesetError(text, RulesetErrorSeverity.OK)) - } - RulesetCache.modCheckerAllowUntypedUniques -> emptyList() - else -> listOf(RulesetError( - "$prefix unique \"${unique.text}\" not found in Unciv's unique types.", - RulesetErrorSeverity.OK)) - } - } + if (unique.type == null) return checkUntypedUnique(unique, tryFixUnknownUniques, prefix) val rulesetErrors = RulesetErrorList() @@ -581,6 +542,66 @@ class RulesetValidator(val ruleset: Ruleset) { return rulesetErrors } + + private fun checkUntypedUnique(unique: Unique, tryFixUnknownUniques: Boolean, prefix: String ): List { + // Malformed conditional is always bad + if (unique.text.count { it == '<' } != unique.text.count { it == '>' }) + return listOf(RulesetError( + "$prefix unique \"${unique.text}\" contains mismatched conditional braces!", + RulesetErrorSeverity.Warning)) + + // Support purely filtering Uniques without actual implementation + if (isFilteringUniqueAllowed(unique)) return emptyList() + if (tryFixUnknownUniques) { + val fixes = tryFixUnknownUnique(unique, prefix) + if (fixes.isNotEmpty()) return fixes + } + + if (RulesetCache.modCheckerAllowUntypedUniques) return emptyList() + + return listOf(RulesetError( + "$prefix unique \"${unique.text}\" not found in Unciv's unique types.", + RulesetErrorSeverity.WarningOptionsOnly)) + } + + private fun isFilteringUniqueAllowed(unique: Unique): Boolean { + // Isolate this decision, to allow easy change of approach + // This says: Must have no conditionals or parameters, and is contained in GlobalUniques + if (unique.conditionals.isNotEmpty() || unique.params.isNotEmpty()) return false + return unique.text in ruleset.globalUniques.uniqueMap + } + + private fun tryFixUnknownUnique(unique: Unique, prefix: String): List { + val similarUniques = UniqueType.values().filter { + getRelativeTextDistance( + it.placeholderText, + unique.placeholderText + ) <= RulesetCache.uniqueMisspellingThreshold + } + val equalUniques = + similarUniques.filter { it.placeholderText == unique.placeholderText } + return when { + // This should only ever happen if a bug is or has been introduced that prevents Unique.type from being set for a valid UniqueType, I think.\ + equalUniques.isNotEmpty() -> listOf(RulesetError( + "$prefix unique \"${unique.text}\" looks like it should be fine, but for some reason isn't recognized.", + RulesetErrorSeverity.OK)) + + similarUniques.isNotEmpty() -> { + val text = + "$prefix unique \"${unique.text}\" looks like it may be a misspelling of:\n" + + similarUniques.joinToString("\n") { uniqueType -> + var text = "\"${uniqueType.text}" + if (unique.conditionals.isNotEmpty()) + text += " " + unique.conditionals.joinToString(" ") { "<${it.text}>" } + text += "\"" + if (uniqueType.getDeprecationAnnotation() != null) text += " (Deprecated)" + return@joinToString text + }.prependIndent("\t") + listOf(RulesetError(text, RulesetErrorSeverity.OK)) + } + else -> emptyList() + } + } } diff --git a/core/src/com/unciv/models/ruleset/nation/Nation.kt b/core/src/com/unciv/models/ruleset/nation/Nation.kt index f2542c2781..6eeba3e906 100644 --- a/core/src/com/unciv/models/ruleset/nation/Nation.kt +++ b/core/src/com/unciv/models/ruleset/nation/Nation.kt @@ -9,7 +9,6 @@ import com.unciv.models.ruleset.unique.UniqueTarget import com.unciv.models.ruleset.unique.UniqueType import com.unciv.models.translations.squareBraceRegex import com.unciv.models.translations.tr -import com.unciv.ui.components.Fonts import com.unciv.ui.components.extensions.colorFromRGB import com.unciv.ui.screens.civilopediascreen.CivilopediaScreen.Companion.showReligionInCivilopedia import com.unciv.ui.screens.civilopediascreen.FormattedLine @@ -84,8 +83,8 @@ class Nation : RulesetObject() { innerColorObject = if (innerColor == null) Color.BLACK else colorFromRGB(innerColor!!) - forestsAndJunglesAreRoads = uniques.contains("All units move through Forest and Jungle Tiles in friendly territory as if they have roads. These tiles can be used to establish City Connections upon researching the Wheel.") - ignoreHillMovementCost = uniques.contains("Units ignore terrain costs when moving into any tile with Hills") + forestsAndJunglesAreRoads = uniqueMap.containsKey(UniqueType.ForestsAndJunglesAreRoads.placeholderText) + ignoreHillMovementCost = uniqueMap.containsKey(UniqueType.IgnoreHillMovementCost.placeholderText) } @@ -288,11 +287,11 @@ fun getRelativeLuminance(color: Color): Double { if (channel < 0.03928) channel / 12.92 else ((channel + 0.055) / 1.055).pow(2.4) - val R = getRelativeChannelLuminance(color.r) - val G = getRelativeChannelLuminance(color.g) - val B = getRelativeChannelLuminance(color.b) + val r = getRelativeChannelLuminance(color.r) + val g = getRelativeChannelLuminance(color.g) + val b = getRelativeChannelLuminance(color.b) - return 0.2126 * R + 0.7152 * G + 0.0722 * B + return 0.2126 * r + 0.7152 * g + 0.0722 * b } /** https://www.w3.org/TR/WCAG20/#contrast-ratiodef */ diff --git a/core/src/com/unciv/models/ruleset/unique/UniqueType.kt b/core/src/com/unciv/models/ruleset/unique/UniqueType.kt index f9f1f76acd..139baba17a 100644 --- a/core/src/com/unciv/models/ruleset/unique/UniqueType.kt +++ b/core/src/com/unciv/models/ruleset/unique/UniqueType.kt @@ -3,6 +3,7 @@ package com.unciv.models.ruleset.unique import com.unciv.Constants import com.unciv.models.ruleset.Ruleset import com.unciv.models.ruleset.RulesetErrorSeverity +import com.unciv.models.ruleset.RulesetValidator // Kdoc only import com.unciv.models.translations.getPlaceholderParameters import com.unciv.models.translations.getPlaceholderText @@ -454,6 +455,9 @@ enum class UniqueType(val text: String, vararg targets: UniqueTarget, val flags: CanEnterForeignTilesButLosesReligiousStrength("May enter foreign tiles without open borders, but loses [amount] religious strength each turn it ends there", UniqueTarget.Unit), ReducedDisembarkCost("[amount] Movement point cost to disembark", UniqueTarget.Global, UniqueTarget.Unit), ReducedEmbarkCost("[amount] Movement point cost to embark", UniqueTarget.Global, UniqueTarget.Unit), + // These affect movement as Nation uniques + ForestsAndJunglesAreRoads("All units move through Forest and Jungle Tiles in friendly territory as if they have roads. These tiles can be used to establish City Connections upon researching the Wheel.", UniqueTarget.Nation), + IgnoreHillMovementCost("Units ignore terrain costs when moving into any tile with Hills", UniqueTarget.Nation), CannotBeBarbarian("Never appears as a Barbarian unit", UniqueTarget.Unit, flags = UniqueFlag.setOfHiddenToUsers), diff --git a/core/src/com/unciv/ui/objectdescriptions/TechnologyDescriptions.kt b/core/src/com/unciv/ui/objectdescriptions/TechnologyDescriptions.kt index a1fc2dddbe..b857c3d3d1 100644 --- a/core/src/com/unciv/ui/objectdescriptions/TechnologyDescriptions.kt +++ b/core/src/com/unciv/ui/objectdescriptions/TechnologyDescriptions.kt @@ -30,6 +30,14 @@ object TechnologyDescriptions { fun getDescription(technology: Technology, viewingCiv: Civilization): String = technology.run { val ruleset = viewingCiv.gameInfo.ruleset val lineList = ArrayList() // more readable than StringBuilder, with same performance for our use-case + + for (pediaText in technology.civilopediaText) { + // This is explicitly to get the "Who knows what the future holds" of Future Tech back into + // the Tech Picker and Tech Researched Alert display, without making it an untyped Unique. + // May need tuning for mods, in vanilla there is just the one case. + if (pediaText.text.isEmpty() || pediaText.header != 0) continue + lineList += pediaText.text + } for (unique in uniques) lineList += unique lineList.addAll( diff --git a/docs/Modders/Mod-file-structure/1-Overview.md b/docs/Modders/Mod-file-structure/1-Overview.md index 3dbe3b8702..05eee5d085 100644 --- a/docs/Modders/Mod-file-structure/1-Overview.md +++ b/docs/Modders/Mod-file-structure/1-Overview.md @@ -82,6 +82,21 @@ The keys in this example are "science" and "culture", and both have the value "5 In some sense you can see from these types that JSON files themselves are actually a list of objects, each describing a single building, unit or something else. +## Uniques + +"Uniques" are a label used by Unciv for extensible and customizable effects. Nearly every "ruleset object" allows a set of them, as a List with the name "uniques". + +Every Unique follows a general structure: `Unique type defining name [placeholder] more name [another placeholder] ...` +The entire string, excluding all `<>`-delimited conditionals or triggers with their separating blanks, and excluding the placeholders but not their `[]` delimiters, are used to look up the Unique's implementation. +The content of the optional `[placeholder]`s are implementation-dependant, they are parameters modifying the effect, and described in [Unique parameters](../Unique-parameters.md). +All ``s are optional (but if they are used the spaces separating them are mandatory), and each in turn follows the Unique structure rules for the part between the `<>` angled brackets, including possible placeholders, but not nested conditionals. + +Example: `"uniques":["[+1 Gold] "]` on a building - does almost the same thing as the `"gold":1` attribute does, except it only applies when the city has a garrison. In this example, `[]` and `with a garrison` are the keys Unciv uses to look up two Uniques, an effect (of type `Stats`) and a condition (of type `ConditionalWhenGarrisoned`). + +All Unique "types" that have an implementation in Unciv are automatically documented in [uniques](../uniques.md). Note that file is entirely machine-generated from source code structures. Also kindly note the separate sections for [conditionals](../uniques.md#conditional-uniques) and [trigger conditions](../uniques.md#triggercondition-uniques). +Uniques that do not correspond to any of those entries (verbatim including upper/lower case!) are called "untyped", will have no _direct_ effect, and may result in the "Ruleset Validator" showing warnings (see the Options Tab "Locate mod errors", it also runs when starting new games). +A legitimate use of "untyped" Uniques is their use as markers that can be recognized elsewhere in filters (example: "Aircraft" in the vanilla rulesets used as [Unit filter](../Unique-parameters.md#baseunitfilter)). To allow "Ruleset Validator" to warn about mistakes leading to untyped uniques, but still allow the "filtering Unique" use, those should be "declared" by including each in [GlobalUniques](5-Miscellaneous-JSON-files.md#globaluniquesjson), too. + ## Information on JSON files used in the game Many parts of Unciv are moddable, and for each there is a separate json file. There is a json file for buildings, for units, for promotions units can have, for technologies, etc. The different new buildings or units you define can also have lots of different attributes, though not all are required. Below are tables documenting all the different attributes everything can have. Only the attributes which are noted to be 'required' must be provided. All others have a default value that will be used when it is omitted. diff --git a/docs/Modders/Mod-file-structure/5-Miscellaneous-JSON-files.md b/docs/Modders/Mod-file-structure/5-Miscellaneous-JSON-files.md index b6b79f159c..8f70475c3b 100644 --- a/docs/Modders/Mod-file-structure/5-Miscellaneous-JSON-files.md +++ b/docs/Modders/Mod-file-structure/5-Miscellaneous-JSON-files.md @@ -205,11 +205,14 @@ With `civModifier` being the multiplicative aggregate of ["\[relativeAmount\]% G ## GlobalUniques.json +[link to original](https://github.com/yairm210/Unciv/tree/master/android/assets/jsons/GlobalUniques.json) + Defines uniques that apply globally. e.g. Vanilla rulesets define the effects of Unhappiness here. Only the `uniques` field is used, but a name must still be set (the Ruleset validator might display it). When extension rulesets define GlobalUniques, all uniques are merged. At the moment there is no way to change/remove uniques set by a base mod. -[link to original](https://github.com/yairm210/Unciv/tree/master/android/assets/jsons/GlobalUniques.json) +Note: Mods can use "arbitrary" Uniques as purely filtering uniques. They are not "Typed" by Unciv code and thus have no actual effect implementation - except by being filterable elsewhere. +In the near future, the ruleset validator will show warnings for all these, unless they are also included here, as validation that they are intentional (and - they **must** have **no** placeholders or conditionals). ## Tutorials.json diff --git a/tests/src/com/unciv/testing/BasicTests.kt b/tests/src/com/unciv/testing/BasicTests.kt index bed615928d..d09e1cc9e8 100644 --- a/tests/src/com/unciv/testing/BasicTests.kt +++ b/tests/src/com/unciv/testing/BasicTests.kt @@ -91,6 +91,7 @@ class BasicTests { @Test fun baseRulesetHasNoBugs() { + RulesetCache.modCheckerAllowUntypedUniques = false for (baseRuleset in BaseRuleset.values()) { val ruleset = RulesetCache[baseRuleset.fullName]!! val modCheck = ruleset.checkModLinks()