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] Metal Burst causes game to crash if intended target faints #4576

Closed
wants to merge 1 commit into from

Conversation

seanrmon
Copy link
Contributor

@seanrmon seanrmon commented Oct 4, 2024

What are the changes the user will see?

Metal burst will no longer cause game to crash if there are no valid targets.

Why am I making these changes?

I believe this was caused by [Refactor] Refactor move phase and add documentation (#3974), which is currently in use on the beta branch.

What are the changes from a developer perspective?

In resolveCounterAttackTarget(), if attacker is null then this.targets[0] does not get set and this.scene.getField()[this.targets[0]].hp will lead to a memory access violation.

Screenshots/Videos

image

PokeRogue.Mozilla.Firefox.2024-10-04.17-27-35.mp4

How to test the changes?

sessionData_metalburstcrash.txt

Use Water Spout and Metal Burst. See video.

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?

@seanrmon seanrmon requested a review from a team as a code owner October 4, 2024 18:21
@Tempo-anon Tempo-anon requested a review from DayKev October 4, 2024 20:21
@Madmadness65 Madmadness65 added Move Affects a move P1 Bug Major. Game crashing move/ability/interaction labels Oct 4, 2024
@DayKev
Copy link
Collaborator

DayKev commented Oct 5, 2024

The solution that fixed this last time was

const attacker = this.pokemon.scene.getPokemonById(this.pokemon.turnData.attacksReceived[0].sourceId);
if (attacker?.isActive(true)) {
this.targets[0] = attacker.getBattlerIndex();
}

being changed to this.targets[0] = this.pokemon.turnData.attacksReceived[0].sourceBattlerIndex; it seems (or maybe that change was made independently, I'm not 100% sure; the git history for this section got clobbered :regiDespair:).
Putting that code back fixes the problem (Metal Burst redirects instead of crashing), whereas yours stops the crash but causes Metal Burst to fail instead of redirecting.

@DayKev DayKev closed this Oct 5, 2024
@DayKev
Copy link
Collaborator

DayKev commented Oct 5, 2024

I'll re-add the code that got reverted and add some tests to prevent this happening again.

@DayKev
Copy link
Collaborator

DayKev commented Oct 5, 2024

Btw sorry to "steal" the PR like this, but since it's a crash issue I wanted to get it fixed w/tests ASAP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Move Affects a move P1 Bug Major. Game crashing move/ability/interaction
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants