Unciv
Unciv copied to clipboard
Improve ruleset validator
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 anUnpillagableunique type and aPillageYieldRandomorPillageYieldFixedunique 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())
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
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.
Quick note that this PR seems to fix this problem. I haven't looked deeply into the root cause, however....
seems to fix this problem
Yes I know :zany_face:
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.