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

[Move] Partially Implement Instruct #4813

Closed
wants to merge 14 commits into from

Conversation

Bertie690
Copy link

@Bertie690 Bertie690 commented Nov 7, 2024

Locales PR

What are the changes the user will see?

Instruct will actually work and calls the target's last used move correctly.
It currently does not play nice with Gigaton Hammer, Blood Moon and Torment, but those are related to the janky implementation of those moves currently. #4289
Also pledge jank happens currently.
Also the enemy cannot really use it atm but whatevs

Why am I making these changes?

Because I want funny klefki prankster instructing power trick shuckle rock slide.

What are the changes from a developer perspective?

Added RepeatMoveAttr for moves that force targets to repeat moves themselves. (This is distinct from CopyMoveAttr which has the user use a target's previous move)

Screenshots/Videos

Instruct repeating an ally's rock slide & subsequently failing on enemy Unown due to not having made moves yet:

Untitled.video.-.Made.with.Clipchamp.mp4

pp after instruction
image
It sorta works on pledges... a bit
https://github.com/user-attachments/assets/60490c80-57bd-43cd-8d0b-66ea511803bb
Instruct fails if used on Instruct/Outrage (Full list of moves on Bulbapedia):
https://github.com/user-attachments/assets/b9a22fa2-ab67-4d1d-bd6b-81b855fa545a
https://github.com/user-attachments/assets/59e6ef89-5a7c-477e-a2d0-469ce402b5c7

How to test the changes?

Run npm run test instruct

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?

Bertie690 and others added 2 commits November 7, 2024 14:39
TODO:
- [ ] Still need to make in-game unit tests (i haaaaate making tests)
- [ ] subsequent move calls have no messages
- [ ] probably other jank i didn't fix yet
Copy link
Collaborator

@innerthunder innerthunder left a comment

Choose a reason for hiding this comment

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

Get rid of all the changes to overrides, package-lock.json, and package.json.

src/data/move.ts Outdated Show resolved Hide resolved
This reverts commit 055eec1.
@Tempo-anon Tempo-anon added the Move Affects a move label Nov 7, 2024
@Bertie690
Copy link
Author

Bertie690 commented Nov 8, 2024

Requesting #3503 to be updated with this PR thz

(Also feedback on tests is welcome - half of them are failing and IDK why)

@Bertie690 Bertie690 changed the title [Move] Instruct [Move] Partially Implement Instruct Nov 11, 2024
@Bertie690

This comment was marked as resolved.

@Bertie690 Bertie690 marked this pull request as ready for review November 11, 2024 05:07
@Bertie690 Bertie690 requested a review from a team as a code owner November 11, 2024 05:07
src/data/move.ts Show resolved Hide resolved
src/data/move.ts Outdated Show resolved Hide resolved
@innerthunder
Copy link
Collaborator

Btw, please don't use your fork's beta branch for PRs. It makes it hard for us to checkout your branch for playtesting without affecting our own beta branch.

The best practice for dev work is to make a new branch for every PR/feature, named differently from any of the existing branches on upstream.

@Bertie690
Copy link
Author

Bertie690 commented Nov 12, 2024

Btw, please don't use your fork's beta branch for PRs. It makes it hard for us to checkout your branch for playtesting without affecting our own beta branch.

The best practice for dev work is to make a new branch for every PR/feature, named differently from any of the existing branches on upstream.

I completely forgot about this when making the branch.
In that case I'll close this PR and make a new one.
Thx for letting me know.

@Bertie690
Copy link
Author

Closing PR to change commit branch.

@Bertie690 Bertie690 closed this Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Move Affects a move
Development

Successfully merging this pull request may close these issues.

3 participants