Unciv icon indicating copy to clipboard operation
Unciv copied to clipboard

Improve ruleset validator

Open SomeTroglodyte opened this issue 5 months ago • 2 comments

Follow-up to #13458, treats all entries in the list mentioned there. Closes #13362, because AntCiv will now be flagged for the row <= 0. Amongst a lot of other things.

Notes:

  • Will probably conflict with #13482 because it reuses the change to class YearsPerTurn (cherry-picked commit, thus the conflict will be caused by the squash-merge, not the reuse itself)
  • The vanilla/G&K clones used in various places are no longer anonymous, unnamed extension mods, hopefully preventing some rare problems.
  • Possible invalid filtering uniques are only flagged in the RulesetSpecific check (think adding another "Aircraft" - the filtering Unique is in the mod, the use as parameter is in the base).
  • AtlasPreview learned to load complex Rulesets, thus the extraImage check no longer erroneously fails.
  • But reading commit names tells the story too.

Verbatim copy of the inconsistency list from earlier PR/issue:

  • addBeliefErrors: RulesetInvariant could do the checkUniques call and the belief.type in (Any,None) check.
  • addBuildingErrorRulesetInvariant: That greatPersonPoints check should be RulesetSpecific.
  • addCityStateTypeErrors: RulesetInvariant could do the checkUnique calls, same overall just the reportRulesetSpecificErrors param different.
  • addDifficultyErrors: RulesetInvariant could test for negative modifiers. BaseRuleset check could check aiDifficultyLevel and aiFreeTechs.
  • addEraErrors: RulesetInvariant could do the checkUniques call. Most of the meat is Ruleset-specific.
  • addEventErrors: RulesetInvariant could do the checkUniques calls, same overall just the reportRulesetSpecificErrors param different.
  • addGlobalUniqueErrors: dito
  • addImprovementErrors: RulesetInvariant could do the "empty terrainsCanBeBuiltOn" test, the "should replace ${improvement.replaces} but does not have uniqueTo assigned" test, the "cannot have a negative value for a pillage yield" test, the "has both an Unpillagable unique type and a PillageYieldRandom or PillageYieldFixed unique type" test, + checkUniques
  • addPromotionErrors/addPromotionErrorsRulesetInvariant: Invariant could do the contrast check. Also, that comment "should be upgraded to error in the future" seems old enough: I'd advocate RulesetErrorSeverity.ErrorOptionsOnly.
  • addRuinsErrors: RulesetInvariant could do the negative weight test + checkUniques
  • addSpecialistErrors is fine! No meaningful RulesetInvariant stuff at all.
  • addSpeedErrors: The current test is already RulesetInvariant and should also run in that case. Also, it could check a lot more values that would cause problems when negative.
  • addTerrainErrors: the usual - RulesetInvariant could only do RulesetInvariant checkUniques
  • addVictoryTypeErrors: RulesetInvariant could do the milestone types known and same requirements checks

Omitted - I was tempted to add the misspelling logic to color "grey" (names are Gdx and they call it "gray")...

Index: core/src/com/unciv/models/ruleset/validation/RulesetValidator.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/core/src/com/unciv/models/ruleset/validation/RulesetValidator.kt b/core/src/com/unciv/models/ruleset/validation/RulesetValidator.kt
--- a/core/src/com/unciv/models/ruleset/validation/RulesetValidator.kt	(revision f45877a4599aec02299faeee5d4bcaeefef89c22)
+++ b/core/src/com/unciv/models/ruleset/validation/RulesetValidator.kt	(date 1750561322220)
@@ -480,12 +480,12 @@
     //endregion
     //region General helpers
 
-    protected fun getPossibleMisspellings(originalText: String, possibleMisspellings: List<String>): List<String> =
+    fun getPossibleMisspellings(originalText: String, possibleMisspellings: List<String>, leniency: Float = 1f): List<String> =
         possibleMisspellings.filter {
             getRelativeTextDistance(
                 it,
                 originalText
-            ) <= RulesetCache.uniqueMisspellingThreshold
+            ) <= RulesetCache.uniqueMisspellingThreshold * leniency
         }
 
     private fun checkContrasts(
Index: core/src/com/unciv/ui/screens/civilopediascreen/FormattedLine.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/core/src/com/unciv/ui/screens/civilopediascreen/FormattedLine.kt b/core/src/com/unciv/ui/screens/civilopediascreen/FormattedLine.kt
--- a/core/src/com/unciv/ui/screens/civilopediascreen/FormattedLine.kt	(revision f45877a4599aec02299faeee5d4bcaeefef89c22)
+++ b/core/src/com/unciv/ui/screens/civilopediascreen/FormattedLine.kt	(date 1750561642201)
@@ -139,8 +139,14 @@
         if (indent !in 0..100)  // arbitrary
             yield("indent is out of sensible range")
         if (color.isNotEmpty())
-            if (parseColor() == null)
-                yield("unknown color \"$color\"")
+            if (parseColor() == null) {
+                // leniency = 1.67f is just enough to allow grey/gray to be displayed
+                val possibleColors = validator.getPossibleMisspellings(color.lowercase(), Colors.getColors().map { it.key.lowercase() }, 1.67f)
+                if (possibleColors.isEmpty())
+                    yield("unknown color \"$color\"")
+                else
+                    yield("unknown color \"$color\" - maybe a misspelling of ${possibleColors.joinToString(" or ") { "\"$it\"" }}?")
+            }
             else if (text.isEmpty() && textToDisplay.isEmpty() && !starred && !separator)
                 yield("color set but nothing to apply it to")
         if (iconCrossed && link.isEmpty() && icon.isEmpty())
... but it doesn't really work perfectly. "green" has the exact same distance.

SomeTroglodyte avatar Jun 22 '25 03:06 SomeTroglodyte

The changes are too widespread for me to track what's actually happened here (in first fixingPass commit, the rest are ok), were it someone else I would require splitting this into sub-PRs, since it's you I'm taking it as a given that the changes are for the better

yairm210 avatar Jun 22 '25 06:06 yairm210

track what's actually happened here

Yes I noticed the diff can't make sense of it, but that's mostly because it's not good at tracking moving code from one file to the other. Such code moves were in order when its RulesetSpecific/RulesetInvariant scope wasn't quite right, and most of that was unifying the checkUniques calls in the RulesetValidator class by using the reportRulesetSpecificErrors field. A few added checks - like more numbers of Speed are checked for non-negative, or the Difficulty aiFreeTechs are now checked IIRC.

Or - another viewpoint - that commit checks off that list item by item and pretty much nothing else. "pretty much" meaning allowing for obvious small things I may have noticed while still concentrating on that checklist.

SomeTroglodyte avatar Jun 22 '25 11:06 SomeTroglodyte

Quick note that this PR seems to fix this problem. I haven't looked deeply into the root cause, however....

Screenshot from 2025-06-22 11-29-22

RobLoach avatar Jun 22 '25 15:06 RobLoach

seems to fix this problem

Yes I know :zany_face:

SomeTroglodyte avatar Jun 22 '25 17:06 SomeTroglodyte

Just added some minimal ModConstants checks on top. Please holler if (any of) you know of more ModOptions.constants where bogus values could crash us.

SomeTroglodyte avatar Jun 23 '25 20:06 SomeTroglodyte