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] Moved code preventing MBH's transfer to post modifier generation #4858

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

Conversation

frutescens
Copy link
Collaborator

What are the changes the user will see?

MBH will no longer be able to be stolen from the opponent.

Why am I making these changes?

Bug report https://discord.com/channels/1125469663833370665/1300228174151680040

What are the changes from a developer perspective?

Code moved down to correct location.

How to test the changes?

Try to steal MBH when generated.

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 tested the changes (manually)?
    • Are all unit tests still passing? (npm run test)

@frutescens frutescens added the P2 Bug Minor. Non crashing Incorrect move/ability/interaction label Nov 12, 2024
@frutescens frutescens requested a review from a team as a code owner November 12, 2024 21:48
src/phases/encounter-phase.ts Outdated Show resolved Hide resolved
src/phases/encounter-phase.ts Outdated Show resolved Hide resolved
flx-sta
flx-sta previously approved these changes Nov 12, 2024
Copy link
Collaborator

@flx-sta flx-sta left a comment

Choose a reason for hiding this comment

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

Except that 1 comment lgtm

src/battle.ts Outdated Show resolved Hide resolved
DayKev
DayKev previously approved these changes Nov 13, 2024
@DayKev DayKev added the Item Affects an item label Nov 13, 2024
@PigeonBar

This comment was marked as resolved.

@frutescens
Copy link
Collaborator Author

@PigeonBar I think I decided just to stop a potential transfer of MBH at the heart of the code - tryTransferModifier(). Not sure if it leads to other problems though.

@PigeonBar

This comment was marked as resolved.

DayKev
DayKev previously approved these changes Nov 15, 2024
Copy link
Collaborator

@MokaStitcher MokaStitcher left a comment

Choose a reason for hiding this comment

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

I haven't tested more as I already found issues with the things I tested :(

  • Magician allows to steal MBH
  • Knock Off allows to knock it off (that can't be intended right?)
  • Thief seems to crash the game when the steal chance procs
    nevermind it seems to happen only if we override the opponent held items... These overrides are bad
console ![image](https://github.com/user-attachments/assets/34a793bd-89f7-447c-966f-c766006973e3)

At this point I'm really starting to think adding a stealable attribute for modifiers that is distinct from the transferrable one may be the way to go, rather than adding MBH-specific checks scattered in various places in the code :(

src/battle-scene.ts Outdated Show resolved Hide resolved
@frutescens
Copy link
Collaborator Author

@MokaStitcher Since there's not much time left, I decided to the code that worked before the October update instead. It's clunky and not elegant but I believe it will do the job.

Copy link
Collaborator

@MokaStitcher MokaStitcher left a comment

Choose a reason for hiding this comment

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

should be good enough for now

@DayKev
Copy link
Collaborator

DayKev commented Nov 15, 2024

At this point I'm really starting to think adding a stealable attribute for modifiers that is distinct from the transferrable one may be the way to go

I've been saying that for a long time. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Item Affects an item P2 Bug Minor. Non crashing Incorrect move/ability/interaction
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

5 participants