From 66b0ddb25a1f9638816345767c48d91b694062ef Mon Sep 17 00:00:00 2001 From: yairm210 Date: Wed, 22 Sep 2021 21:28:19 +0300 Subject: [PATCH] Mod error detection improvements! Separated Warning vs Error, show "options only" warning in options only, color warnings by severity --- core/src/com/unciv/logic/GameInfo.kt | 4 +- core/src/com/unciv/models/ruleset/Ruleset.kt | 90 ++++++++++--------- .../ui/newgamescreen/ModCheckboxTable.kt | 11 +-- .../ui/worldscreen/mainmenu/OptionsPopup.kt | 27 +++--- 4 files changed, 68 insertions(+), 64 deletions(-) diff --git a/core/src/com/unciv/logic/GameInfo.kt b/core/src/com/unciv/logic/GameInfo.kt index aa225f3162..0915ebe717 100644 --- a/core/src/com/unciv/logic/GameInfo.kt +++ b/core/src/com/unciv/logic/GameInfo.kt @@ -135,9 +135,7 @@ class GameInfo { fun isReligionEnabled(): Boolean { if (ruleSet.eras[gameParameters.startingEra]!!.hasUnique("Starting in this era disables religion") || ruleSet.modOptions.uniques.contains(ModOptionsConstants.disableReligion) - ) { - return false - } + ) return false return gameParameters.religionEnabled } diff --git a/core/src/com/unciv/models/ruleset/Ruleset.kt b/core/src/com/unciv/models/ruleset/Ruleset.kt index d6bd5034a0..ef3143f957 100644 --- a/core/src/com/unciv/models/ruleset/Ruleset.kt +++ b/core/src/com/unciv/models/ruleset/Ruleset.kt @@ -278,34 +278,8 @@ class Ruleset { return stringList.joinToString { it.tr() } } - /** Severity level of Mod RuleSet check */ - enum class CheckModLinksStatus {OK, Warning, Error} - /** Result of a Mod RuleSet check */ - // essentially a named Pair with a few shortcuts - class CheckModLinksResult(val status: CheckModLinksStatus, val message: String) { - // Empty constructor just makes the Complex Mod Check on the new game screen shorter - constructor(): this(CheckModLinksStatus.OK, "") - // Constructor that joins lines - constructor(status: CheckModLinksStatus, lines: ArrayList): - this (status, - lines.joinToString("\n")) - // Constructor that auto-determines severity - constructor(warningCount: Int, lines: ArrayList): - this ( - when { - lines.isEmpty() -> CheckModLinksStatus.OK - lines.size == warningCount -> CheckModLinksStatus.Warning - else -> CheckModLinksStatus.Error - }, - lines) - // Allows $this in format strings - override fun toString() = message - // Readability shortcuts - fun isError() = status == CheckModLinksStatus.Error - fun isNotOK() = status != CheckModLinksStatus.OK - } - fun checkUniques(uniqueContainer:IHasUniques, lines:ArrayList, + fun checkUniques(uniqueContainer:IHasUniques, lines:RulesetErrorList, severityToReport: UniqueType.UniqueComplianceErrorSeverity) { val name = if (uniqueContainer is INamed) uniqueContainer.name else "" @@ -323,15 +297,47 @@ class Ruleset { .getAnnotation(Deprecated::class.java) if (deprecationAnnotation != null) { // Not user-visible - println("$name's unique \"${unique.text}\" is deprecated ${deprecationAnnotation.message}," + - " replace with \"${deprecationAnnotation.replaceWith.expression}\"") + lines.add("$name's unique \"${unique.text}\" is deprecated ${deprecationAnnotation.message}," + + " replace with \"${deprecationAnnotation.replaceWith.expression}\"", + RulesetErrorSeverity.WarningOptionsOnly) } } } - fun checkModLinks(): CheckModLinksResult { - val lines = ArrayList() - var warningCount = 0 + + class RulesetError(val text:String, val errorSeverityToReport: RulesetErrorSeverity) + enum class RulesetErrorSeverity{ + OK, + Warning, + WarningOptionsOnly, + Error, + } + + class RulesetErrorList:ArrayList() { + operator fun plusAssign(text: String) { + add(text, RulesetErrorSeverity.Error) + } + + fun add(text: String, errorSeverityToReport: RulesetErrorSeverity) { + add(RulesetError(text, errorSeverityToReport)) + } + + fun getFinalSeverity(): RulesetErrorSeverity { + if (isEmpty()) return RulesetErrorSeverity.OK + return this.maxOf { it.errorSeverityToReport } + } + + fun isError() = getFinalSeverity() == RulesetErrorSeverity.Error + fun isNotOK() = getFinalSeverity() != RulesetErrorSeverity.OK + + fun getErrorText() = + filter { it.errorSeverityToReport != RulesetErrorSeverity.WarningOptionsOnly } + .sortedByDescending { it.errorSeverityToReport } + .joinToString { it.errorSeverityToReport.name + ": " + it.text } + } + + fun checkModLinks(): RulesetErrorList { + val lines = RulesetErrorList() // Checks for all mods - only those that can succeed without loading a base ruleset // When not checking the entire ruleset, we can only really detect ruleset-invariant errors in uniques @@ -373,7 +379,7 @@ class Ruleset { } // Quit here when no base ruleset is loaded - references cannot be checked - if (!modOptions.isBaseRuleset) return CheckModLinksResult(warningCount, lines) + if (!modOptions.isBaseRuleset) return lines val baseRuleset = RulesetCache.getBaseRuleset() // for UnitTypes fallback @@ -401,8 +407,8 @@ class Ruleset { else if ((tileImprovements[improvementName] as Stats).none() && unit.isCivilian() && !unit.hasUnique("Bonus for units in 2 tile radius 15%")) { - lines += "${unit.name} can place improvement $improvementName which has no stats, preventing unit automation!" - warningCount++ + lines.add("${unit.name} can place improvement $improvementName which has no stats, preventing unit automation!", + RulesetErrorSeverity.Warning) } } @@ -474,8 +480,8 @@ class Ruleset { if (tech.prerequisites.asSequence().filterNot { it == prereq } .any { getPrereqTree(it).contains(prereq) }){ - lines += "No need to add $prereq as a prerequisite of ${tech.name} - it is already implicit from the other prerequisites!" - warningCount++ + lines.add("No need to add $prereq as a prerequisite of ${tech.name} - it is already implicit from the other prerequisites!", + RulesetErrorSeverity.Warning) } } if (tech.era() !in eras) @@ -485,7 +491,6 @@ class Ruleset { if (eras.isEmpty()) { lines += "Eras file is empty! This will likely lead to crashes. Ask the mod maker to update this mod!" - warningCount++ } for (era in eras.values) { @@ -504,7 +509,7 @@ class Ruleset { checkUniques(era, lines, UniqueType.UniqueComplianceErrorSeverity.RulesetSpecific) } - return CheckModLinksResult(warningCount, lines) + return lines } } @@ -536,7 +541,7 @@ object RulesetCache : HashMap() { this[modRuleset.name] = modRuleset if (printOutput) { println("Mod loaded successfully: " + modRuleset.name) - println(modRuleset.checkModLinks()) + println(modRuleset.checkModLinks().getErrorText()) } } catch (ex: Exception) { if (printOutput) { @@ -588,7 +593,7 @@ object RulesetCache : HashMap() { /** * Runs [Ruleset.checkModLinks] on a temporary [combined Ruleset][getComplexRuleset] for a list of [mods] */ - fun checkCombinedModLinks(mods: LinkedHashSet): Ruleset.CheckModLinksResult { + fun checkCombinedModLinks(mods: LinkedHashSet): Ruleset.RulesetErrorList { return try { val newRuleset = getComplexRuleset(mods) newRuleset.modOptions.isBaseRuleset = true // This is so the checkModLinks finds all connections @@ -596,7 +601,8 @@ object RulesetCache : HashMap() { } catch (ex: Exception) { // This happens if a building is dependent on a tech not in the base ruleset // because newRuleset.updateBuildingCosts() in getComplexRuleset() throws an error - Ruleset.CheckModLinksResult(Ruleset.CheckModLinksStatus.Error, ex.localizedMessage) + Ruleset.RulesetErrorList() + .apply { add(ex.localizedMessage, Ruleset.RulesetErrorSeverity.Error) } } } diff --git a/core/src/com/unciv/ui/newgamescreen/ModCheckboxTable.kt b/core/src/com/unciv/ui/newgamescreen/ModCheckboxTable.kt index 136aa45410..384b16f7dd 100644 --- a/core/src/com/unciv/ui/newgamescreen/ModCheckboxTable.kt +++ b/core/src/com/unciv/ui/newgamescreen/ModCheckboxTable.kt @@ -4,7 +4,6 @@ import com.badlogic.gdx.graphics.Color import com.badlogic.gdx.scenes.scene2d.ui.CheckBox import com.badlogic.gdx.scenes.scene2d.ui.Table import com.unciv.models.ruleset.Ruleset -import com.unciv.models.ruleset.Ruleset.CheckModLinksResult import com.unciv.models.ruleset.RulesetCache import com.unciv.models.translations.tr import com.unciv.ui.utils.* @@ -61,12 +60,10 @@ class ModCheckboxTable( if (modLinkErrors.isError()) { lastToast?.close() val toastMessage = - "The mod you selected is incorrectly defined!".tr() + "\n\n$modLinkErrors" + "The mod you selected is incorrectly defined!".tr() + "\n\n${modLinkErrors.getErrorText()}" lastToast = ToastPopup(toastMessage, screen, 5000L) - if (modLinkErrors.isError()) { - checkBox.isChecked = false - return false - } + checkBox.isChecked = false + return false } // Save selection for a rollback @@ -88,7 +85,7 @@ class ModCheckboxTable( val toastMessage = ( if (complexModLinkCheck.isError()) "The mod combination you selected is incorrectly defined!" else "{The mod combination you selected has problems.}\n{You can play it, but don't expect everything to work!}" - ).tr() + "\n\n$complexModLinkCheck" + ).tr() + "\n\n${complexModLinkCheck.getErrorText()}" lastToast = ToastPopup(toastMessage, screen, 5000L) if (complexModLinkCheck.isError()) { diff --git a/core/src/com/unciv/ui/worldscreen/mainmenu/OptionsPopup.kt b/core/src/com/unciv/ui/worldscreen/mainmenu/OptionsPopup.kt index ee6edb2900..c62f85c729 100644 --- a/core/src/com/unciv/ui/worldscreen/mainmenu/OptionsPopup.kt +++ b/core/src/com/unciv/ui/worldscreen/mainmenu/OptionsPopup.kt @@ -14,7 +14,7 @@ import com.unciv.logic.MapSaver import com.unciv.logic.civilization.PlayerType import com.unciv.models.UncivSound import com.unciv.models.metadata.BaseRuleset -import com.unciv.models.ruleset.Ruleset.CheckModLinksStatus +import com.unciv.models.ruleset.Ruleset import com.unciv.models.ruleset.RulesetCache import com.unciv.models.tilesets.TileSetCache import com.unciv.models.translations.TranslationFileWriter @@ -275,19 +275,22 @@ class OptionsPopup(val previousScreen: CameraStageBaseScreen) : Popup(previousSc val lines = ArrayList() var noProblem = true for (mod in RulesetCache.values.sortedBy { it.name }) { - val modLinks = if (complex) RulesetCache.checkCombinedModLinks(linkedSetOf(mod.name)) - else mod.checkModLinks() - val color = when (modLinks.status) { - CheckModLinksStatus.OK -> "#0F0" - CheckModLinksStatus.Warning -> "#FF0" - CheckModLinksStatus.Error -> "#F00" - } val label = if (mod.name.isEmpty()) BaseRuleset.Civ_V_Vanilla.fullName else mod.name - lines += FormattedLine("$label{}", starred = true, color = color, header = 3) - if (modLinks.isNotOK()) { - lines += FormattedLine(modLinks.message) - noProblem = false + lines += FormattedLine("$label{}", starred = true, header = 3) + + val modLinks = + if (complex) RulesetCache.checkCombinedModLinks(linkedSetOf(mod.name)) + else mod.checkModLinks() + for (error in modLinks) { + val color = when (error.errorSeverityToReport) { + Ruleset.RulesetErrorSeverity.OK -> "#0F0" + Ruleset.RulesetErrorSeverity.Warning, + Ruleset.RulesetErrorSeverity.WarningOptionsOnly -> "#FF0" + Ruleset.RulesetErrorSeverity.Error -> "#F00" + } + lines += FormattedLine(error.text, color = color) } + if (modLinks.isNotOK()) noProblem = false lines += FormattedLine() } if (noProblem) lines += FormattedLine("{No problems found}.")