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

Opportunist Ability #2994

Merged
merged 18 commits into from
Oct 10, 2023
Merged

Conversation

ghoulslash
Copy link
Collaborator

@ghoulslash ghoulslash commented May 11, 2023

Now that mirror herb is merged, adding opportunist was relatively similar.

opportunist

I needed to add a new MOVEEND_ case and ABILITYEFFECT_ case because we needed all other ability, item, etc moveend effects to occur first. This might not be the right approach though. Would need to think if there's a simpler way to handle this. But I made the gTotemBoosts statValue setting a cumulative effect, so if two opponents have e.g. intrepid sword, the opportunist mon will get a sharp attack boost (see below) - This needs more research as far as official mechanics. Would opportunist trigger after each individual intrepid sword?

opportunist_2

opportunist3

Closes #2753

@ghoulslash ghoulslash changed the title add opportunist Opportunist Ability May 11, 2023
Copy link
Collaborator

@AgustinGDLV AgustinGDLV left a comment

Choose a reason for hiding this comment

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

Looks great on my local branch! I'm a little put off by gTotemBoosts staying the name for Mirror Herb and Opportunist cases. I think it's fine that we're reusing it, and I understand that this was already approved for Mirror Herb, so I'll let you decide if it should be changed to something a bit more accurate. I can only think of something like gQueuedStatBoosts, but that might be a bit misleading, too.

Illegal Ability Tests

@AsparagusEduardo
Copy link
Collaborator

I agree, gQueuedStatBoosts would be a better name for the field now that it's used for other stuff than Totem boosts.

@ghoulslash
Copy link
Collaborator Author

I agree, gQueuedStatBoosts would be a better name for the field now that it's used for other stuff than Totem boosts.

Done

@AsparagusEduardo
Copy link
Collaborator

Can you merge with upcoming so that it passed the checks thanks to #3045?

@AsparagusEduardo
Copy link
Collaborator

It's failing due to a different reason 👀

@ghoulslash
Copy link
Collaborator Author

branch updated and test fixed.ready for re-review

Copy link
Collaborator

@DizzyEggg DizzyEggg left a comment

Choose a reason for hiding this comment

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

Just a comment for now.
Would you mind creating a test for Opportunist in a double battle where partner has Intimidate, but one of the foes has Contrary, so its Attack rises. This seems like an edge case that could explode, but if it works, then I'd feel pretty comfortable merging it.

@ghoulslash
Copy link
Collaborator Author

Just a comment for now.
Would you mind creating a test for Opportunist in a double battle where partner has Intimidate, but one of the foes has Contrary, so its Attack rises. This seems like an edge case that could explode, but if it works, then I'd feel pretty comfortable merging it.

As per issue #3356 that given test will fail but not because of opportunist. Do we want to make the test now or add it to whichever PR fixes the bug?

@mrgriffin
Copy link
Collaborator

As per issue #3356 that given test will fail but not because of opportunist. Do we want to make the test now or add it to whichever PR fixes the bug?

I would add it now, but marked with KNOWN_FAILING.

test/battle/ability/opportunist.c Outdated Show resolved Hide resolved
test/battle/ability/opportunist.c Show resolved Hide resolved
@ghoulslash
Copy link
Collaborator Author

Added contrary + intimidate + opportunist test. ready to review

Co-authored-by: Eduardo Quezada D'Ottone <[email protected]>
@AsparagusEduardo AsparagusEduardo merged commit 70fbf9e into rh-hideout:upcoming Oct 10, 2023
1 check passed
@ghoulslash ghoulslash deleted the be/opportunist branch November 4, 2023 01:34
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.

ABILITY_OPPORTUNIST
5 participants