Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] Fix #4972 Status-Prevention Abilities do not Cure Status #5406

Open
wants to merge 25 commits into
base: beta
Choose a base branch
from

Conversation

emdeann
Copy link
Contributor

@emdeann emdeann commented Feb 24, 2025

What are the changes the user will see?

The abilities: Immunity, Insomnia, Limber, Magma Armor, Oblivious, Own Tempo, Thermal Exchange, Vital Spirit, Water Bubble, and Water Veil now correctly remove corresponding status effects when they are gained. I determined that other status prevention abilities do not reapply, so they aren't on here (but let me know if I missed something).

These abilities also immediately proc and remove their corresponding statuses if the status was applied while the ability was otherwise active (i.e. mold breaker)

Why am I making these changes?

Fixes #4972

What are the changes from a developer perspective?

This reactivation part was trivial now that onGain activation as a whole is supported. I added PostSummonHealStatusAbAttr and PostSummonRemoveBattlerTagAbAttr and attached them to the relevant abilities. These will automatically be called in any "gaining" circumstance (swapping, copying, un-suppression once the neut gas fix is in, etc.)

This is not some hacky fix - these abilities are post-summon and will activate immediately on summon if the pokemon has the corresponding status.

The Mold Breaker/etc. fix is much more arduous as it's not inherently supported. What I went with instead of trying to rework mold breaker was to attach a PostApply(Status/BattlerTag) attribute to each ability that immediately removes the respective status if the ability is now active again. This feels a little messy because now each of these abilities have a lot of attributes - if mold breaker were able to call onGain similar to neutralizing gas this would be alleviated, but that is nontrivial due to mold breaker only selectively impacting abilities (I can't think of a case besides this where an ability would need to proc after a mold breaker move).

It also means I have to call arena.setIgnoreAbilities(false) before calling the PostApply attrs - this shouldn't be an issue as any mold breaker effects are over after a status/tag has already been applied, but again feels less than ideal. Unsure if I can do better here or if it's just down to the implementation of mold breaker abilities.

In doing this I also made status clearing into a phase to avoid the status icon desyncing from the rest of the battle. Could that be a separate PR? Probably, but it's relevant to making this work nicely and will be necessary for the future ability display stuff anyway.

Open to lots of feedback on how I did any of the mold breaker stuff as it feels a little janky to me and I hate janky

Screenshots/Videos

2025-02-24.19-59-52.mp4
2025-02-23.22-23-12.mp4
2025-02-24.19-51-17.mp4

How to test the changes?

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?

@emdeann emdeann requested a review from a team as a code owner February 24, 2025 06:52
@Madmadness65 Madmadness65 added Ability Affects an ability P2 Bug Minor. Non crashing Incorrect move/ability/interaction labels Feb 24, 2025
@SirzBenjie
Copy link
Member

SirzBenjie commented Feb 25, 2025

Mold Breaker/etc. fix is much more arduous as it's not inherently supported. What I went with instead of trying to rework mold breaker was to attach a PostApply(Status/BattlerTag) attribute to each ability that immediately removes the respective status if the ability is now active again. This feels a little messy because now each of these abilities have a lot of attributes - if mold breaker were able to call onGain similar to neutralizing gas this would be alleviated, but that is nontrivial due to mold breaker only selectively impacting abilities (I can't think of a case besides this where an ability would need to proc after a mold breaker move).

Okay you can make this a lot smoother by adding the logic to the MoveEndPhase, which is where the flag for mold breaker gets disabled. You may have to add a few args, but this is better than adding a new attribute. You should just be able to check the attribute you already have for these abilities.

In general, I would strongly prefer these to not make any new attributes, and instead just add optional arguments to the attributes we have.
E.g. add a field to StatusEffectImmunityAbAttr for what you need.

Copy link
Member

@SirzBenjie SirzBenjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is structured well and the code is good (other than the noted issues dean pointed out with mold breaker).
However, I want to firmly push back against adding new ability attributes unless it shows to be absolutely necessary. It seems much better to add a field to the StatusEffectImmunityAbAttr and then check if the flag is present instead of having yet another ability attribute.
I think this is a problem because we have so many ability attributes, even for those that are related. It's quite easy to forget to add an ability attribute (which has already been done in several instances and has been the sole source of a couple of bugs).

Then, you can add the applyPostSetStatus method to that class, for instance.

I like the new phase, though. That's good.
I think the mold breaker stuff needs to be handled/initiated in the MoveEndPhase.

@emdeann
Copy link
Contributor Author

emdeann commented Feb 25, 2025

Agreed on all the Mold Breaker stuff.

I still prefer to keep the two distinct attributes though - "heal when gained" and "prevent" are two distinctly different things. In this case it's a good thing that you'd have to intentionally add the extra attribute because it should be considered when adding a status prevention ability whether it also activates on summon or not (several do not).

The other thing is that StatusEffectImmunityAbAttr is not PostSummon nor can be made PostSummon, which means it couldn't take advantage of the existing activate on gain infrastructure which it absolutely should be doing.

@DayKev
Copy link
Collaborator

DayKev commented Feb 25, 2025

In this case it's a good thing that you'd have to intentionally add the extra attribute because it should be considered when adding a status prevention ability whether it also activates on summon or not (several do not).

If it's a required parameter then you still have to do that. It doesn't matter in this case, but it would achieve the same effect (and arguably be better at it, because you would be explicitly asked to define it instead of needing to know about both AbAttrs).

@emdeann
Copy link
Contributor Author

emdeann commented Feb 25, 2025

Fair enough, if there's a reasonable way to get the abilities to activate on gain without adding an extra attribute then I'm happy doing it that way. Unsure what that would look like though.

Mold breaker stuff should be a lot cleaner now and the tests are passing locally. Failing here due to some Pokedex thing it looks like

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ability Affects an ability P2 Bug Minor. Non crashing Incorrect move/ability/interaction
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Status-Prevention Abilities do not Cure Status
5 participants