search

smogon / damage-calc

Pokemon games damage calculator
https://calc.pokemonshowdown.com
MIT License
396 stars 366 forks source link

Population Bomb should select 10 hits in the calc #617

Closed shrianshChari closed 5 months ago

shrianshChari commented 7 months ago

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:

  1. if ($(this).closest(".poke-info").find(".move1").find(".select2-chosen").text() === 'Population Bomb' ||
$(this).closest(".poke-info").find(".move2").find(".select2-chosen").text() === 'Population Bomb' ||
$(this).closest(".poke-info").find(".move3").find(".select2-chosen").text() === 'Population Bomb' ||
$(this).closest(".poke-info").find(".move4").find(".select2-chosen").text() === 'Population Bomb') {

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

  2. $(this).closest(".poke-info").find(".move-hits").val(moveHits);

    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 set moveHits to 10, which is not a legal value 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:

    moveGroupObj.children(".move-hits").val(moveHits)

Again, this shouldn't be merged yet, I'm mostly looking for thoughts on how I should proceed.

thejetou commented 5 months ago

Sorry for late response, but I think the ideal solution for this issue would not be to special case Population Bomb but to make it the max amount of hits for moves that are multiaccuracy (only Triple Kick, Triple Axel and Population Bomb).

As for the updating issue, I think you can just loop over move1-move4 and update them accordingly to what the move is?

thejetou commented 5 months ago

Good work, thank you!