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] Ignore Held Items for Stat Calculation #5254

Merged

Conversation

geeilhan
Copy link
Contributor

@geeilhan geeilhan commented Feb 4, 2025

Fixes issue mentioned in #289

What are the changes the user will see?

Quark Drive and Protosynthesis now increase correct stat.

Why am I making these changes?

What are the changes from a developer perspective?

Both Abilities used the getEffectiveStat function which could not ignore held items. (Wiki-Link)

I added another non-mandatory input to getEffectiveStat and getStatStageMultiplier which makes it possible to ignore stat changes from items (e.g. X Speed).

Screenshots/Videos

Deactive and Active Quark Drive test with Mew with X Speed (should increase ATK since X Speed is ignored and all stats are the same value):

quark-drive-test-after.webm

Same tests with harsh sunlight and Protosynthesis:

protosynthesis-test-after.webm

How to test the changes?

npm run test passes.

Also tested the implementations manually with Mew (all same stats), Ramparados and Shuckle and the stats increased correctly after the conditions for the abilities to activate are met.
In addition, I tested the Abilities effects with held items such as X ATK, and X SPEED and the stat increases worked correctly.

Currently struggling writing automated tests.
I tried using vi.spyon(player, "getEffectiveStat") but this does not work. Just using expect(player.getEffectiveStat(Stat.ATK)).toBe([Value]) also did not work.
-> Still working on the tests. Maybe just reading the damage numbers directly might work.

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

@geeilhan geeilhan requested a review from a team as a code owner February 4, 2025 22:51
@Madmadness65 Madmadness65 added Ability Affects an ability P2 Bug Minor. Non crashing Incorrect move/ability/interaction labels Feb 5, 2025
damocleas
damocleas previously approved these changes Feb 5, 2025
Copy link
Collaborator

@damocleas damocleas left a comment

Choose a reason for hiding this comment

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

Works as intended based off of stat changes and x items etc, need a dev to review

@DayKev
Copy link
Collaborator

DayKev commented Feb 7, 2025

Code looks good, needs automated tests then it's good to go.

@SirzBenjie
Copy link
Member

I added a unit test for this, but I can't suggest it here and I seem to be lacking the ability to do a PR for geeilhan's repo
https://github.com/SirzBenjie/pokerogue/tree/ignorehelditem-stat-implementation

DayKev
DayKev previously approved these changes Feb 8, 2025
src/test/abilities/protosythesis.test.ts Outdated Show resolved Hide resolved
src/test/abilities/protosythesis.test.ts Outdated Show resolved Hide resolved
src/test/abilities/protosythesis.test.ts Outdated Show resolved Hide resolved
src/test/abilities/protosythesis.test.ts Outdated Show resolved Hide resolved
src/test/abilities/protosythesis.test.ts Outdated Show resolved Hide resolved
@DayKev
Copy link
Collaborator

DayKev commented Feb 8, 2025

Seems like the test might be flaky. Re-ran it locally and it passed.
image

@geeilhan
Copy link
Contributor Author

geeilhan commented Feb 8, 2025

Seems like the test might be flaky. Re-ran it locally and it passed. image

That is weird. It previously failed because of the nature but it should work fine now.

@SirzBenjie
Copy link
Member

Mew's base stats were being changed by nature. I did an override to set the nature and I no longer observed an inconsistency across a dozen runs.
Unless starting IVs are also randomized?

@DayKev
Copy link
Collaborator

DayKev commented Feb 8, 2025

IVs shouldn't be random, last I checked they were consistent between test runs.

@SirzBenjie
Copy link
Member

IVs shouldn't be random, last I checked they were consistent between test runs.

Huh. It looks like the test failed after the nature change (unless actions messed up). Is it still non deterministic?

@DayKev
Copy link
Collaborator

DayKev commented Feb 8, 2025

Ran the test a bunch and when it failed the Magikarp moved before Mew on the first turn.

@SirzBenjie
Copy link
Member

SirzBenjie commented Feb 8, 2025

Oh I didn't do a turn order override?

I should probably fix that.

@geeilhan
Copy link
Contributor Author

geeilhan commented Feb 8, 2025

Oh I didn't do a turn order override?

I should probably fix that.

Added turnorder to tests

DayKev
DayKev previously approved these changes Feb 10, 2025
@DayKev
Copy link
Collaborator

DayKev commented Feb 10, 2025

Oh it looks like you made the same change in #4984, could you merge the changes to Tera Blast from there to this PR?

@geeilhan
Copy link
Contributor Author

Oh it looks like you made the same change in #4984, could you merge the changes to Tera Blast from there to this PR?

Added Tera Blast Changes

@SirzBenjie SirzBenjie self-requested a review February 11, 2025 00:51
@SirzBenjie SirzBenjie merged commit e75ddfa into pagefaultgames:beta Feb 11, 2025
13 checks passed
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.

5 participants