Add backwards compatible updated base shiny odds

[kittenchilly]

Description

This adds a config P_BASE_SHINY_ODDS. I implemented it in a way that should make it backwards compatible with existing pokemon, as the config is only applied during the CreateBoxMon process.

Issue(s) that this PR fixes

Fixes #3910

Discord contact info

kittenchilly

[AsparagusEduardo]

Interesting...
We need to check what other places use GET_SHINY_VALUE and see if they need to be updated as well.

[kittenchilly]

Interesting... We need to check what other places use GET_SHINY_VALUE and see if they need to be updated as well.

I checked and all the other places are only in CreateBoxMon, so its fine I believe.

[Sneed69]

I would define the 8s as GEN_3_SHINY_ODDS or something similar.

[tertu-m]

My concern with this is that it’s backwards compartible with Gen 3, and not whatever SHINY_ODDS was previously. People can and do change that value in their hacks, and presumably if the shiny odds were already higher they would want Pokémon that used to be shiny to remain that way (say somebody was migrating from pre-BoxMon refactor)

[mrgriffin]

My concern with this is that it’s backwards compartible with Gen 3, and not whatever SHINY_ODDS was previously. People can and do change that value in their hacks, and presumably if the shiny odds were already higher they would want Pokémon that used to be shiny to remain that way (say somebody was migrating from pre-BoxMon refactor)

This is a good point... I wonder if the best place to address it would be in the changelog? We could say something like "if you have made a change to SHINY_ODDS, please copy that change to BOXMON_SHINY_ODDS in src/pokemon.c". (Assuming we #define BOXMON_SHINY_ODDS 8 and use that instead of literal 8 in MON_DATA_IS_SHINY)

[pkmnsnfrn]

I think the changelog is a perfectly good place to address this. Should we just edit the PR desc to make sure there's a note for the changelog so it doesn't get missed on generation?

[pkmnsnfrn]

Separate question, how do we test that this actually works?

[AlexOn1ine]

Separate question, how do we test that this actually works?

We merge it and if some user finds a bug revert it after

Though I generally don't see why we wouldn't merge this.

[AlexOn1ine]

I would define the 8s as GEN_3_SHINY_ODDS or something similar.

@kittenchilly can you do this?

Also can you address this comment at the top of the description?

[kittenchilly]

I would define the 8s as GEN_3_SHINY_ODDS or something similar.

@kittenchilly can you do this?

Also can you address this comment at the top of the description?

Alright, done. Tried to make it clear but it could probably be better

[AlexOn1ine]

Hey kitten, the majority of the maintainers aren't fond of this solution so it was decided to close the PR. The following reasons were mentioned

• Can't verify if it works
• Potentially there is a better solution to this problem
• Assumes users don't change shiny odds which can be dangerous

[kittenchilly]

Fair enough, but I do want to say that it's gonna be hard to verify if any changes to shiny odds work without someone shiny hunting multiple times and checking probability, which obviously would take a long time.