[Bug] Fix moves/abilities which modify abilities

[emdeann]

What are the changes the user will see?

Moves like skill swap/entrainment and abilities like trace properly activate gained abilities.

i.e. A Pokemon gains intimidate and intimidate immediately activates - any ability which activates on summon should also activate on obtaining it for the first time.

Why am I making these changes?

To match mainline behavior - will mostly fix #3905 but I haven't touched neutralizing gas as that's a different thing.

What are the changes from a developer perspective?

• Added a setTempAbility() function to pokemon.ts which automatically applies the new ability if needed and clears primal weather (if the original ability was a primal weather ability). This should be used in place of directly modifying pokemon.summonData in most cases where an ability is changed during the battle.
• Added a couple helper functions to ability.ts as well as slightly modifying applyAbAttrsInternal. This was necessary because I needed a way to ignore passive abilities (they shouldn't be reapplied when the main ability changes) without rewriting entire ability application methods. I just added a boolean considerPassive which defaults to true but can be turned off so that applyAbAttrsInternal doesn't call the apply function with the passive ability.
• Modified all existing moves/abilities which change abilities to use setTempAbility() (at least, I think I got them all)
• Added automated tests for the above moves/abilities

I am aware that the ability bar popouts/ability text are very janky for a lot of this, but that problem goes way beyond these changes. Ideally an ability would have a lot more agency over when exactly its message/bar get displayed, but that's a major change and not in my scope here

Screenshots/Videos

Skill Swap + Intimidate:

skillswap.mp4

Trace + Intimidate:

trace.mp4

Role Play + Intimidate + Desolate Land ending

roleplay.mp4

...etc
Let me know if there's another video you want me to grab, the amount of possible test cases here kinda hurts my head

How to test the changes?

Added automated tests

Checklist

• I'm using beta as my base branch
• There is no overlap with another PR?
• The PR is self-contained and cannot be split into smaller PRs?
• Have I provided a clear explanation of the changes?
• Have I tested the changes manually?
• Are all unit tests still passing? (npm run test)
  • Have I created new automated tests (npm run create-test) or updated existing tests related to the PR's changes?
• Have I provided screenshots/videos of the changes (if applicable)?
  • Have I made sure that any UI change works for both UI themes (default and legacy)?

Are there any localization additions or changes? If so:

• Has a locales PR been created on the locales repo?
  • If so, please leave a link to it here:
• Has the translation team been contacted for proofreading/translation?

[Xavion3]

Can we possibly go for just a separate attribute for "trigger when gained" effects? It'd achieve the same effect, but be a positive restriction instead of a negative one. This would be better future proofed for any other weird abilities or potential new abilities that do one thing on gain that isn't exactly the same as what happens on summon, and avoid introducing edge cases into summon attributes by making those not actually based purely on summoning anymore.

Also it's currently an undocumented edge case, so at minimum that'd have to be fixed even if we go with the current less rigorous approach.

[emdeann]

I thought about doing it that way and I really didn't prefer it because afaik every single post-summon ability activates on gain and vice versa (illusion is better described as a "pre summon" ability as in that PR). As a model it might make more sense to make a new attribute, but I wouldn't consider it less rigorous to make the hypothetical edge case handle its own business as opposed to changing every existing post summon ability. I do agree that it would be an issue if they introduced some weird ability that had two different effects depending on whether it was gained during the turn or not, but that doesn't feel like it would align with how the mechanics are in mainline.

It's not the end of the world and I'm fine with it either way, but I feel like it would just be adding unnecessary difficulty

[Wlowscha]

I agree with Xavion on this point. One solution would be to have a MidTurnAbAttr, called by applyMidTurnAbAttr, and then a derived class MidTurnSummonAbAttr (or something to this effect) which, as part of its .apply, checks that the ability also has a PostSummonAbAttr and calls that. Then we can give MidTurnSummonAbAttr to all the abilities that need it, and exclude the few cases that don't, without having to define a separate attribute for each PostSummonAbAttr.

On a separate note, I feel that "MidTurn" is not very informative for what is doing---when I read it, it makes me wonder what is happening in the middle of the turn. Since this is always tied to receiving or reactivating an ability, maybe something like "OnGain" would be more descriptive. This is more of a personal gripe, however.

[emdeann]

@Wlowscha I agree with your point on naming, went ahead and changed that.

The way I'm looking at "on gain" vs. "post summon" is that they literally are the same thing - when a pokemon is sent out it "gains" the ability for the first time and the effects are no different than if it gains the ability during the battle. It would almost make more sense to have the base attribute named "OnGain" instead of "PostSummon" in that sense, it's more general. If there's some ability I'm not thinking of that goes against that idea then I'd happily change it, but I just don't think it makes sense right now. It would be future proofing for a mechanic change game freak could maybe make one day.

also apologies to mad who has approved this pr at least 50 times

[Xavion3]

So, a question I've got upon reviewing this properly now and how it approaches it.

Why have the temporary ability code at all? Wouldn't it make more sense to just proc the newly gained onGain abilities instead?

[SirzBenjie]

I really like this PR.
You added a tiny bit of code to have a ton of impact. The method name is clear and it is intuitive to use and understand.
I wouldn't change a thing.

[Xavion3]

So, realised some merits, but facing a different issue.

Is there any way to make this code support passives changing? Since we can do that now as of 1.6? (might've been 1.5, the versions blur a bit.)

[SirzBenjie]

So, realised some merits, but facing a different issue
Is there any way to make this code support passives changing? Since we can do that now as of 1.6? (might've been 1.5, the versions blur a bit.)

It was 1.6

[DayKev]

Well it adds even more apply*AbAttrs functions but whatever, just means more work for whoever wants to take care of that mess later lol