Implement Stockpile, Spit Up, and Swallow

[mareksison]

Currently partially implemented Stockpile (apply the STOCKPILE BattlerTag that does not do anything yet and buffs Stockpile user's Defense and Special Defense)

To-Do:

• Stockpile counter
• find way to detect and remove Stockpile counter and debuff Defense and Special Defense
• implement Spit Up and Swallow

[Tempo-anon]

Where are you planning on storing the stockpile counter? Somewhere in the Pokemon or just through battler tags like STOCKPILE_ONE, STOCKPILE_TWO, and STOCKPILE_THREE?

[mareksison]

Where are you planning on storing the stockpile counter? Somewhere in the Pokemon or just through battler tags like STOCKPILE_ONE, STOCKPILE_TWO, and STOCKPILE_THREE?

I was wrestling with this question actually, and I think Im leaning towards battler tags, because I think it would be cleaner code wise, as opposed to having to write functions into the pokemon class

[mareksison]

stockType = BattlerTagType.STOCKPILE_ONE;

thank you so much for the patience and for helping me along with the changes. I have made the changes in my local and will upload once I have properly tested it.

[mareksison]

@TempsRay hi! just following up on this

[mareksison]

Add docs and address comments

Added documentation and localisation, removed "partial," and fixed the switch statement in StockpileAttr

[mareksison]

Merged conflicts

[mareksison]

Added more detail to documentation and attaching videos that show the moves work as intended

https://github.com/pagefaultgames/pokerogue/assets/1327797/9d79367a-89e1-476f-8918-f990b89bb48b

https://github.com/pagefaultgames/pokerogue/assets/1327797/2ca41ab4-b1cb-4c92-b4b4-2572e6413abd

[mareksison]

Merged once again with latest

[mareksison]

@flx-sta resolved merge conflicts

[flx-sta]

You should mark some conversations as resolved if you fixed that

[mareksison]

Thanks for the reminder @flx-sta. I resolved the conversatiions and made the necessary changes as well

[flx-sta]

Still LGTM ✅

https://github.com/pagefaultgames/pokerogue/assets/50131232/d69c5163-a824-4c66-8892-8dc8c92d957a

[zacharied]

I hate to be that guy, but the behavior here is incorrect. To reproduce the problem:

• Use Amnesia to bring SpDef up to +6.
• Use Stockpile; note that it does not increase the SpDef stat
• Use Swallow

You will see that upon Swallowing, the user loses a stage of SpDef. This should not be happening, since Swallow/Spit Up are only supposed to undo the stat changes that were provided by uses of Stockpile (source).

https://github.com/pagefaultgames/pokerogue/assets/6374275/7e4e4349-18da-4faa-bd35-f7c93cf181de

Expected behavior would be for only Def to get decreased by Swallow.

[mareksison]

I hate to be that guy, but the behavior here is incorrect. To reproduce the problem:

• Use Amnesia to bring SpDef up to +6.
• Use Stockpile; note that it does not increase the SpDef stat
• Use Swallow

You will see that upon Swallowing, the user loses a stage of SpDef. This should not be happening, since Swallow/Spit Up are only supposed to undo the stat changes that were provided by uses of Stockpile (source).

firefox_oCY5Tn7IUZ.mp4 Expected behavior would be for only Def to get decreased by Swallow.

Good point. Will fix this issue. I think for this the best course of action is to put variable son the Pokemon itself to act as trackers. That's what I will be working on

[mareksison]

@zacharied I think I fixed the issue. Initial testing shows that it should be working as normal. Please do check. I will test some more when I wake up.

[zacharied]

Why not just have a single Stockpile BattlerTag that tracks the stack count, Def, and SpDef increase? I'm not a huge fan of having to add the stockpileStats field to the Pokemon class when that's already what a BattlerTag is supposed to accomplish. It would also save you from having to mess with StatChangePhase. Just my personal recommendation.

[mareksison]

Why not just have a single Stockpile BattlerTag that tracks the stack count, Def, and SpDef increase? I'm not a huge fan of having to add the stockpileStats field to the Pokemon class when that's already what a BattlerTag is supposed to accomplish. It would also save you from having to mess with StatChangePhase. Just my personal recommendation.

So, it would be like having 9 tags? 3 for just stockpile, 3 for knowing that defense was buffed, and 3 for knowing spdef was buffed?

I just realised that "getTargetBenefitScore" in StatChangeAttr does give me the feedback I need, which I needed "StatChangePhase" for, so I can move the check in that level at least, maybe in "StockpileStatChangeAttr"'s own "getTargetBenefitScore"

[zacharied]

So, it would be like having 9 tags? 3 for just stockpile, 3 for knowing that defense was buffed, and 3 for knowing spdef was buffed?

The idea I have in mind would only require one tag -- just store a stackCount, defIncrease, and spDefIncrease as variables of the tag. Then, in the tag's onAdd / onOverlap you can just check if the user's Def/SpDef is below +6, and add to the defIncrease/spDefIncrease. Once the user removes their stockpiles, you can just check defIncrease to see how much you need to decrease their Def by, and likewise for SpDef.

You can see HighestStatBoostTag as an example of a tag that stores and manipulates its own data.

I just realised that "getTargetBenefitScore" in StatChangeAttr does give me the feedback I need, which I needed "StatChangePhase" for, so I can move the check in that level at least, maybe in "StockpileStatChangeAttr"'s own "getTargetBenefitScore"

I see where you're coming from but definitely don't do this. getTargetBenefitScore is used by the AI for calculating which move to use, and will get run regardless of whether the move is actually used in battle or not. If one of those functions tries to change the state of the battle, you will encounter bugs when you give one of these moves to the AI.

[zacharied]

@mareksison, I got curious and tried out the approach I described on my own remote. You can see the changes here. It's basically completely implemented besides AI and localization.

IMO it ends up being way cleaner. It avoids adding state to the Pokemon class and requires no modification to phases, which greatly reduces our chances of bugs. It's also just much less code in general. Let me know your thoughts when you have a moment.

[mareksison]

@mareksison, I got curious and tried out the approach I described on my own remote. You can see the changes here. It's basically completely implemented besides AI and localization.

IMO it ends up being way cleaner. It avoids adding state to the Pokemon class and requires no modification to phases, which greatly reduces our chances of bugs. It's also just much less code in general. Let me know your thoughts when you have a moment.

Alright, will take a look and implement them

[mareksison]

@mareksison, I got curious and tried out the approach I described on my own remote. You can see the changes here. It's basically completely implemented besides AI and localization.

IMO it ends up being way cleaner. It avoids adding state to the Pokemon class and requires no modification to phases, which greatly reduces our chances of bugs. It's also just much less code in general. Let me know your thoughts when you have a moment.

I see, I understand now, you can actually just add the variable straight to the tag. Didn't know that was possible when I started this. Thanks. Implementing now.

[mareksison]

https://github.com/pagefaultgames/pokerogue/assets/1327797/099d12de-3180-470e-98d7-23c921472b88

Tested and merged into PR

[mareksison]

@zacharied hello, this should be ready for checking again

[mcmontag]

@mareksison Not to add a cook to this kitchen, but I took a stab at implementing Stockpile/Spit Up/Swallow as a way to familiarize myself with the codebase yesterday. Having done that, I just wanted to note that this PR doesn't properly implement stat change tracking when adding and removing stacks. Most of it centers around Contrary, but there may be other problematic cases, as well.

More specifically, removing stacks should apply -1 DEF and/or -1 SPDEF (before taking abilities into account) for each stack that successfully changed DEF or SPDEF, respectively.

The stacking logic in this PR simply checks that DEF/SPDEF are below +6 before raising them (and counting them as successful).

But consider (as just one example) a pokemon with Contrary and -6 DEF (or SPDEF). With your logic, using Stockpile would not actually change their DEF, but if their DEF is then raised to at least -5 before removing stacks, the removal will incorrectly lower DEF by 1 stage (well, raise it because of Contrary, but the point is that it shouldn't try to adjust DEF at all).

My implementation (https://github.com/mcmontag/pokerogue/commit/c16b1610c) happens to otherwise be almost identical to yours (though I didn't get around to localization), and handles stat change tracking by adding an optional callback to the constructor of StatChangePhase, which the tag utilizes.

Thoughts on this? I'd be happy to make a PR to your branch with this logic, if you think it makes sense to go that route. When I was working through it (and unaware of this PR), my assumption was that hooking into StatChangePhase is the only way to be certain that a stat did in fact change, but I could be missing something.

[mareksison]

@mareksison Not to add a cook to this kitchen, but I took a stab at implementing Stockpile/Spit Up/Swallow as a way to familiarize myself with the codebase yesterday. Having done that, I just wanted to note that this PR doesn't properly implement stat change tracking when adding and removing stacks. Most of it centers around Contrary, but there may be other problematic cases, as well.

More specifically, removing stacks should apply -1 DEF and/or -1 SPDEF (before taking abilities into account) for each stack that successfully changed DEF or SPDEF, respectively.

The stacking logic in this PR simply checks that DEF/SPDEF are below +6 before raising them (and counting them as successful).

But consider (as just one example) a pokemon with Contrary and -6 DEF (or SPDEF). With your logic, using Stockpile would not actually change their DEF, but if their DEF is then raised to at least -5 before removing stacks, the removal will incorrectly lower DEF by 1 stage (well, raise it because of Contrary, but the point is that it shouldn't try to adjust DEF at all).

My implementation (mcmontag@c16b1610c) happens to otherwise be almost identical to yours (though I didn't get around to localization), and handles stat change tracking by adding an optional callback to the constructor of StatChangePhase, which the tag utilizes.

Thoughts on this? I'd be happy to make a PR to your branch with this logic, if you think it makes sense to go that route. When I was working through it (and unaware of this PR), my assumption was that hooking into StatChangePhase is the only way to be certain that a stat did in fact change, but I could be missing something.

I get it. @mcmontag I think it would make sense if you PR into my branch so that we can update this PR

[flx-sta]

@mareksison since you are still looking into the implementation I'd ask you to mark this PR as draft

[Tempo-anon]

Closing due to merge conflicts and https://github.com/pagefaultgames/pokerogue/pull/2960 was merged in