Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Gen. 9 Move Effects, 2nd batch #2870
Gen. 9 Move Effects, 2nd batch #2870
Changes from 22 commits
1595a66
05a34ed
d2a221e
8ae57dc
000077f
ed9e860
5bde987
977a55b
8c4396d
a59ecf8
318b1ce
7d49954
43a5aa0
28ce87f
2043380
787c0a1
b09ab5a
313f522
5fbfc11
eb26039
3a23207
81bb299
9896f20
9984a9e
48c04f9
2bf8850
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't needed. Jump directly to Calm Mind after the ppreduce.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This battle script isn't in use anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not needed. Should already be covered in the flinch move effect itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And what if either the attacker or partner has a status condition? It would be better if that was checked for as well. Take for instance both the attacker and partner are at full HP but are paralyzed. The AI would choose not to use Jungle Healing because of the current logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what
is for. if they're both at max health AND both it and its partner have no status condition, reduce 10 from the score.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's how it reads currently. If both the attacker and partner are all full health and neither the attacker nor the partner have a status condition, do
score - 10
. So far so good. But I'm commenting on the next block that is entered if either mon isn't at full health or either mon has a status condition. This second block is saying that if either mon has more than 90% of their HP remaining, doscore - 10
.So take for instance another case. Attacker is at 1 HP remaining while the partner is at full health. The
AI_DATA->hpPercents[BATTLE_PARTNER(battlerAtk)] >= 90
condition will evaluate toTRUE
causingscore - 9
. This is clearly not what's wanted.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, gotcha, I misread it. An
&&
would be needed for the HP percent, but it's not checking status in the second block. Do you think that's ok?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole section is still incredibly hacky with a lot of duplicate code. Even if it works, it would be best if cleaned up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't keep it like that because it worked, but because I couldn't come up with something better and that worked.
I've been staring really hard and scratching my peanut sized brain some more, and I think this is a nice improvement and it still seems to work. Probably the farthest I can personally go.
https://diffy.org/diff/9f85000bb97c8
EDIT: Ignore the double semicolon in the call at
Cmd_seteffectwithchance
. Fixed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you're giving yourself enough credit 🙂
Take it slow, and take a break if you need to. I'm sure after enough time you'll be able to figure it out! Hacky code can often lead to unwanted bugs down the line. It's better to take it slow and write something that if modified in the future will stay bug-free.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I stand by my earlier statement, this section has still not been fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that, that's why I haven't resolved the conversation.
Still, I can't do more than I did here, so we're at an impasse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just do
If your tests fail because of this change, might be worth reworking those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it would be cleaner to pass
gCurrentMove
instead ofsecondaryEffectChance
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would probably fit the function's own label better. I'll make the change.
EDIT: Is what I'd say if I didn't make this for Triple Arrows which has 2 different effects with different chances, lel.
Uh, suggestions for a new label?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about
CalcMoveSecondaryEffectChance
? To make it similar toCalcMoveBasePower
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me. In the Discord server, Cancer Fairy and I settled for
CalculateMoveSecondaryEffectChance
, so we were all on pretty much the same wavelength, I'll rename and push for another review.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...This move has only been in a main series game since Gen 9? Why is this needed? (PLA is not considered a main series game in the same sense as all of the other ones).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, Nintendo does seem to consider it a main game.
Regardless, as it's explained in some of the comments at
include/config/battle.h
, we're considering LA a Gen. 8 Game (so we can have a configuration like this one for Triple Arrows because adding sub-version labels likeGEN_9_LA
was rejected), and in PLA, Triple Arrows had a power of 50 and a base amount of PP of 15."Why is it [the config for Triple Arrows] needed?" It isn't. I highly doubt someone will want to make use of Triple Arrows' LA data which makes the move weaker, but I have to cover my bases. We have configurations for a lot of different things, and the different aspects of move data is one of them.
I'm just going with the flow, and for better or worse, the expansion wants to have configurations for every little thing possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it might be better in that case to invert the condition and make a special define for PLA moves (such as
#ifdef PLA_MOVE_EFFECTS
) since they don't belong to any gen. That way if someone wants the moves to do what they did in PLA, they'll get all moves that existed in PLA to do what they did.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PLA is Gen 8, but I get what you mean. Since we're not planning on adding PLA's combat mechanics, having the PLA data for these moves and not others is a bit redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you want me to proceed then, @AsparagusEduardo?
Should I just remove the move config for the PLA moves?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say so, yeah.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All moves that have a flinch chance are not affected by King's Rock.