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

Gen 1: Fix Haze and Disable #8807

Merged
merged 10 commits into from
Sep 13, 2022
Merged

Gen 1: Fix Haze and Disable #8807

merged 10 commits into from
Sep 13, 2022

Conversation

gigalh128
Copy link
Contributor

I wrote many more test cases for the functionality of both haze and disable from generation 1. Please take a look at the way I implemented the fix for pokemon not being able to move on the turn they are healed by haze/thawed because I'm not 100% certain that this is the correct way to do this.

@gigalh128
Copy link
Contributor Author

gigalh128 commented Jun 9, 2022

Changes in string quotes are based off of https://github.com/smogon/pokemon-showdown/blob/master/CONTRIBUTING.md#strings

I got it wrong before and also fixed the formatting for the rest of the test cases.

Additionally, I changed back the disable turn range to 1-7 because I realized making it 0 breaks stuff (disable will last indefinitely)

sim/battle.ts Outdated Show resolved Hide resolved
sim/battle.ts Outdated
@@ -834,6 +834,14 @@ export class Battle {
// ('0000' + (this.event.modifier * 4096).toString(16)).slice(-4).toUpperCase());
relayVar = this.modify(relayVar, this.event.modifier);
}

// handle haze/thawing in gen 1 here since it needs to be part of the BeforeMove event
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't feel like the right place for this, but I'm not familiar enough with the simulation engine to say where it should go instead, so I guess it's fine.

@KrisXV KrisXV changed the title Fix Haze and Disable (Gen 1) Gen 1: Fix Haze and Disable Aug 4, 2022
Copy link
Member

@DaWoblefet DaWoblefet left a comment

Choose a reason for hiding this comment

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

I don't think the new lastStatusCuredThisTurn property is necessary. I would try investigate if onEnd() for slp and frz can be leveraged.

data/mods/gen1/moves.ts Show resolved Hide resolved
data/mods/gen1/moves.ts Outdated Show resolved Hide resolved
test/sim/moves/disable.js Outdated Show resolved Hide resolved
test/sim/moves/haze.js Outdated Show resolved Hide resolved
test/sim/moves/haze.js Outdated Show resolved Hide resolved
@gigalh128
Copy link
Contributor Author

gigalh128 commented Aug 5, 2022

Hey there, thanks for the review. I have a few notes on the commit I just made for things I did not address in my changes to the code.

You should keep inherit: true and just override functions / other properties as necessary. You have a lot of extra duplicate code!

The backwards inheritance from gen 8 was causing a lot of issues for disable, namely with its validity conditions and its duration mechanic, so I decided to stop using inheritance for disable to be safe.

This shouldn't need && undefined.

This function isn't supposed to return if the two conditions (finding a move with more than 0 pp and not being disabled already) are true, so I added an && undefined to make sure it did not return true. The reasoning for this comes from https://github.com/smogon/pokemon-showdown/blob/master/sim/pokemon.ts#L211. Disable hasn't finished executing at this point, so it is imperative that we DO NOT return true.

I don't think the new lastStatusCuredThisTurn property is necessary. I would try investigate if onEnd() for slp and frz can be leveraged.

Unfortunately, onEnd() for slp and frz doesn't actually get triggered when the status is cured, so it's useless in this scenario (in contrast to something like confusion being healed by haze, where it does get triggered). Since I figure this would be a point of further contention and would like to have my pull request merged in a reasonable amount of time, I have decided to kick this problem down the road by rolling back its changes. Please let me know if you would prefer I don't do that.

Copy link
Collaborator

@AnnikaCodes AnnikaCodes left a comment

Choose a reason for hiding this comment

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

This seems fine to me, although I'm not too familiar with Gen 1 mechanics.

@AnnikaCodes
Copy link
Collaborator

The backwards inheritance from gen 8 was causing a lot of issues for disable, namely with its validity conditions and its duration mechanic, so I decided to stop using inheritance for disable to be safe.

This should probably be addressed in the future, but inheritance bugs aren't really in the scope of this PR.

@DaWoblefet DaWoblefet merged commit 230d25a into smogon:master Sep 13, 2022
@DaWoblefet
Copy link
Member

I appreciate your patience here @gigalh128; thank you much for the fix!

MaribelHearn pushed a commit to MaribelHearn/pokemon-showdown that referenced this pull request Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants