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

fix: overflow with critical hit and leech calculations #3091

Closed
wants to merge 3 commits into from

Conversation

dudantas
Copy link
Member

@dudantas dudantas commented Nov 9, 2024

Summary

This addresses an overflow issue in the skill level calculations for critical hit and leech stats. The problem occurred when the sum of varSkills and skillLevel exceeded the maximum value allowed by uint16_t, resulting in incorrect calculations for critical damage and leech values. These stats were recently updated to float, enabling high values that unfit the uint16_t.

Root Cause

The overflow issue was caused by using uint16_t for variables that could receive values exceeding its maximum limit (65,535). This was particularly problematic for SKILL_CRITICAL_HIT_DAMAGE, SKILL_LIFE_LEECH_AMOUNT, and SKILL_MANA_LEECH_AMOUNT, as they are now floats.

Solution

The solution involves treating these skills differently in the getSkillLevel function to prevent overflow and ensure accurate calculations for high values. This includes adjusting the handling of the critical hit multiplier to correctly reflect percentages over 100%.


Description

This change fixes the overflow issue that was causing incorrect critical damage calculations. The handling of SKILL_CRITICAL_HIT_DAMAGE, SKILL_LIFE_LEECH_AMOUNT, and SKILL_MANA_LEECH_AMOUNT has been adjusted to treat them as floats to prevent truncation when values exceed the limit of uint16_t. This change ensures that critical hits and leech effects are calculated correctly even for high values.

Behaviour

Actual

When the sum of varSkills + skillLevel exceeded the uint16_t limit, it caused an overflow, leading to incorrect critical damage calculations. For example, a value of 70000 for critical damage was being truncated to 65,535, resulting in incorrect damage scaling.

Expected

Correctly handle critical and leech stats as floats, even when their values exceed the uint16_t limit. For example, a critical damage value of 70000 should be treated as a 700% bonus.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested

The fix was manually tested using characters with high varSkills values. Logs were added to confirm that critical hits and leech calculations are now performed correctly, even when values exceed the previous limit.

Tests Conducted:

  • Manual testing with characters having high critical hit and leech stats.

  • Adjusted logs to confirm the correct calculations for critical hits and leech amounts.

  • Verified that the overflow issue no longer occurs with values above 65,535.

    • Test A: Verified critical hit multiplier calculations with high varSkills values.
    • Test B: Confirmed leech calculations with maximum possible values.

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I checked the PR checks reports
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works

@dudantas dudantas changed the title fix: overflow issue with critical hit and leech calculations fix: overflow with critical hit and leech calculations Nov 9, 2024
If the sum of "varSkills" + "skillLevel" exceeds the maximum uint16_t (allowed skillLevel), then overflow would occur and the critical bug would occur.

As critical and leech are now floats, they can have very high values ​​that do not fit in a uint16_t, requiring different treatment
@dudantas dudantas force-pushed the dudantas/fix-float-skills-percentage branch from ac6fbd0 to e883f1e Compare November 9, 2024 21:41
@W4gNII
Copy link

W4gNII commented Nov 10, 2024

when effect is aplicated have a certain variation of attack value / life leech

normal hit 4k

with fix 80K

@dudantas
Copy link
Member Author

when effect is aplicated have a certain variation of attack value / life leech

normal hit 4k

with fix 80K

Test this please: e574ea5

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
E Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@dudantas
Copy link
Member Author

This is a client limitation, which happens because the maximum allowed is uint16_t (that's why the overflow happens), so it doesn't make sense for the server to increase this limit if the client won't accept it (at least for now)

@dudantas dudantas closed this Dec 11, 2024
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