[Suggestion] Statstring Enhancements

[MaxMahem]

So I make fairly heavy use of stat strings, a feature I quite like. And for mods that have a lot of soldiertypes (like piratez) the ability to set stat string by solidertype is a welcome addition.

However, it also means having to set a big-long list of statstrings for all the soldier types, which is both tedious and often open to error when setting values for soldier types that are similar but not identical. And in any case, it seems often the information I want to know is how far (that is, what percentage) a stat is from its cap.

So, why not add the capability of setting a statstring on a percentage basis? The design I am contemplating is pretty straightforward. In an existing statstring block, we simply add another (optional) option to the "range" array after the existing min and max scores. Something like...

Code: [Select]

statStrings:
 - string: "x"
   psiStrength: [~, 30, p] # percentage based
 - string: "P"
   psiStrength: [80, ~] # field absent or any other value, normal behavior
Quite compact and backward compatible.

The disadvantage with this format is that it no longer looks quite so "clean," and I guess it's maybe no longer copy-and-paste backward compatible with x-com util, though I don't know to what degree that is still valuable. Alternatively, the tag could be placed at a higher level, but this eliminates the ability to have differing percentage-based and non-percentage-based conditions within the same rule.

While I am in here working on this, it also occurs that some people might find it desirable to set conditions on an absolute basis. That is; currently, stat strings only take a soldier's "base" stats into account. But with armor, commendations, and other bonuses, a soldier's stats can rise or fall above or below that level. Some people might want to apply statstrings based on this augmented level.

This could be accomplished by adding yet another field to the array after the percentage-based marker, which I don't hate as a method, though at this point, it is becoming somewhat bulky. I'm open to other suggestions.

This is a fairly small patch, so I'm already working on it for my use. I'm not asking for anyone else to do so. I'm just trying to get feedback on what might be desirable. And what might be necessary to get this accepted?

One last thing. While I'm at it, one other minor feature I'd like to bundle with this is the ability to customize the statstring divider, which is currently hardcoded to be a "/". Ideally, I would add this as another translation string, but as the "Soldier->getName" function where this logic resides does not derive from state, I'm quite a long way away from having access to the "tr" function. Other similar classes handle this by passing the language along with the call to "getName," but Soldier does not (because soldier names don't need to be translated). I could add that argument to the method, though this would be a fairly far-reaching change for such a minor feature. Alternatively, since "Options" are global, I could put the functionality there since it is not technically a translation anyway. Open to suggestions on this point as well.

Thanks for your attention.

[Meridian]

This is a fairly small patch, so I'm already working on it for my use. I'm not asking for anyone else to do so. I'm just trying to get feedback on what might be desirable. And what might be necessary to get this accepted?

I'm indifferent towards such a feature.
Not a yes, not a no.

So I guess, to be accepted, it would need support from a significant number of community members. Or at least from a non-negligible number.

One last thing. While I'm at it, one other minor feature I'd like to bundle with this is the ability to customize the statstring divider, which is currently hardcoded to be a "/". Ideally, I would add this as another translation string, but as the "Soldier->getName" function where this logic resides does not derive from state, I'm quite a long way away from having access to the "tr" function. Other similar classes handle this by passing the language along with the call to "getName," but Soldier does not (because soldier names don't need to be translated). I could add that argument to the method, though this would be a fairly far-reaching change for such a minor feature. Alternatively, since "Options" are global, I could put the functionality there since it is not technically a translation anyway. Open to suggestions on this point as well.

Why is "/" even bothering you?

I dunno, probably a "hidden" options.cfg entry? If I had to choose.
But best just leave it be...

[R1dO]

Always nice to see people sharing the same ideas on quality of life features.

I can see it's usefulness for those mods with a lot of soldier types and a broad spectrum of stats (and regular updates that add/change).
It reduces the need for ruleset dives in order to keep one's (personalized) statstrings up to date (granted one is OK with less optimized versions).

Had a go of it myself, but it never got past a (barely) functionally concept.
Got sidetracked and haven't found the time to solve some of the challenges / bring it into a state that was useful for others [1].

Since you are actively working on it you might be interested in those challenges as to come up with solutions (or deem them inconsequential).


A. Base value
===========
Percentages need a base value as to what constitutes as 100%, there are multiple candidates one can chose:
1) Max of soldier 'statCaps'
2) Max of soldier 'trainingStatCaps'
3) Max of current soldiers on craft
4) Max of current soldiers on base
5) Max of all soldiers still alive on the globe (e.g. all bases)
6) Something else ...
Based on your post I suspect you've chosen '1' but it would not hurt to specify which one (or multiple?) you want to support.


B. Stat overflows
=============
It is possible for soldiers to slightly overflow a stat (e.g. percentages > 100), at least for the fixed base values.
Will you cap the values at 100 or recognize percentages above 100?


C. Behavior for base values != 100
===================================
From experience most (max) stat values tend to not align neatly with 100, meaning any percentage based range will lead to gaps or duplicates.
For reference, given the following statstring definition:

Code: [Select]

statStrings:
 - string: "x"
   firing: [~, 50, p]
 - string: "M"
   firing: [51, ~, p]

If the base value is < 100 one could end up with:

Code: [Select]

"SoldierName/xM"If the base value is > 100 one could end up with the following naming scheme when a soldier levels up:

Code: [Select]

"SoldierName/x" -> "SoldierName" -> "SoldierName/M" (e.g. missing statstring when a number falls between 50% and 51%).

How would you tackle this?


D. Allow both fixed and percentage for the same stat.
========================================
Although you've already touched this subject it is a bit unclear if you want support "one OR the other" or "both at the same time".
E.g. is the following example supported?

Code: [Select]

statStrings:
 - string: "x"
   firing: [75, ~, p] # Percentage based but it could map to a value of [40, ~] if base value is pretty low.
   firing: [60, ~]    # We only consider soldiers that can identify: "the business end of a gun".

E. Backwards compatibility.
=====================
When using any new definition one probably want to prevent errors (or undefined behavior) when a user tries to load it into an older OXC(E) version.
Worst case the application could choke (crash) on the ruleset. More likely it could lead to unexpected designations (percentages being treated as fixed values).
Any idea how to approach this?


My personal vision on those challenges:
===============================
A. Option (1) seemed to be the most logical one. Certainly more stable than 3-5 (not depending on RNG and not varying during the course of a campaign).
    Should prevent situations where: "One-eye would become *the* marksman in the land of the blind".
B. I believe this is less of an issue since I suspect most users will opt to use something akin to '[90, ~, p]' instead of '[90, 100, p]' but it is something to consider.
    My (weak) preference would be allowing percentages > 100. So one could designate: "the best of the best of the best ..." (e.g. '[100, ~, p]').
C. Never found a good way to solve this (yet), currently I tend toward the solution that does extra calculations:
    - If value is at upper boundary (e.g. '[~, 50, p]') calculate the next one up (e.g. '[~, 51, p]').
      + Use the midpoint for a decision if base value > 100.
      + Use floating point rounding for a decission if base value < 100.
    - If value is at lower boundary (e.g. '[51, ~, p]') calculate the next one down (e.g. '[50, ~, p]').
      + Use the midpoint for a decision if base value > 100.
      + Use floating point rounding for a decision if base value < 100.
D. Due to the nature of some stats a fixed minimum might be desired, hence my preference to support both at the same time.
    Again: "one-eye ... land of the blind".
E. Not a strong one since 'metadata.yml' value "requiredExtendedVersion" could prevent this situation, something to consider though.


Sidenote (since you are open to suggestions).
+++++++++++++++++++++++++++++++++++
Instead of adding an option to the range array, why not use specialized value definitions (like for example 'firingPercentage: [~, 66]')?
In my opinion that would make the desired use case stands out a bit more in the ruleset.


As for the statstring divider
++++++++++++++++++++
Although I do not deem it that important, percentage based statsrings deliver more QoL, I do believe it could be beneficial to personalize OXC(E) behavior.
However, there are as many opinions for separators as there are users (for example, "/", "\", " ", "|", etc...).

This means a translatable string (e.g. 'STR_STATSTRING_SEPERATOR' or something akin) has the best potential to cater for user wishes.
Since i understand bringing 'state' into 'soldier.cpp' might be hard (or nearly impossible) you might want to consider the following alternatives:
+ Add a new statstring top-level attribute e.g (replacing the current hard-coded one).

Code: [Select]

statStrings:
 - divider: "|"
 - string: "x"
   firing: [75, ~, p] # Percentage based but it could map to a value of [40, ~] if base value is pretty low.
   firing: [60, ~]    # We only consider soldiers that can identify: "the business end of a gun". 
 
+ Add a divider before each individual statstring (so one can end up with something akin to "Soldier|9|6|8|1|5")

Code: [Select]

statStrings:
 - string: "x"
   divider: "|"
   firing: [75, ~, p] # Percentage based but it could map to a value of [40, ~] if base value is pretty low.
   firing: [60, ~]    # We only consider soldiers that can identify: "the business end of a gun". 

Hope this post was of any use.

[1]_ Plus my soldiers rarely tend to age that well for it to actually becomes important for my play style :-)

[MaxMahem]

Wow, you've given me a lot to reply to, but that's good, I really appreciate the feedback. I cut some of your post together to cut down on the amount of spaghetti in my response.

A. Base value
===========
Percentages need a base value as to what constitutes as 100%, there are multiple candidates one can chose:
1) Max of soldier 'statCaps'
2) Max of soldier 'trainingStatCaps'
3) Max of current soldiers on craft
4) Max of current soldiers on base
5) Max of all soldiers still alive on the globe (e.g. all bases)
6) Something else ...
Based on your post I suspect you've chosen '1' but it would not hurt to specify which one (or multiple?) you want to support.

A. Option (1) seemed to be the most logical one. Certainly more stable than 3-5 (not depending on RNG and not varying during the course of a campaign).
    Should prevent situations where: "One-eye would become *the* marksman in the land of the blind".

I'm working on a design that is flexible and allows (potentially) multiple designs. (Specifically, I'm reworking how the `StatStringCondition` class works so that it has a bunch of sub-classes that can implement different behavior. Its `isMet` method will now take a reference to the soldier, to allow a lot of flexibility in this). Under this design 1 and 2 are trivial, and 3 and 4 might be possible (soldiers have a reference to their craft and craft's have a reference to their base). Though I don't really see working on 3 and 4 unless there is a lot of desire for it. 5 is likely more difficult as there doesn't appear to be an easy way to get a reference to all soldiers from here (a problem I've run into elsewhere).

In any case, I plan on (well this part is already done actually) #1 as the primary function. I also added support for number #2 while typing this response... I might back it out though. I think there are better ways to display this information that I might address in a different patch.

B. Stat overflows
=============
It is possible for soldiers to slightly overflow a stat (e.g. percentages > 100), at least for the fixed base values.
Will you cap the values at 100 or recognize percentages above 100?

B. I believe this is less of an issue since I suspect most users will opt to use something akin to '[90, ~, p]' instead of '[90, 100, p]' but it is something to consider.
    My (weak) preference would be allowing percentages > 100. So one could designate: "the best of the best of the best ..." (e.g. '[100, ~, p]').

The easier the logic, the better. So I'll copy the existing functionality here. If you feed it "~" (or actually any non-integer) as the first value in the array, the minimum value will get set to 0 (or 0%). If you set "~" (or any non-integer) as the secondary value, the maximum value will get set to 255 (corresponding to 255%). I don't see any specific reason to limit the ranges otherwise (and the existing code doesn't do so) so yes, percentages over 100% will be allowed. Stats less than 0 aren't meaningful in X-Com, but who knows what might happen in the future, so I'll allow for negative percents as well (the current code allows for them FWIW). We'll assume the user knows best what they are doing if they set some funky values or edge-cases.

C. Behavior for base values != 100
===================================
From experience most (max) stat values tend to not align neatly with 100, meaning any percentage based range will lead to gaps or duplicates.
For reference, given the following statstring definition:

Code: [Select]

statStrings:
 - string: "x"
   firing: [~, 50, p]
 - string: "M"
   firing: [51, ~, p]

If the base value is < 100 one could end up with:

Code: [Select]

"SoldierName/xM"If the base value is > 100 one could end up with the following naming scheme when a soldier levels up:

Code: [Select]

"SoldierName/x" -> "SoldierName" -> "SoldierName/M" (e.g. missing statstring when a number falls between 50% and 51%).

How would you tackle this?

C. Never found a good way to solve this (yet), currently I tend toward the solution that does extra calculations:
    - If value is at upper boundary (e.g. '[~, 50, p]') calculate the next one up (e.g. '[~, 51, p]').
      + Use the midpoint for a decision if base value > 100.
      + Use floating point rounding for a decission if base value < 100.
    - If value is at lower boundary (e.g. '[51, ~, p]') calculate the next one down (e.g. '[50, ~, p]').
      + Use the midpoint for a decision if base value > 100.
      + Use floating point rounding for a decision if base value < 100.

Wow, you've put a lot of thought into it. My current code here is very basic:

Code: [Select]

const int stat = (*soldier->getCurrentStats())[_conditionName];
const int cap = soldier->getRules()->getStatCaps()[_conditionName];

// percents are stored as integers, ie 50% = 50, so multiply stat by 100 to offset.
int percent = (stat * 100) / cap;

return percent >= _minVal && percent <= _maxVal;
(not tested yet either). I'll investigate more deeply when I get a bit further in design. I was planning on treating the percents as integers in definition, but if necessary switching to floats is possible.

D. Allow both fixed and percentage for the same stat.
========================================
Although you've already touched this subject it is a bit unclear if you want support "one OR the other" or "both at the same time".
E.g. is the following example supported?

Code: [Select]

statStrings:
 - string: "x"
   firing: [75, ~, p] # Percentage based but it could map to a value of [40, ~] if base value is pretty low.
   firing: [60, ~]    # We only consider soldiers that can identify: "the business end of a gun".

D. Due to the nature of some stats a fixed minimum might be desired, hence my preference to support both at the same time.
    Again: "one-eye ... land of the blind".

Yes, under my current plan, this is supported. Every line corresponds to a new condition, so you can have percentage-based conditions and traditional conditions within the same rule.

E. Backwards compatibility.
=====================
When using any new definition one probably want to prevent errors (or undefined behavior) when a user tries to load it into an older OXC(E) version.
Worst case the application could choke (crash) on the ruleset. More likely it could lead to unexpected designations (percentages being treated as fixed values).
Any idea how to approach this?

E. Not a strong one since 'metadata.yml' value "requiredExtendedVersion" could prevent this situation, something to consider though.

Well, there is nothing I can do about a new ruleset being used on an old version. Since I can't change old code, incorrect behavior is inevitable. However, under my current plan, it would not crash (elements of the array beyond the first two are just ignored). But as you say, the percentages will be treated as fixed values. If I instead treat the percentages as floats, then I think the most likely behavior is they will fail the conversion, and the minimum (0) and maximum (255) values will get used instead.

Honestly, though this isn't a thing I worry too much about. The new rules won't cause things to blow up if run with an outdated version which is about as good as can be asked for. However, your sidenote offers other possibilities.

Sidenote (since you are open to suggestions).
+++++++++++++++++++++++++++++++++++
Instead of adding an option to the range array, why not use specialized value definitions (like for example 'firingPercentage: [~, 66]')?
In my opinion that would make the desired use case stands out a bit more in the ruleset.

Wow, this is a great idea, I'm upset I didn't think about it. It makes my design slightly more complicated in someways, but simpler in others (hard to explain without posting all the code). The biggest advantage is backward compatibility would be 0 concern. My current read of the code is that condition names not present in the preset list would simply be ignored. I'll strongly ponder this design.

As for the statstring divider
++++++++++++++++++++
Although I do not deem it that important, percentage based statsrings deliver more QoL, I do believe it could be beneficial to personalize OXC(E) behavior.
However, there are as many opinions for separators as there are users (for example, "/", "\", " ", "|", etc...).

This means a translatable string (e.g. 'STR_STATSTRING_SEPERATOR' or something akin) has the best potential to cater for user wishes.
Since i understand bringing 'state' into 'soldier.cpp' might be hard (or nearly impossible) you might want to consider the following alternatives:
+ Add a new statstring top-level attribute e.g (replacing the current hard-coded one).

Code: [Select]

statStrings:
 - divider: "|"
 - string: "x"
   firing: [75, ~, p] # Percentage based but it could map to a value of [40, ~] if base value is pretty low.
   firing: [60, ~]    # We only consider soldiers that can identify: "the business end of a gun". 
 
+ Add a divider before each individual statstring (so one can end up with something akin to "Soldier|9|6|8|1|5")

Code: [Select]

statStrings:
 - string: "x"
   divider: "|"
   firing: [75, ~, p] # Percentage based but it could map to a value of [40, ~] if base value is pretty low.
   firing: [60, ~]    # We only consider soldiers that can identify: "the business end of a gun". 


I had considered this as an option. The problem with it is it means either storing the statString divider in the `Soldier` class, where it seems a bit out of place or diving somewhat deeply into the statString class to pull this bit of data out. And the problem here is that there is not just one instance of the `StatString` class, but a vector of them, one for every "rule." Neither are necessarily show-stoppers, but the design feels inelegant. I'll ponder.

The problem with your second option is there simply is not enough screen real estate to make this version very useful. Every character is at a premium.

Hope this post was of any use.

It was incredibly helpful, thanks so much for the feedback.

[MaxMahem]

I thought I'd make a separate post giving a short update on stuff I've had to back away from and new stuff I've been able to implement.

1. While I think the ability to base stat strings of a soldier's augmented stats could be quite useful to some people, generating those stats relies on having a reference to the `Mod` object (to refresh the bonuses, apparently). Unless I'm missing something (always possible) I don't have easy access to this object from this code section, so I'll back off on this one for now.

2. The existing code allows for a binary condition "is this soldier in psiTraining or not." While I'm rewriting this section, it is easy enough to extend it to also allow for normal training, so a new binary condition, "training," will also be allowed.

[Yankes]

I do not want break fun, but:

Code: [Select]

statStrings:
 - string: "x"
   firing: [75, ~, p] # Percentage based but it could map to a value of [40, ~] if base value is pretty low.
   firing: [60, ~]    # We only consider soldiers that can identify: "the business end of a gun".
this code is illegal and can't be handed by OXCE in any way. You are not allowed to duplicated key names in one block in yaml.
Game will read only `[75, ~, p]` or `[60, ~]`.

[MaxMahem]

I do not want break fun, but:

Code: [Select]

statStrings:
 - string: "x"
   firing: [75, ~, p] # Percentage based but it could map to a value of [40, ~] if base value is pretty low.
   firing: [60, ~]    # We only consider soldiers that can identify: "the business end of a gun".
this code is illegal and can't be handed by OXCE in any way. You are not allowed to duplicated key names in one block in yaml.
Game will read only `[75, ~, p]` or `[60, ~]`.

My bad, you are, of course, right. No duplicate keys. However, with the alternate design that uses a different key for percent-based conditions, it would be allowed. This is the one I'm moving forward with at the moment.

However, I should also note that conditions are inherently and based, so if you have a percent condition and an "absolute" condition active on the same attribute (which would be allowed), you would have to satisfy them both to trigger the stat string. This probably limits the usefulness of such an approach, but changing the logic to be or based is well beyond the scope of what I'm doing here.

[Yankes]

There could be point where complexity of rules like this would excess of complexity of my script engine and would be easier to use it instead.

[MaxMahem]

There could be point where complexity of rules like this would excess of complexity of my script engine and would be easier to use it instead.

The thought did definitely occur to me. But such a task was beyond my (current) level of knowledge of the code. I'm trying to ease you guy's burden, not add to it! And frankly providing a percent-based stat string both felt more accessible and fit my specific need for it. It also seemed that the relative sparseness of text manipulation in the scripting language might potentially pose a difficulty, though you would obviously know more about the limitations there than I.

But if you're willing to add it, I would certainly not want to stand in your way! Frankly, there is a lot of stuff in here related to this stat string concept that I'd like to rip out/replace, though I imagine you all will want it to stay for backward compatibility reasons regardless.

As it is, I've got a working concept running at this point, though I still need to do a lot of testing, obviously. As usual, I stuck to the conservative approach so most of the code outside my "scope" (statString and statStringConditional) remains unchanged.

[Yankes]

Right now I do not plan adding any thing, I have lot other task to do and adding another would be counter productive.

It was bit a reflection, as some time ago I have case that look on surface similar, it was stats bonuses where there was request to add limits but exposing it to normal rulesets would be painful.

though I imagine you all will want it to stay for backward compatibility reasons regardless.

yup, this is important, many people use current system and probably would like it to stay same.
But this mean end result stay same not how you archive it, some times you can rewire every thing from scratch to archive it (like I did with stat bonuses).

[R1dO]

My bad, you are, of course, right. No duplicate keys. However, with the alternate design that uses a different key for percent-based conditions, it would be allowed. This is the one I'm moving forward with at the moment.

However, I should also note that conditions are inherently and based, so if you have a percent condition and an "absolute" condition active on the same attribute (which would be allowed), you would have to satisfy them both to trigger the stat string. This probably limits the usefulness of such an approach, but changing the logic to be or based is well beyond the scope of what I'm doing here.

The "no duplicate keys" allowed was actually the reason why i've asked question D. In order not to pollute my questions with made-up parameters i decided to keep the parameters close to the original post (as to reduce confusion).
The behavior i was trying to convey is show in the comments after the parameters.
For this scenario the default AND logic is actually beneficial (someone being considered a marksman if the stat is at least 60 AND at >75% of the max).


I do not think you need to add OR logic, it is a different kind of extension.
The whole point is providing a player with a means for global definition of statstrings without the need to know beforehand the limits of each specific soldier type in a mod.
Besides that: any OR logic is already possible using existing (fixed) statstrings rules, albeit one has to use some convoluted parameter combinations that can very easily escalate into very complex (and big) rulesets. If such advanced specifications are needed I suspect Y-script is indeed the better approach.
I have yet to see mods going that route, most likely since OR logic is only needed if someone wants to be extremely specific about their designations. My suggestion would be to keep it simple. If OR is desired it will be requested in the future. No need to make your current endeavors harder than strictly necessary.

[MaxMahem]

This feature is finished, pull request sent.

Details are described in the pull request, but I think all the issues discussed should be handled.

• The ability to have both sorts of rules concurrently is provided by using new keys: "percentTu", "percentHealth", ect.
• Range conflicts should be handled by float math. Percent ranges are done inclusive, exclusive ie: [min, max).
• Percents are stored in a 0-100 notation, rather than 0-1 (this can also help avoid some unintuitive limitations of float storage)
• There is now a tag that allows for global rules to be processed with type-specific rules "globalRule". If not present, the old behavior is preserved. If set to true, this rule is processed with the soldier rules (global rules happening first)
• Soldiers can be tagged (and anti-tagged) in physicalTraining as well as psi training with the new "physTraining" rule. (setting this, or psiTraining to false will tag soldiers *not* in training.
• The stat string divider can be customized with an OXCE option "oxceStatStringDivider" which defaults to "/" it is not present in the menu but can be changed in the cfg file or by mod.
  

[li]
[/li][/list]

[Meridian]

The new features look reasonable and backwards-compatible to me.
The code also looks fine (on the first look).
I guess now we wait for community feedback.

PS: is Visual Studio automatically formatting it "const Soldier &soldier" for you or are you doing this manually? VS should be formatting it "const Soldier& soldier", at least it does that for me.

[MaxMahem]

PS: is Visual Studio automatically formatting it "const Soldier &soldier" for you or are you doing this manually? VS should be formatting it "const Soldier& soldier", at least it does that for me.

Honestly, I'm not sure. I'll check.

[MaxMahem]

The new features look reasonable and backwards-compatible to me.
The code also looks fine (on the first look).
I guess now we wait for community feedback.

PS: is Visual Studio automatically formatting it "const Soldier &soldier" for you or are you doing this manually? VS should be formatting it "const Soldier& soldier", at least it does that for me.

Figured it out. Not sure why more people aren't having this problem, but there is a .clang-format file in the root that has this line:

Code: [Select]

PointerAlignment: Right,Which has the obvious effect.