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

[Move] Implement (or re-implement?) Lucky Chant #3352

Merged
merged 6 commits into from
Aug 7, 2024

Conversation

innerthunder
Copy link
Collaborator

@innerthunder innerthunder commented Aug 4, 2024

What are the changes?

This fully implements the move Lucky Chant.

Why am I doing these changes?

Lucky Chant is unimplemented... or, at least, it's marked as unimplemented. Before these changes, Lucky Chant used a battler tag to prevent critical hits on the user, but since it was marked (N), its effects were not functional and the move was disabled. Also, just removing the .unimplemented() flag from the move was insufficient since Lucky Chant is meant to protect both the user and its ally. The changes in this PR bring Lucky Chant to its full functionality.

What did change?

  • data/arena-tag and enums/arena-tag-type: New NoCritTag (of tag type ArenaTagType.NO_CRIT) prevents hits against the tag's side from dealing critical damage.
  • data/battler-tags and enums/battler-tag-type: Removed BattlerTagType.NO_CRIT, a part of Lucky Chant's previous partial implementation.
  • data/move: Updated Lucky Chant:
    • Removed .unimplemented() flag
    • Changed AddBattlerTagAttr to AddArenaTagAttr with the new NO_CRIT arena tag.
  • field/pokemon: Minor changes reflecting changes to NO_CRIT tag implementation.
  • test/moves/lucky_chant.test: New integration test for Lucky Chant's effects in single and double battles
  • test/moves/dragon_rage.test: Fixed a test that used the NO_CRIT tag incorrectly.

Screenshots/Videos

PokeRogueTest-LuckyChant.mp4

How to test the changes?

npm run test lucky_chant

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

@innerthunder
Copy link
Collaborator Author

innerthunder commented Aug 4, 2024

TODO before moving out of draft:

  • Localize messages in NoCritTag (keys added, just need translations)
  • Record test video

@Tempo-anon Tempo-anon added the Move Affects a move label Aug 4, 2024
src/locales/ko/arena-tag.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@Adri1 Adri1 left a comment

Choose a reason for hiding this comment

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

Added French

src/locales/fr/arena-tag.ts Outdated Show resolved Hide resolved
Co-authored-by: Jannik Tappert <38758606+CodeTappert@users.noreply.github.com>
Co-authored-by: Lugiad' <adrien.grivel@hotmail.fr>
Co-authored-by: Enoch <enoch.jwsong@gmail.com>
Co-authored-by: José Ricardo Fleury Oliveira <josefleury@discente.ufg.br>
Copy link
Contributor

@sonnyding1 sonnyding1 left a comment

Choose a reason for hiding this comment

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

Added zh_CN and zh_TW translations

src/locales/zh_CN/arena-tag.ts Outdated Show resolved Hide resolved
src/locales/zh_TW/arena-tag.ts Outdated Show resolved Hide resolved
Co-authored-by: Sonny Ding <93831983+sonnyding1@users.noreply.github.com>
@EnochG1
Copy link
Contributor

EnochG1 commented Aug 7, 2024

@innerthunder
From now Japanese is added in beta.
Could you merge latest beta and add Japanese locale files? 🙏

@innerthunder
Copy link
Collaborator Author

@innerthunder From now Japanese is added in beta. Could you merge latest beta and add Japanese locale files? 🙏

Done.

Gonna take this out of draft since I've heard some of the translators are busy/on vacation atm, but this is still waiting on ES and IT (and now JA) translations.

@innerthunder innerthunder marked this pull request as ready for review August 7, 2024 06:11
@EnochG1
Copy link
Contributor

EnochG1 commented Aug 7, 2024

IMO you don't have to get all translation. Once you add message key with english text, translators can make new PR :)

@Tempo-anon Tempo-anon merged commit a4c913d into pagefaultgames:beta Aug 7, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Move Affects a move
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants