Inverted skill rust value

[Foxtrot-Uniform42]

Describe the bug

Higher intelligence increases skill rust and vice versa

Steps To Reproduce

1. Select "Int or IntCap" in options
2. Create new character
3. Go to stats tab, increase and decrease intelligence

Expected behavior

Higher intelligence must decrease skill rust
Lower intelligence must increase skill rust

Screenshots

Versions and configuration

• OS: Windows 10
• Game Version: 0.E-8804-g7e87e50
• Graphics version: Tiles
• Ingame language: English
• Mods loaded: CDDA

Additional context

I didn't tested if it is just showing misleading numbers, or higher intelligence indeed increases skill rust.

[ToxiClay]

I found the following code defining the skill rust rate in the most recent version of the repo:

int Character::rust_rate() const
{
    const std::string &rate_option = get_option<std::string>( "SKILL_RUST" );
    if( rate_option == "off" ) {
        return 0;
    }

    // Stat window shows stat effects on based on current stat
    int intel = get_int();
    /** @EFFECT_INT reduces skill rust by 10% per level above 8 */
    int ret = ( ( rate_option == "vanilla" || rate_option == "capped" ) ?
                100 : 100 + 10 * ( intel - 8 ) );

    ret *= mutation_value( "skill_rust_multiplier" );

    if( ret < 0 ) {
        ret = 0;
    }

    return ret;
}

I've identified the problem: intel - 8 needs to be inverted in order to fix this issue. 8 - intel should do the trick.

If your intelligence is 14:

100 + 10 * ( intel - 8 )
100 + 10 * (14-8)
100 + 10 * (6)
100 + 60
==============
160

This is what you're experiencing. Reversing this will generate the opposite case:

100 + 10 * ( 8 - intel )
100 + 10 * (8 - 14)
100 + 10 * (-6)
100 + -60
==============
40

[ToxiClay]

I'm also not certain about the conditional operator there.

int ret = ( ( rate_option == "vanilla" || rate_option == "capped" ) ?
                100 : 100 + 10 * ( intel - 8 ) );

If I understand conditionals right, Condition ? X : Y, if "Condition" is true, it returns X.

( rate_option == "vanilla" || rate_option == "capped" ) would seem to return true no matter which, due to the OR operator, so I'm not sure how it gets to the second result.

It seems to work in practice, at least, so I'm not gonna poke it.

[Foxtrot-Uniform42]

I've done some searching, and it seems that skill_rust_modifier value increasing a time between skill rusting from #37281, #37883. So high intelligence must increase this variable.

I think "skill rust" name in intelligence description should be changed to something like "skill rust delay" or "skill retention", because currently it is misleading.
Should the #46605 PR be reverted @ToxiClay?

[ToxiClay]

Should the #46605 PR be reverted @ToxiClay?

I do not believe so.

You are correct in the sense that my codefix is incomplete in its current state; I believe a better option would be to reverse the multiplier values given by FORGETFUL and GOODMEMORY. Changing the description would necessitate a larger code change; anywhere rust_rate is called, it would need to be changed to whatever new name is devised.

@kevingranade -- does this make sense to you?

Further edit: I discovered this while taking a closer look through the code:

bool SkillLevel::rust( bool charged_bio_mem, int character_rate )
{
    const time_duration delta = calendar::turn - _lastPracticed;
    const float char_rate = character_rate / 100.0f;
    const time_duration skill_rate = rustRate( _level );
    if( to_turns<int>( skill_rate ) * char_rate <= 0 || delta <= 0_turns ||
        delta % ( skill_rate * char_rate ) != 0_turns ) {
        return false;
    }

    if( charged_bio_mem ) {
        return one_in( 5 );
    }

    _exercise -= _level;
    const std::string &rust_type = get_option<std::string>( "SKILL_RUST" );
    if( _exercise < 0 ) {
        if( rust_type == "vanilla" || rust_type == "int" ) {
            _exercise = ( 100 * _level * _level ) - 1;
            --_level;
        } else {
            _exercise = 0;
        }
    }

    return false;
}

Observe that the rate calculated by rust_rate() is passed in as character_rate and then divided by 100. Further, observe that the math being performed on the resulting char_rate is more favorable the lower char_rate is. This supports my code change.

[Foxtrot-Uniform42]

I believe a better option would be to reverse the multiplier values given by FORGETFUL and GOODMEMORY.

That what I was thinking too, before I stumbled upon PR's I mentioned: #37883 where FORGETFUL and GOODMEMORY traits were inverted to current state, because of changes in skill rust by this PR: #37281.

I'm not familiar with C++ game code, and don't know how exactly skill rust calculated, I made this issue due to poor indication of what "skill rust" name in intelligence description does.
So if increasing skill_rust_modifier makes actual skill rust lower and vice versa (according to that PR #37281), then we mustn't revert traits, and should revert intelligence to increase skill_rust_modifier value.
@Fris0uman could you please comment on how currently skill rust is calculated, and why it's value was increased by higher intelligence?

[Fris0uman]

It was almost a year ago so I don't remember very well, looking back at it the code is still pretty obscure U_U" .
I think higher rust_rate() means longer delay between rusts which would mean that the (intel - 8) part was correct and that the main problem was in the way it was displayed.
Alternativley this whole system is a mess so dumping it and making something new and cleaner instead would be nice.