-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Overhaul 'SwitchIn' event for more accurate effect resolution order #10766
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor notes on the tests.
test/sim/items/mirrorherb.js
Outdated
{species: 'Zacian', ability: 'Intrepid Sword', moves: ['sleeptalk']}, | ||
{species: 'Annihilape', ability: 'Defiant', moves: ['sleeptalk']}, | ||
]]); | ||
common.saveReplay(battle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra debug print
test/sim/items/mirrorherb.js
Outdated
{species: 'Primeape', ability: 'Defiant', item: 'Weakness Policy', moves: ['sleeptalk', 'haze']}, | ||
{species: 'Annihilape', ability: 'Defiant', item: 'Weakness Policy', moves: ['sleeptalk', 'howl']}, | ||
]]); | ||
assert.statStage(battle.p1.active[0], 'atk', 4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're checking statStage
on the same mon a bunch, add a fourth argument to these with a string indicating what should be happening (so if the test were to fail, it's more obviously apparent where it's failing)
Some notes about this refactor: the The Handler resolution order has been reworked to automatically sort effects so their handlers activate in the same order they do in-game. This preserves hazard activation timing within the The events Speed is only sorted once at the start of the switch-in event block in-game, which includes speed ties. To replicate this, an array of fractional values in the range [0, 1) are stored by the |
Several tests have been unskipped, as the mechanics they check for now work correctly. The two tests currently failing are both related to Commander, and before I can fix them I need confirmation on some mechanics questions. The eject pack one was written when Commander activated instantly instead of waiting for its turn in the switch-in event order, and now Intimidate correctly activates first, so I'm not sure if the test's assertions are accurate to the games. If they are, then this test is failing because Eject Pack is bugged in a way that I didn't fix as part of this refactor because I didn't have the relevant information on its activation timings. As for the other test, the issue is that the Tatsugiri already has Commander, so when it transforms into another Pokemon with Commander, its ability isn't restarted. However, Commander's |
3ba7af3
to
95409ab
Compare
This doesn't include a fix that lets it copy multiple Intrepid Swords, unfortunately. That still needs a larger refactor to how events trigger after simultaneous switches in.
Edit: now it does. Sorry, I got a little carried away.