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 all 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 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.
...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.