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

specWarnGTFO reuse for multiple spells shows wrong icon #267

Open
Zidras opened this issue Aug 29, 2023 · 3 comments
Open

specWarnGTFO reuse for multiple spells shows wrong icon #267

Zidras opened this issue Aug 29, 2023 · 3 comments

Comments

@Zidras
Copy link
Contributor

Zidras commented Aug 29, 2023

Describe the bug
GTFO is being commonly re-used for multiple spells inside the same mod, which leads to the icon being wrong if it doesn't match the metadata set in mod load.

Do you have an error log of what happened?
NA

To Reproduce
Example:
https://github.com/DeadlyBossMods/DBM-WotLK/blob/1e1803885425d59091953699422c987366d8fb7f/DBM-Raids-WoTLK/Coliseum/NorthrendBeasts.lua#L254

Screenshots
image

Did you try having DeadlyBossMods as the only enabled addon and everything else (especially something like ElvUI) disabled?
Yes

Which version of DeadlyBossMods are you using?
Independent of version

Was it working in a previous version? If yes, which was the last good one?
No

Additional context
There are also several cases (on multiple mods) where the text argument for the %s locale string is not passed:
https://github.com/DeadlyBossMods/DBM-Unified/blob/master/DBM-Core/localization.en.lua#L400
One example, with two different spells and different icons, and no spellname passed (which defaults to "bad" damage and wrong icon):
https://github.com/DeadlyBossMods/DBM-WotLK/blob/1e1803885425d59091953699422c987366d8fb7f/DBM-Raids-WoTLK/Coliseum/Jaraxxus.lua#L158-L159
image
This might be intended, but decided to write it here as well since retail mods all pass the spellname into gtfo Show.

@MysticalOS
Copy link
Member

Yeah I know, it's an inconsequential issue really as fixing it would require annoying effort of calling seticon ever time it's reused in every mod fore very spell. passing spellname though is something that is usually done though so a mod not doing that is worth fixing but that's about it

@MysticalOS
Copy link
Member

Fixed 3 spells in wraith raids to now show spell name

@Zidras
Copy link
Contributor Author

Zidras commented Aug 29, 2023

Why not use UpdateKey before every gtfo Show with multiple id? Is there anything wrong with it?
https://github.com/DeadlyBossMods/DBM-Unified/blob/master/DBM-Core/DBM-Core.lua#L9299

With this method, you can quickly update the icon on the fly:

function mod:SPELL_DAMAGE(_, _, _, _, destGUID, _, _, _, spellId, spellName)
	if (spellId == 66317 or spellId == 66320 or spellId == 66881) and destGUID == UnitGUID("player") and self:AntiSpam(3, 1) then	-- Fire Bomb (66317 is impact damage, not avoidable but leaving in because it still means earliest possible warning to move. Other 4 are tick damage from standing in it)
		specWarnGTFO:UpdateKey(spellId) -- added
		specWarnGTFO:Show(spellName)
		specWarnGTFO:Play("watchfeet")
	end
end

Might not be the cleanest implementation due to how gtfo is widespread and not unique, but is a low effort hotfix, don't you agree? At least for your future mods this is something that can be used.

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

No branches or pull requests

2 participants