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

Population Bomb should select 10 hits in the calc #617

Merged
merged 3 commits into from
Jun 1, 2024

Conversation

shrianshChari
Copy link
Contributor

@shrianshChari shrianshChari commented Apr 13, 2024

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:

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

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

@shrianshChari shrianshChari marked this pull request as draft April 13, 2024 05:27
@thejetou
Copy link
Collaborator

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?

@shrianshChari shrianshChari marked this pull request as ready for review June 1, 2024 01:03
@thejetou thejetou merged commit 762089a into smogon:master Jun 1, 2024
2 checks passed
@thejetou
Copy link
Collaborator

thejetou commented Jun 1, 2024

Good work, thank you!

@shrianshChari shrianshChari deleted the pop-bomb-10-hits branch June 1, 2024 23:45
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.

None yet

2 participants