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

[Ability] Add form change support for Flower Gift #3941

Merged
merged 3 commits into from
Sep 2, 2024

Conversation

torranx
Copy link
Collaborator

@torranx torranx commented Aug 31, 2024

What are the changes the user will see?

Cherrim should now transform during Harsh Sunlight similar to Castform

Why am I making these changes?

Cherrim does not transform on Harsh Sunlight

What are the changes from a developer perspective?

Added form change conditions - attached to what Forecast is using

Screenshots/Videos

How to test the changes?

Use Cherrim and set weather to Harsh Sunlight/Sunny

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 considered writing automated tests for the issue?
  • If I have text, did I make it translatable and add a key in the English locale file(s)?
  • Have I tested the changes (manually)?
    • Are all unit tests still passing? (npm run test)
  • Are the changes visual?
    • Have I provided screenshots/videos of the changes?

@torranx torranx requested a review from a team as a code owner August 31, 2024 18:52
@Madmadness65 Madmadness65 added the Ability Affects an ability label Aug 31, 2024
Copy link
Collaborator

@Tempo-anon Tempo-anon left a comment

Choose a reason for hiding this comment

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

Wasn't around for when the Forecast PR got merged in but I assume it also works with all the misc edge cases such as weather being suppressed and then no longer being suppressed? If so then this looks good to me, just some personal style nits

src/data/ability.ts Outdated Show resolved Hide resolved
src/data/ability.ts Outdated Show resolved Hide resolved
@torranx
Copy link
Collaborator Author

torranx commented Aug 31, 2024

Wasn't around for when the Forecast PR got merged in but I assume it also works with all the misc edge cases such as weather being suppressed and then no longer being suppressed? If so then this looks good to me, just some personal style nits

Yes, pesky Air Lock/Cloud Nine - but one edge case is noted on the Forecast PR description (which does have already at least 2 PRs trying to fix it rn)

TLDR: if enemy with air lock/cloud nine didnt receive any attacks and fainted, post faint ability attrs do not proc - hence form change not happening, this is bypassed by PreSwitchOut ab attrs for the player & enemy trainer tho

@DayKev
Copy link
Collaborator

DayKev commented Sep 1, 2024

Well, if Cherubi evolves into Cherrim, then it won't form-change if the weather is currently sunny when it evolves. Dunno if we care about this edge case.

@torranx
Copy link
Collaborator Author

torranx commented Sep 1, 2024

Well, if Cherubi evolves into Cherrim, then it won't form-change if the weather is currently sunny when it evolves. Dunno if we care about this edge case.

Yeah this is an unaccounted edge case but I'll leave this as is for now since we currently don't have a system for post evolution.

@Tempo-anon Tempo-anon merged commit 64368b6 into pagefaultgames:beta Sep 2, 2024
4 checks passed
@torranx torranx deleted the ability/flower_gift branch October 4, 2024 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ability Affects an ability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants