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

Fix Disable/Encore interaction with Healing Wish/Lunar Dance/Baton Pass/U-turn in Gen IV #4519

Merged
merged 2 commits into from
Mar 28, 2018

Conversation

Andi-Wang
Copy link
Contributor

No description provided.

@TravisBuddy
Copy link
Contributor

Travis tests have failed

Hey @Andi-Wang,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

Node.js: 7.7

npm test
> pokemon-showdown@0.11.2 test /home/travis/build/Zarel/Pokemon-Showdown
> eslint --cache . && tsc && mocha


/home/travis/build/Zarel/Pokemon-Showdown/sim/battle.js
  1422:3  error  Expected space(s) after "if"  keyword-spacing

✖ 1 problem (1 error, 0 warnings)
  1 error, 0 warnings potentially fixable with the `--fix` option.

npm ERR! Test failed.  See above for more details.

@Andi-Wang
Copy link
Contributor Author

Mentioned in #2367

Thought I'd try my hand at contributing to a project. I think I've followed all the procedures, but it's more than possible I've missed or broken something.

@Slayer95
Copy link
Contributor

Slayer95 commented Mar 26, 2018

This would also change the behavior of Conversion 2, Mimic, Mirror Move, Sketch, Spite, Torment, and I am not sure if also anything else. Idk if that would be a conformant change. Maybe @Marty-D can give more details?

@Andi-Wang
Copy link
Contributor Author

Andi-Wang commented Mar 26, 2018

Yeah. I'm going to go digging around some research threads to see if I can find testing results for anything that relies on last move interactions. I initially assumed that they would be treated the same.

Like you said, if anyone can clarify off the top of their head, that'd be great.

EDIT: Last Resort might also be affected by my changes depending on how it's coded. Can't check right now, but going to test if a mon that only knows U-turn and Last Resort can use Last Resort after being U-turned to (Last Resort should fail in-game).

Also, what game is the simulator based on in 4th gen? This could be relevant since my changes might change Choice item+U-turn behaviour (there was a relevant bug in DP that was fixed in Platinum).

EDIT2: Found some information in Smogon's DP research threads on how those moves work in isolation, but nothing on the interactions in question.

@Marty-D
Copy link
Collaborator

Marty-D commented Mar 26, 2018

Here's a fairly detailed breakdown: http://upcarchive.playker.info/0/upokecenter/content/pokemon-diamond-version-pearl-version-and-platinum-version-last-move-used.html
PS simulates the last game(s) released during the generation, in this case HGSS. I'm not aware of any new or fixed bugs between Platinum and HGSS battle mechanics, though.

@Slayer95
Copy link
Contributor

From the chart, it seems that all "last move" cases are equivalent after Healing Wish / Lunar Dance / U-Turn 😄

@Andi-Wang
Copy link
Contributor Author

Andi-Wang commented Mar 26, 2018

Seems like a useful resource, thanks! If I'm reading the chart properly, it seems like any changes to interactions with other moves and those 4 switching moves should be correct?

A full explanation is probably unnecessary, but just for clarity's sake:

The main thing I was worried about with different games is that Diamond/Pearl have a bug where U-turning into a Choiced Pokemon that also has U-turn would lock the second Pokemon into U-turn. That in-game bug was fixed in Platinum (so if we're going off HGSS, then the bug shouldn't exist on PS).

I haven't looked at how Choice items are coded, but it's possible that my changes break that interaction and reintroduce the D/P bug. I'll take another look at it when I have time today to check.

@Andi-Wang
Copy link
Contributor Author

Andi-Wang commented Mar 26, 2018

Checked Last Resort and Choice item interactions: Last Resort works as it should (fails), and U-turn+Choice Band works as it should as well (does not lock second Pokemon with Choice Band into U-turn).

Baton Pass is the only one that doesn't work with Choice items because it looks like being choice-locked is a volatile, which Baton Pass maintains. The fix should be pretty simple (remove the choicelock volatile when Baton Passing since I'm pretty sure Baton Pass shouldn't be passing that in the first place), so I'll just go in and do that.

@Marty-D
Copy link
Collaborator

Marty-D commented Mar 27, 2018

You actually just need a noCopy: true in the choicelock volatile in statuses.js to accomplish that. Much like this: https://github.com/Zarel/Pokemon-Showdown/blob/master/data/statuses.js#L224

@Andi-Wang
Copy link
Contributor Author

Andi-Wang commented Mar 27, 2018

Ah, thanks. I'll remove the previous change and do that, then.

EDIT: Hm. That was of silly of me since evidently I didn't understand what I was trying to do with git (was trying to revert the last push and just add the line in statuses.js). Gonna try and read up about it to see what's good practice in a situation like this.

Trying to squash using the guide, but seems like I'm just making it worse, haha.
EDIT2: Think I got it to work, though now I'm reading some articles that say that I shouldn't be rebasing during a pull request when things might be under review. Too late now, but should I follow that as standard practice for the future?

@Slayer95
Copy link
Contributor

Slayer95 commented Mar 27, 2018

For us, it's fine to rebase when each review cycle is done. However, nowadays Github supports automatically rebasing when we finally merge your PR, so the rebase guide for PS can be considered obsolete now, and you shouldn't normally worry about it .... unless there are conflicts.

Unfortunately, your Git manipulation seems to have generated conflicts where there used to be none, so you'll have to fix them :/

Conflicting files
data/statuses.js

@Andi-Wang
Copy link
Contributor Author

Okay, fair enough. So in the future, I just leave it during PR?

In any case, I'll go fix the conflict.

@Slayer95
Copy link
Contributor

I fixed it myself, @Andi-Wang .

And I shall correct myself: there was indeed a conflict with another PR just merged.

So in the future, I just leave it during PR?

Yup

@Andi-Wang
Copy link
Contributor Author

Cool. Thanks for the help, @Slayer95 .

@Zarel
Copy link
Member

Zarel commented Mar 28, 2018

I think this is ready? Not sure, though.

I'll let @Marty-D merge this.

@Marty-D
Copy link
Collaborator

Marty-D commented Mar 28, 2018

Yep, looks great. Nice work, thanks very much @Andi-Wang!

@Marty-D Marty-D merged commit 6621b84 into smogon:master Mar 28, 2018
@Marty-D Marty-D mentioned this pull request Mar 28, 2018
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.

5 participants