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

[Bug][ME] Allow Part Timer Rewards to Account for Amulet Coin #5343

Open
wants to merge 2 commits into
base: beta
Choose a base branch
from

Conversation

demyinn00
Copy link

@demyinn00 demyinn00 commented Feb 16, 2025

What are the changes the user will see?

Background
There was a reported bug where if a player holds the amulet coin and completes a job for the part-timer mystery encounter, they will not receive the bonus. In other words, the rewards given from the mystery encounter are unaffected by the amulet coins benefit.

Why am I making these changes?

This PR addresses this issue by ensuring that after the money multiplier is applied, we'll also give a bonus for the amulet coin.

Fixes #5285

What are the changes from a developer perspective?

The only changes in this PR resides in part-time-encounter.ts where after the money multiplier has been determined, we then give the player a bonus for the amulet coin.

Screenshots/Videos

Note: I did not write unit tests for this change since this seems to be a minor bug fix and not game breaking

Set up

  • I've created to save files, one with the amulet coin, and the other without
  • I've also used the exact party in both save files (gen 1 starting trio)
Reproducing the bug (before this commit)

Notice that the amulet coin has the same money reward

  • Without the amulet coin image image
  • With the amulet coin image
Bug fix
  • Option 1
    • Without amulet coin image image
    • With amulet coin image image
  • Option 2
  • Without amulet coin image image
  • With amulet coin image image
  • Option 3
  • Without amulet coin

    Note, I replaced this to require tackle in the moveset for the sake of testing.

    image image
  • With amulet coin image image

How to test the changes?

The testing description outlines how you can test this along with the issues.
To summarize,

  • Create two load files with the overrides
	STARTING_WAVE_OVERRIDE: 11,
	MYSTERY_ENCOUNTER_RATE_OVERRIDE: 255,
	MYSTERY_ENCOUNTER_OVERRIDE: MysteryEncounterType.PART_TIMER,
	STARTING_MODIFIER_OVERRIDE: [{name: "AMULET_COIN"}] // comment this out for one save
  • Select a pokemon for the mystery encounter for both saves and you should see that there is an increase with the amulet coin
  • Check the logs and you should see the amulet coin event logged

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

Are there any localization additions or changes? If so:

  • Has a locales PR been created on the locales repo?
    • If so, please leave a link to it here:
  • Has the translation team been contacted for proofreading/translation?

@damocleas damocleas changed the title bugfix: allow mystery encounter part timer rewards to account for amulet coin [Bug][ME] Allow Part Timer Rewards to Account for Amulet Coin Feb 16, 2025
@damocleas damocleas added P2 Bug Minor. Non crashing Incorrect move/ability/interaction Mystery Encounter Mystery Encounters and related work labels Feb 16, 2025
@demyinn00 demyinn00 marked this pull request as ready for review February 16, 2025 03:42
@demyinn00 demyinn00 requested a review from a team as a code owner February 16, 2025 03:42
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.

This is probably just me, but imo everything testable should have an automated test, especially stuff related to bug reports -- this is helpful for regression testing.

I'm not sure it's necessarily your responsibility to write the test, though. I'll approve it, because I'm writing a huge test suite anyway.

@demyinn00
Copy link
Author

demyinn00 commented Feb 16, 2025

This is probably just me, but imo everything testable should have an automated test, especially stuff related to bug reports -- this is helpful for regression testing.

I'm not sure it's necessarily your responsibility to write the test, though. I'll approve it, because I'm writing a huge test suite anyway.

Ah ok noted! I'll happily write some unit tests for this! This might be somewhere in the docs, but should I create a new part-timer test for this, or should I update the existing part-timer-encounter.test?

@SirzBenjie
Copy link
Member

Ah ok noted! I'll happily write some unit tests for this! This might be somewhere in the docs, but should I create a new part-timer test for this, or should I update the existing part-timer-encounter.test?

Oh, you should use the existing file. It's a bit different to write tests for mystery encounters (compared to, say, moves and abilities) so if you get confused don't hesitate to reach out, whether here or on the discord!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mystery Encounter Mystery Encounters and related work P2 Bug Minor. Non crashing Incorrect move/ability/interaction
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Part-Timer and An Offer you can't Refuse do not Consider Amulet Coins
3 participants