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: skill overflow event not posted #3146

Merged
merged 1 commit into from
Jan 4, 2025

Conversation

appable0
Copy link
Contributor

@appable0 appable0 commented Jan 2, 2025

What

Fixes SkillOverflowLevelUpEvent never being posted (not sure if this was an intentional removal; it was removed in #3068 without any mention). Also removes unneeded dependency of overflow level up message on the skill progress display.

Images Screenshot 2025-01-01 at 11 45 26 PM

Changelog Fixes

  • Fixed overflow level-up message not showing and removed dependency on skill progress display. - appable

@github-actions github-actions bot added Bug Fix Bug fixes Wrong Title/Changelog There is an error in the title or changelog labels Jan 2, 2025
Copy link

github-actions bot commented Jan 2, 2025

I have detected some issues with your pull request:

Title issues:
Unknown category: 'Fixed', valid categories are: Feature, Improvement, Fix, Backend, Remove and expected categories based on your changes are: Fix

Please fix these issues. For the correct format, refer to the pull request template.

@appable0 appable0 changed the title Fixed: skill overflow event not posted Fix: skill overflow event not posted Jan 2, 2025
@github-actions github-actions bot removed the Wrong Title/Changelog There is an error in the title or changelog label Jan 2, 2025
@hannibal002 hannibal002 added this to the Version 1.1.0 milestone Jan 4, 2025
Copy link
Owner

@hannibal002 hannibal002 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while this properly reverts the removal of the old code, the old code was flawed if i see this correctly:
we compare two xp values exactly, not if the xp value is exceeded, this event will almost never get triggered in practice, right? (im blind sry)
also this will not trigger for overflow between 50 and 60 for skills that are max at 50 from hypixel

do you want to fix this in this pr or should we merge it as is and do this later?

@appable0
Copy link
Contributor Author

appable0 commented Jan 4, 2025

do you want to fix this in this pr or should we merge it as is and do this later?

I think that might need to be saved for the future unless I'm missing an obvious way to do that (I don't see anywhere that we have max skill caps stored, only default skill caps?).

Copy link
Owner

@hannibal002 hannibal002 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its fine

@hannibal002 hannibal002 merged commit 47e5c55 into hannibal002:beta Jan 4, 2025
10 of 12 checks passed
@github-actions github-actions bot removed the Bug Fix Bug fixes label Jan 4, 2025
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