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

[Refactor] Align ability display with mainline #5267

Draft
wants to merge 40 commits into
base: beta
Choose a base branch
from

Conversation

emdeann
Copy link
Contributor

@emdeann emdeann commented Feb 7, 2025

What are the changes the user will see?

  • Abilities correctly display before their effects happen, rather than after
  • The show/hide animations of the ability bar now block phase changes, so it no longer gets desynced with the battle
  • When multiple abilities go off in succession, they now display properly instead of overlapping or other weirdness
  • Generally, it is more clear when an ability is activating and what its immediate effect is

Todo:
Ability issues are quite prevalent right now, probably best to leave this one in draft until some of them are sorted

  • Make abilities that don't show in mainline not show here (i.e. technician shouldn't be showing up on every hit of a multi hit move, or at all)
  • Does HideAbilityPhase need to be separate from ShowAbilityPhase? Think I like it better this way but it's a lot of duplicate code
  • Add automated tests
  • Find bugs popping up from the splitting of apply, which almost certainly exist
  • Remove the boolean return type from apply

Why am I making these changes?

To make ability display as a whole stop feeling janky

What are the changes from a developer perspective?

apply (and friends) of AbAttr is now split into two functions: canApply, which handles the logic of whether an ability can apply or not in the current situation, and apply, which actually applies the ability's effects. This is necessary to allow "looking into the future" to see whether an ability will succeed.

applyAbAttrsInternal now takes an additional function parameter successFunc, which is treated essentially identically to applyFunc. The various forms of apply...AbAttrs are edited to match this.

In the process of refactoring I pulled some code common to Magician and Pickpocket's attributes and made it into helper functions. I also added a simulated parameter to tryTransferHeldItemModifier in BattleScene so that that method can be used either as a test (i.e. in canApply...) or to apply its effect (i.e. in apply...). For the most part I separated "can" and "do" logic for these changes, but with the promise weirdness of magician and pickpocket it didn't seem worth it.

Arena now has canSet(Weather/Terrain) in addition to trySet(Weather/Terrain) to allow checking those conditions without actually changing the arena.

Phase.start() and MessagePhase no longer do anything ability bar related. It's all handled by the ability-related phases now

ShowAbilityPhase now stores the pokemon and ability names as strings rather than storing the object, because abilities which directly modify properties may change the pokemon object before it's shown (i.e. imposter would show the copied ability from the opponent, not "Imposter").

HideAbilityPhase now handles hiding the ability. This is done as a separate phase as opposed to just a timer on the tween in AbilityBar because in mainline, the ability shows -> the ability's effects happen -> the ability hides. That is how it's implemented here; if apply adds a phase (stat change, etc.) the bar won't be hidden until that phase finishes. Worth noting that HideAbilityPhase is extremely similar to ShowAbilityPhase to the point where it might be worth making them one class to avoid duplication of code.

(Show/Hide)AbilityPhase being blocking allowed significant simplification of AbilityBar.

Screenshots/Videos

Old ability bar:

old.mp4
old2.mp4

New:

new.mp4
new2.mp4

(Post summon abilities aren't the only things changed by this, but are the easiest to show)

How to test the changes?

hopefully automated tests sometime

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)?

@Madmadness65 Madmadness65 added the Refactor Rewriting existing code related label Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Refactor Rewriting existing code related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants