Skip to content
New issue

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

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

Already on GitHub? # to your account

[Move] Spectral Thief Full Implementation #4891

Merged
merged 31 commits into from
Feb 11, 2025

Conversation

geeilhan
Copy link
Contributor

@geeilhan geeilhan commented Nov 17, 2024

What are the changes the user will see?

Spectral Thief now steals the stat changes correctly.

Why am I making these changes?

(#3503)

What are the changes from a developer perspective?

Added a SpectralThiefAttr which extends a new StatChangeBeforeDmgCalcAttr that applies Stat Stage changes before damage calculation.

Ability Attributes such as Contrary and Simple are applied correctly since the stat changes happen through the StatStageChangePhase.

I would also like a message to pop up if stats are stolen correctly. I added the english text here public/locales/en/move-trigger.json. Do I need to push it for the other languages too?

target.scene.queueMessage(i18next.t("moveTriggers:stolePositiveStatChanges", { pokemonName: getPokemonNameWithAffix(user), targetName: getPokemonNameWithAffix(target) }));

https://github.com/geeilhan/pokerogueFork/blob/590f66e09f61f2163534d265d21b84048175cfe2/src/data/move.ts#L4367-L4368

Link to pokerogue-locales PR

Screenshots/Videos

Before Spectral Thief implementation:

spectral-thief-implementation-before.webm

After Spectral Thief implementation:

spectral-thief-implementation-after.webm

How to test the changes?

added automated tests for

  • Basic Use
  • Substitute Interaction
  • Protect Interaction
  • Clear Body, White Smoke and Hyper Cutter Interaction
  • Simple Interaction
  • Contrary Interaction
  • Check if Dmg is calculated after stat changes

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?

@geeilhan geeilhan requested a review from a team as a code owner November 17, 2024 00:01
@geeilhan geeilhan marked this pull request as draft November 17, 2024 00:42
@geeilhan geeilhan changed the title [Move] Spectral Thief Full Implementation [Move] Spectral Thief Partial Implementation Nov 17, 2024
@geeilhan geeilhan changed the title [Move] Spectral Thief Partial Implementation [Move] Spectral Thief Full Implementation Nov 17, 2024
@Madmadness65 Madmadness65 added the Move Affects a move label Dec 14, 2024
@geeilhan geeilhan marked this pull request as ready for review December 22, 2024 19:34
geeilhan and others added 4 commits February 7, 2025 20:54
@geeilhan
Copy link
Contributor Author

geeilhan commented Feb 7, 2025

How do i solve this conflict? Running git submodule update --recursive does not work.

@damocleas
Copy link
Collaborator

damocleas commented Feb 7, 2025

How do i solve this conflict? Running git submodule update --recursive does not work.

git submodule update --progress --init --recursive --force --remote
should be the one that updates locales

@geeilhan
Copy link
Contributor Author

geeilhan commented Feb 7, 2025

git submodule update --progress --init --recursive --force --remote

thanks!

Unfortunately that did not resolve it :/. Is it because I have this?
globalScene.queueMessage(i18next.t("moveTriggers:stealPositiveStats", { pokemonName: getPokemonNameWithAffix(user), targetName: getPokemonNameWithAffix(target) }));

@SirzBenjie
Copy link
Member

SirzBenjie commented Feb 8, 2025

I believe I've figured out the fix.

I have a merge PR ready I did via command line.

@SirzBenjie
Copy link
Member

The associated locales PR is here: pagefaultgames/pokerogue-locales#52 (It's marked as a draft)

@SirzBenjie SirzBenjie requested review from SirzBenjie and removed request for MokaStitcher and ben-lear February 10, 2025 00:59
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.

Being able to approve this PR is the entire motivation behind me contributing so heavily to the project recently. (I jest)

@DayKev
Copy link
Collaborator

DayKev commented Feb 11, 2025

Once the locales PR is merged the locales can be updated in this PR and then it can be merged. 👍

@SirzBenjie
Copy link
Member

SirzBenjie commented Feb 11, 2025

Once the locales PR is merged the locales can be updated in this PR and then it can be merged. 👍

Wait does the PR have to be updated? It should just work right?

Or it will cause a conflict cause of submodules

@DayKev
Copy link
Collaborator

DayKev commented Feb 11, 2025

The submodule will need to be updated, without doing so the i18n keys won't exist and then it'll display the raw i18n keys in-game.

@SirzBenjie
Copy link
Member

the locales can be updated in this PR

This is what I meant. Is the locales submodule not merged independently of the PRs that need them? It really should be.

@DayKev
Copy link
Collaborator

DayKev commented Feb 11, 2025

Even if the locales were to be updated separately we'd still want to wait until it's ready before merging this PR or there'd be broken text in the game.

@SirzBenjie
Copy link
Member

Even if the locales were to be updated separately we'd still want to wait until it's ready before merging this PR or there'd be broken text in the game.

Yup yup, entirely agree :) we def don't put this in until the locales is in the beta branch. I was just unclear on how the locales got into the beta branch.

Thought there was something that needed to be changed in this PR, as in a commit to be added.

@DayKev DayKev merged commit b31d5fd into pagefaultgames:beta Feb 11, 2025
13 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Move Affects a move
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants