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

Add "1 hit" option for multi-hit moves #637

Merged
merged 3 commits into from
Aug 24, 2024
Merged

Conversation

lily-ac
Copy link
Contributor

@lily-ac lily-ac commented Jul 30, 2024

@thejetou
Copy link
Collaborator

thejetou commented Aug 8, 2024

This is an incorrect approach to implement this feature because .empty() is called on .move-hits so any changes there on HTML template will be overwritten. A correct implementation would instead start iterating from 1 in this loop.

I'm not sure how much I like this to be honest since the calc is reporting incorrect multihit options for moves (Icicle Spear cannot actually hit only once) but if it helps LC players calculate rolls more efficiently then I suppose I'm fine with it.

@lily-ac
Copy link
Contributor Author

lily-ac commented Aug 14, 2024

thanks, is fixed
i agree tbh, i don't love this either, it's a janky workaround for functionality that was removed by a previous change

ideally the way damage numbers are displayed for multi-hit moves needs reworking altogether, because the current numbers are very misleading

236+ Atk Life Orb Shellder Icicle Spear (5 hits) vs. 36 HP / 0 Def Eviolite Tinkatink: 5-15 (22.7 - 68.1%) -- approx. 0.4% chance to 2HKO
Possible damage amounts: (5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 15)

this makes it look like you have a 1 in 16 chance to do 15 dmg, as opposed to the reality of 1 in 1,048,576
it also doesn't show that you can do 6 dmg for example, which would be a lot more likely to happen, and therefore helpful to know

i'm not sure what the solution looks like though, so if we can just have this workaround for now, that would be great :)

@thejetou thejetou merged commit cdf1522 into smogon:master Aug 24, 2024
2 checks passed
@thejetou
Copy link
Collaborator

Yeah, rolls are a difficult topic and would require much more invasive work to get right. I'm fine with this workaround though for now. Thanks.

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.

2 participants