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

[Refactor][Move] Add options param interface for MoveEffectAttr #4710

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

Conversation

innerthunder
Copy link
Collaborator

@innerthunder innerthunder commented Oct 23, 2024

What are the changes the user will see?

N/A

Why am I making these changes?

At the time of writing this, MoveEffectAttr has five optional fields:

/**
 * Defines when this effect should trigger in the move's effect order
 * @see {@linkcode phases.MoveEffectPhase.start}
 */
trigger?: MoveEffectTrigger;
/** Should this effect only apply on the first hit? */
firstHitOnly?: boolean;
/** Should this effect only apply on the last hit? */
lastHitOnly?: boolean;
/** Should this effect only apply on the first target hit? */
firstTargetOnly?: boolean;
/** Overrides the secondary effect chance for this attr if set. */
effectChanceOverride?: number;

These fields help control when the attribute's effect triggers and what chance the effect has of triggering, but in general these fields are used very sparsely throughout the move code to resolve specific edge cases.

At the same time, we currently set these fields with a long sequence of constructor params:

constructor(selfTarget?: boolean, trigger?: MoveEffectTrigger, firstHitOnly: boolean = false, lastHitOnly: boolean = false, firstTargetOnly: boolean = false, effectChanceOverride?: number) {
  // ...
}

This can cause a lot of issues in terms of readability. For example, Triple Arrows has an attribute for its chance to drop the target's Defense that requires effectChanceOverride to set its chance of applying to 50%. Despite only requiring one of the optional parameters, this attribute needs to be constructed like this in allMoves:

.attr(StatStageChangeAttr, [ Stat.DEF ], -1, undefined, undefined, undefined, undefined, undefined, undefined, undefined, 50)

Of course, this also extends beyond the existing options. With potential additions from future implementations and the move scoring flags planned for the AI Rework, the code's readability is only going to get worse if the same structure is kept.

What are the changes from a developer perspective?

data/move:

  • Added interfaces (MoveEffectOptions and StatStageChangeAttrOptions) to store optional parameters for MoveEffectAttrs and StatStageChangeAttrs, respectively.
  • MoveEffectAttr now stores all of these parameters in a single field:
    protected options?: MoveEffectOptions;
    The attribute's constructor params were also changed to reflect this, and similar structural changes were made to StatStageChangeAttr.
  • New get functions allow optional parameters to be accessed in the same way as before (albeit readonly) and with the same default values.
  • SecretPowerAttr has been changed to reflect effectChanceOverride now being readonly. The effect chance roll is now done within attributes invoked internally within the apply function.

The End Result: Attributes e.g. Triple Arrows' Defense drop can be initialized as follows:

.attr(StatStageChangeAttr, [ Stat.DEF ], -1, false, { effectChanceOverride: 50 })

Screenshots/Videos

N/A

How to test the changes?

npm run test secret_power (1 test updated)
npm run test for general coverage of attributes with changed syntax

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)
  • [n/a] Are the changes visual?
    • [n/a] Have I provided screenshots/videos of the changes?

@innerthunder innerthunder requested a review from a team as a code owner October 23, 2024 08:29
@innerthunder innerthunder changed the title [Refactor][Move] Added options param interface for 'MoveEffectAttr' and 'StatStageChangeAttr' [Refactor][Move] Added options param interface for MoveEffectAttr and StatStageChangeAttr Oct 23, 2024
@innerthunder innerthunder added Move Affects a move Refactor Rewriting existing code related labels Oct 23, 2024
@innerthunder innerthunder marked this pull request as draft October 23, 2024 08:50
@innerthunder innerthunder marked this pull request as ready for review October 23, 2024 09:03
@innerthunder innerthunder changed the title [Refactor][Move] Added options param interface for MoveEffectAttr and StatStageChangeAttr [Refactor][Move] Add options param interface for MoveEffectAttr Oct 23, 2024
src/data/move.ts Outdated Show resolved Hide resolved
src/data/move.ts Outdated Show resolved Hide resolved
src/data/move.ts Outdated Show resolved Hide resolved
src/data/move.ts Outdated Show resolved Hide resolved
src/data/move.ts Outdated Show resolved Hide resolved
src/data/move.ts Outdated Show resolved Hide resolved
src/data/move.ts Outdated Show resolved Hide resolved
src/data/move.ts Outdated
@@ -8871,7 +8919,7 @@ export function initMoves() {
// If any fielded pokémon is grass-type and grounded.
return [ ...user.scene.getEnemyParty(), ...user.scene.getParty() ].some((poke) => poke.isOfType(Type.GRASS) && poke.isGrounded());
})
.attr(StatStageChangeAttr, [ Stat.ATK, Stat.SPATK ], 1, false, (user, target, move) => target.isOfType(Type.GRASS) && target.isGrounded()),
.attr(StatStageChangeAttr, [ Stat.ATK, Stat.SPATK ], 1, false, undefined, { condition: (user, target, move) => target.isOfType(Type.GRASS) && target.isGrounded() }),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe moveEffectTrigger should also be moved into the options interface, since it's set to undefined fairly often still in these declarations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Move Affects a move Refactor Rewriting existing code related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants