Population Bomb should select 10 hits in the calc #617
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.
https://www.smogon.com/forums/threads/pok%C3%A9mon-showdown-damage-calculator.3593546/page-37#post-10072018
Population Bomb will now automatically default to hitting 10 times when selected.
However, the code is not fit to be merged in its current state, because of the way I have implemented the code as of now. I'm mostly opening this PR so that I can get your feedback before implementing a fix.
The Error: If I have a Pokemon that knows the moves Population Bomb and a different multi-hit move (such as Icicle Spear), then if I change the item or the ability, then the multi-hit move will be put to 2 hits automatically instead of the correct number of hits (5 for Skill Link, 4 for Loaded Dice, 3 by default). This is partially a result of my current implementation, which only checks if one of the moves that the Pokemon knows is Population Bomb, instead of checking if checking if the given move is Population Bomb (this is what I do when the move itself changes, as the change function can track the new move). Because of this, it may lead to misleading calculations for Pokemon that are using Population Bomb.
Potential spots for improvement:
As stated earlier, this if statement will only check whether or not one of the moves is Population Bomb, instead of only checking if the current move is Population Bomb. The issue is that the way the code is written, it doesn't allow me to check an individual move if it isn't being changed. That takes us to...
It seems as though this call sets every
move-hits
dropdown to the same value, which causes issues when Population Bomb is present. This is because the way I have set it up, Population Bomb will setmoveHits
to 10, which is not a legalvalue
for any other multi-hit move. As a result, it seems to default to the lowest value, 2. This worked fine when Population Bomb wasn't a factor in the damage calc, but with it, it makes things quite a bit annoying. A potential workaround may be to try and update the value of each dropdown individually for each move, similar to what happens here:Again, this shouldn't be merged yet, I'm mostly looking for thoughts on how I should proceed.