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: action bar spells progress bars ignoring group cooldowns #748

Merged
merged 1 commit into from
May 3, 2024
Merged

fix: action bar spells progress bars ignoring group cooldowns #748

merged 1 commit into from
May 3, 2024

Conversation

glecko
Copy link
Contributor

@glecko glecko commented Apr 30, 2024

Description

Spell slots on the action bar only show progress bars for the spell's individual cooldown. The logic for doing so is there (& seems to work correctly), it's just the group check seems to be written for an older data structure of spells.lua.

Behavior

Actual

If you assign an attack spell to slot 1 (e.g. lightning strike) and another attack spell to slot 2 (e.g. fire wave), and then cast fire wave, no progress bar will be shown on spell slot 1.

Expected

Since fire wave causes spell group 1 to go into cooldown for 2s, I expect slot 1 to show a progress bar for 2s.

Type of change

Please delete options that are not relevant.

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

How Has This Been Tested

  • Tested the scenario above (two spells that share a single group cooldown)
  • Tested that the highest cooldown amonst multiple cooldown groups is shown (e.g. ultimate flame strike & ultimate energy strike)

Test Configuration:

  • Server Version: 10.77
  • Client: Latest
  • Operating System: Window

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

@@ -972,7 +972,7 @@ function onSpellGroupCooldown(groupId, duration)
end
end
if spell then
if table.contains(spell.group, groupId) then
if spell.group[groupId] ~= nil then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if spell.group[groupId] ~= nil then
if spell.group[groupId] then

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is cleaner and more idiomatic in Lua than explicitly comparing with nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree with this being more idiomatic. But won't this return false when the spell has a group cooldown of 0s?

I admit it's a bit of an edge case, but it is technically a valid config value (the spell belongs to a spell group and cannot be cast while that group is on cooldown, but casting that spell does not actually cause that group to go on cooldown).

Copy link
Owner

Choose a reason for hiding this comment

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

@glecko

From what I saw, group is an object, so it can only be a [table] or nil.

..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but spell.group[groupId] can either be nil or a number.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, but spell.group[groupId] can either be nil or a number.

correct, sry, my mistake.

@mehah mehah merged commit 597aa57 into mehah:main May 3, 2024
1 check passed
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.

3 participants