Update Simplified_Chinese.properties and Support +-*/%^ for mod-countable

[AutumnPizazz]

No description provided.

[AutumnPizazz]

I'm sorry that I only submitted a rough code contribution this time, as I'm a beginner and not familiar with all of this. Apart from the translation updates, I was unable to correspondingly adjust the module documentation, module error checker, and the translation optimization within the brackets [] for the arithmetic extension feature. This challenging task is beyond my current capability, so I'll leave it to everyone else for now.

[SomeTroglodyte]

I'm a beginner

Well, we remember you as valuable modder and helper improving Unicode handling and such stuff, so welcome to a new challenge!

Notes in no particular precedence:

• Don't PR from your master branch for any significant effort. It blocks you from doing anything else from the moment you're a commit ahead to the moment your PR is merged - and rejected PR's are a bit more effort to clean up too. Look in one of my old docs in https://github.com/SomeTroglodyte/Scratchpad/tree/master/Unciv-notes where I describe a little how and why. Assumes you're working in Android Studio, with other IDE's you might have to interpret a little, and when you're only working with the Github web interface, well, don't for actual code changes where you might want to test/debug... You seem to have worked in a separate branch 'my-test' too, but then merged to your master within your fork - that's conceptually close to how I would work, but handles very differently. Can't really recommend a way forward off the top of my head, too lazy to think.
• Don't mix translations and any other changes in one PR - l10n PR's are handled differently for a release.
• Don't include any changes under .idea unless you have a very good reason. Specifically, changes to inspectionProfiles that would affect everybody should get their own PR.
• I get that you think to optimize away multiple evaluations of placeholderParameters[0], but unrolling the building sumOf into an explicit loop seems no advantage, same for remaining civs.
• Your Regexes could profit from writing the pattern as """ string - mind you, those are not raw strings as other languages do, they're one of my pet peeves - long history, including kotlin 2.1 offering an opt-in to the worst possible workaround for the issue... Sorry, rambling. Anyway, they can clarify \ backslashes, but have problems with $ characters...
• Even more advanced would be to use commented regexes - you using a lookbehind almost merits doing so.
• isOperator doesn't need to compile a new Regex instance every time it runs, nor does it really need Regex at all at this point.
• isExpression seems... off. One - 'contains any char out of' has simpler solutions, Zero - really? That would put the old syntax into the expression category too often, or am I blind? Maybe we should introduce an explicit marker instead... Like LibreOffice Calc using a leading = as formula intro, same in a few other products I can think of, or maybe $ or somesuch?
• Reminds me once again that I think Countables should be an empowered enum, with the UniqueParameterType implementation delegating everything to companion methods - there are already inconsistencies between check and implementation, so those would be clearer when centralized...
• Separating parseExpression from the actual AST executor - not that you have any true AST - is nice, but maybe call it tokenizeExpression?
• Don't leave println in offered PR's. At least use the built-in logger.
• I experimented a bit with expressions roughly a year ago... ANTLR is an academic parser generator with some clout, but didn't have a native kotlin output at the time. Search for that keyword if you're curious.
• The implementation looks nice at first glance - a complex loop instead of recursion for (), ok why not if it works, complexity is just low enough. But... converting evaluated simpleCountableAmount nodes to string and later right back must be avoidable...
• Operator... Sounds like a candidate for an empowered enum as well. Would save a when (token) right away. Then one could go for an abstract Token interface so val outputQueue = mutableListOf<Token>(), with methods like eval(operands: List<Token>?): Int and tryParse(String): Token? - that's a very rough idea - and implementations maybe including Operator, IntegerTerm, SimpleCountable... The tokenizer could already distinguish those then. Oops, that leads to an actual AST quickly...

All in all, respect, that's an ambitious first code PR. My notes don't mean to say "ditch this and start over" at all, more like 'food for thought' for you and the boss, let's hear more competent voices first...

[yairm210]

What I'm missing here is validation that a complex expression is correct

[AutumnPizazz]

Notes in no particular precedence:

Oh my gosh, your notes are way too advanced for me. It's like trying to get a primary school student to understand quantum mechanics.

other IDE

I use IntelliJ IDEA Community Edition 2024.1.4

Don't mix translations and any other changes in one PR

I will obey it next time.

Don't include any changes under .idea

Idk what happened and that's from my IDEA Itsel's change.

Don't leave println in offered PR's

My bad, this is a leftover from the debugging process.

Overall, I'm just an amateur programmer. In fact, my major is integrated circuit design, which involves building chips by assembling countless transistors, rather than writing program code. Many of your terms and theories are quite cutting-edge and difficult for me to understand. Thank you for your kind intentions. If you think there's anything inappropriate, please feel free to make changes as you see fit.

[AutumnPizazz]

What I'm missing here is validation that a complex expression is correct

I tested it, ([[Your] Cities] + 1) * [Unit] is okay.

[SomeTroglodyte]

my major is integrated circuit design

Oh my, way way too advanced for me. It's like trying to get a kindergarden toddler to understand quantum philosophy. 😆

Anyway, yes, but any learning curve always has parts where you can't see the next stretch around the next bend...

.idea

Yes, after cloning a new project I often do a conscious scan to look for any folders/files that need exclusions or type overrides and/or index exclusion¹ and that the upstream might have forgotten or not known about because they use some other IDE. Basically any folder is suspect unless already excluded or you can guess how it has a part in the actual project sources or resources.

I tested it

He means the UniqueParameterType should be able to verify the expressions compile or at least won't crash. Like Bobby Tables, you should treat mods as external input and defend against involuntary (or not) bad input endangering the app.

feel free to make changes

Nah, not that involved these days. I'm toying with LUA/C++ bindings at the moment, which means quite somewhere else.

Footnotes

1. Right-click Mark Directory As...
   is completely separate from
   Right-click - Git - Add to .gitignore - .git/info/exclude² ↩

2. Yes exclusions go in .git/info/exclude unless you want to PR them for all participants ↩

[yairm210]

I don't think you understand what I mean about validation.

When a modder makes a mistake, we need to tell them as clearly as possible what the mistake was.
If someone fumbles a countable expression, we don't tell them anything. What if someone put a countable that doesn't exist, or forgot a square bracket, or basically anything? No data for the modder
This check is handled in UniqueParameterType, and there's nothing even close to being production ready for expression checking :)

I experimented previously with using exp4j for moddable complex math functions and that fell on validation as well, it's a tough problem, but one that's absolutely critical if we want to allow modders to input freeform equations

[AutumnPizazz]

someone fumbles a countable expression

That's not a problem, we could write a detailed doc file to tell modders how to use it, like this:

## countable

Indicates *something that can be counted*, used both for comparisons and for multiplying uniques

Allowed *simple* values:

- `year`, `turns`
- `Cities`, `[cityFilter] Cities`
- `City-States` - counts all undefeated city-states
- `Units`, `[mapUnitFilter] Units`
- `[buildingFilter] Buildings`
- `Remaining [civFilter] Civilizations`
- `Owned [tileFilter] Tiles`
- Stat name - gets the stat *reserve*, not the amount per turn (can be city stats or civilization stats, depending on where the unique is used)
- Resource name (can be city stats or civilization stats, depending on where the unique is used)

For example: If a unique is placed on a building, then the retrieved resources will be of the city. If placed on a policy, they will be of the civilization.

This can make a difference for e.g. local resources, which are counted per city.

Allowed *complex* values:
- `[complex value]`
- `[complex value] + [complex value]` - such as `[Cities] + 1`
- `[complex value] - [complex value]`
- `[complex value] * [complex value]`
- `[complex value] / [complex value]`
- `[complex value] % [complex value]`
- `[complex value] ^ [complex value]`

In addition, complex nested expressions can also be used, such as: `([Cities] + [turns]) * 2 + [Units]`. Note that the precedence of the 6 operations follows mathematical intuition: exponentiation(`^`) has the highest precedence, followed by multiplication(`*`), division(`/`), and modulus(`%`), and then addition(`+`) and subtraction(`-`) have the lowest precedence. If you need to adjust the precedence, please use parentheses.

Specifically, when calculating `n/0` or `n%0` , although these operations are meaningless, we still compute them as `0`.

[yairm210]

That's not good enough. We need to be able to parse the expression and locate the problem, not just point modders at documentation.

[AutumnPizazz]

I'm really flustered... I couldn't understand what these words meant, and after clicking, I realized I had made a big mistake. I'm really sorry for cluttering everyone's notifications... This time, I have added support for the correct complex Countables parameters to UniqueParameterType, at least that's what I think I've done.

[AutumnPizazz]

at least that's what I think I've done

When the countable parameter of the mod is invalid, the mod checker will throw an exception to inform the modder that this countable does not conform to the type. I think this prompt is very appropriate. What do you think? @yairm210

[yairm210]

No :)

If someone does "[A]++[B]", I want to indicate to them that the "++" is the problem.
If there's "[A]+[BNotCountable]" we should indicate that the "BNotCountable" is not a countable. Etc.
That's the kind of support I expect from my programming languages, and what you're doing here is bringing the Uniques one step closer to being a Turing-complete DSL. We already have 'if', 'if not', and 'for' loops, so countables accepting number functions is really the next step.
But if we're going to do this, we need to do it right, and that means being super clear what the problem is if there's a problem

[AutumnPizazz]

This task is too difficult for me at the moment.On the other hand,is it really cost-effective to make the mod checker so considerate that it teaches modders like a teacher?I think it is sufficient to get to the current stage.When modders write invalid parameters,they can ask questions on Discord and get corrected by others.We should assume that modders have such a level of literacy.

[yairm210]

I disagree :)
Our policy has always been "modder errors should be as clear as possible". Without this explainability, this looks like an endless pit of debugging to me.

[yairm210]

Since this is basically a shunting yard implementation, we can separate the parse into 2 stages:

1. Create a AST for the function - this can be validated without inputting specific values
2. When we actually need to calculate, then do the evaluation

This is also better from a performance perspective, since currently, every time we want to evaluate the countable, we need to parse the tree from scratch.
I'm not entirely sure where we should store the AST in the meantime - this is a problem I'm encountering in other places as well... I think for now we can cheat and store them all in one big hashmap in the ruleset, even if that is a bit ugly, otherwise we'd need to have specific parameter classes and I don't want to get into that :/

[yairm210]

Also it needs some testing, it doesn't work in all conditions...

[yairm210]

There are several hard parts here

• Determining what is a "simple countable" is probably the hardest one - but one that's already solved, luckily, so any square-brackets-delimited string we can shove into UniqueParameterType.Countable.isKnownValue
• Building the AST itself, once we have the tokens - super easy, barely an inconvenience
• Validating the AST (no unclosed brackets etc) - not too hard
• Plugging values in at the end - also easy

When I say "AST" you can think of this as "reverse hungarian notation" if you want, then it's just list where each one corresponds to a function or a value and it's easy to iterate on

[yairm210]

Basic function for validating countables :)

    fun getCountableErrors(string:String, ruleset: Ruleset): List<String>{
        // If this is a simple countable, we're done
        if (UniqueParameterType.Countable.isKnownValue(string, ruleset)) return emptyList()
        
        val countableTokens = string.getPlaceholderParameters()
        val errorList = mutableListOf<String>()
        for (token in countableTokens){
            if (!UniqueParameterType.Countable.isKnownValue(string, ruleset))
                errorList.add("\"$string\" contains the parameter \"$token\", which is not a valid countable")
        }
        
        // Check that all braces are matched
        var openBraces = 0
        for ((index,char) in string.withIndex()){
            if (char == '(') openBraces += 1
            if (char == ')') {
                if (openBraces > 0) openBraces -= 1
                else errorList.add("\"$string\" contains an unmatched closing brace at index $index")
            }
        }
        
        return errorList
    }

IMO we should be using getPlaceholderParameters() to parse the initial square brackets into much more manageable tokens, that'll seriously cut down on the amount of work required

[yairm210]

Although now that I examine it further, the current countable validity checks are sorely lacking - we only test if they fit the outer pattern, not the inner pattern
For example, "[] Buildings" - but we don't check if the buildingFilter itself, is valid. That's bad design.
If we want to be able to check validity recursively, we should start with that 👍🏿