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] Potential fix for Expert Breeder's Pokemon being invisible #4661

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

Conversation

PigeonBar
Copy link
Collaborator

@PigeonBar PigeonBar commented Oct 14, 2024

What are the changes the user will see?

(Original bug report: https://discord.com/channels/1125469663833370665/1289869659297812520)

The Expert Pokemon Breeder's ace should never be invisible.

(Note: The linked bug report also mentioned some crashes when starting the Expert Pokemon Breeder. However, I was never able to reproduce a crash. So, I guess that this PR could also fix the crashes, but we can't know for sure.)

Why am I making these changes?

Fixing a bug, potentially fixing a crash.

What are the changes from a developer perspective?

The ME options for Expert Pokemon Breeder now properly return a Promise so that the game can wait for the Promise to be resolved before starting the battle. This Promise includes loading all assets needed for the battle, so this change should cause the game to wait for all assets to be loaded before starting the battle.

Also updated some tests.

How to test the changes?

Modify encounter-phase.utils.ts:340 to add a 5000ms delay to asset loading, like this:

loadEnemyAssets.push(new Promise(resolve => {
  setTimeout(() => {
    enemyPokemon.loadAssets().then(() => {
      resolve();
    });
  }, 5000);
}));

Also: npm run test expert-breeder

Screenshots/Videos

Before the changes:

20241014.Before.Breeder.Fix.mp4

After the changes:

20241014.After.Breeder.Fix.mp4

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?

@PigeonBar PigeonBar requested a review from a team as a code owner October 14, 2024 15:28
@Tempo-anon Tempo-anon added the P3 Bug Non gameplay affecting bug. typos, graphical issues, or other minor incorrect interactions. label Oct 14, 2024
}));
}

return scene.currentBattle?.enemyParty || [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return scene.currentBattle?.enemyParty || [];
return scene.currentBattle?.enemyParty ?? [];

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I used || instead of ?? because that's also the implementation of the original scene.getEnemyParty(), should it be changed there as well?

getEnemyParty(): EnemyPokemon[] {
return this.currentBattle?.enemyParty || [];
}

}));
}

return scene.currentBattle?.enemyParty || [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return scene.currentBattle?.enemyParty || [];
return scene.currentBattle?.enemyParty ?? [];

}));
}

return scene.currentBattle?.enemyParty || [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return scene.currentBattle?.enemyParty || [];
return scene.currentBattle?.enemyParty ?? [];

vi.spyOn(scene, "getEnemyParty").mockImplementation(() => {
const ace = scene.currentBattle?.enemyParty[0];
if (ace) {
// Pretend that loading assets takes an extra 500ms
Copy link
Collaborator

Choose a reason for hiding this comment

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

Tests currently override delays to be 1ms, might need to disable that for these tests. Lemme look into making some helper functions for that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

inputs.test.ts also ultimately relies on setTimeout, so using setTimeout here should be fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 Bug Non gameplay affecting bug. typos, graphical issues, or other minor incorrect interactions.
Projects
Status: To triage
Development

Successfully merging this pull request may close these issues.

3 participants