GlyphList

GlyphList

2.4k Downloads

Inconsistent glyph matches

Meivyn opened this issue ยท 15 comments

commented

Name matching for glyphs produces inconsistent results due to the weirdness of the locales. One solution would be to grab all the glyph IDs and then match them using this:

local spellLink = C_SpellBook.GetSpellLinkFromSpellID(spellID)
local glyphId = tonumber(strmatch(spellLink, "%b::(%d+)"))

For example, this returns false (using strlower also doesn't work due to ' and โ€™ not matching either)

image

Unfortunately, that would require crafting/purchasing and applying all of the glyphs available in the game as I cannot find any information on current glyph IDs.

When I'm at it, you should probably make the function SetTitleText local.

commented

I'm testing this and there are still some inconsistencies with regards to spells that have their ID/name changed when you apply a glyph to them, for instance Shadowfiend -> Voidling. GetSpellLink(actionID) returns the generic Shadowfiend spell ID, not the modified Voidling spell ID. HasAttachedGlyph(actionID) correctly identifies that a glyph has been applied.

image

commented

Does it also returns 0 with C_SpellBook.GetSpellLinkFromSpellID(actionID)?

commented

Yes, although I think I've found a way to fix it:

local _, _, spellID = GetSpellBookItemName(slotIndex, BOOKTYPE_SPELL)
local spellLink = GetSpellLink(spellID)
local glyphId = tonumber(spellLink:match("%b::(%d+)"))

Instead of using actionID from GetSpellBookItemInfo(), I'm using spellID from GetSpellBookItemName() - this does appear to return the right IDs.

image

commented

You could probably just change the spellLink to this, so you don't make an extra call:

local spellLink = GetSpellLink(slotIndex, BOOKTYPE_SPELL)

That should work as well, since what's wrong here is the actionID returned by GetSpellBookItemInfo.

commented

You should probably filter out glyphs for a specific spec, since you can't retrieve their state either when the spec isn't active. Or remembering the state of those glyphs if you happen to switch spec instead of recreating back the entire glyph list.

When I'm at it, I would love to see a picture of what the glyph is doing in some way. So your add-on could entirely replace Wowhead for finding glyphs.

commented

Filtering by spec is still on my to-do list, I think the best way to do that would be to link the player specialisationID to the itemID in the same way that we're linking the glyphID to the itemID in the data file. It may also be possible to achieve the same results by going through the class and active spec tabs of the spellbook and seeing if any of the glyphs for that class can be applied to those spells using IsSpellValidForPendingGlyph(actionID), but that involves a lot of scanning and extra calls. With the extra dataset I can save the currentSpecID on GetCurrentSpec() and just retrieve the associated itemIDs for display if the spell exists on tab 2 or 3. When you switch spec the list would have to be recreated anyway as there may be different glyphs to show.

If I were to also add pictures for each glyph, wouldn't that make the addon a lot bigger/slower?

commented

I agree that that would be the better way to sort it. I'm just a bit apprehensive that with the removal of the glyph interface a few years ago there's no guarantee that these IDs will remain in the API for any length of time. For now, v1.2.4 fixes the name comparison issues, and I can work towards compiling a list of IDs in the longer term.

commented

Thanks for your report, as I can only test on the English client this is super helpful! I know that matching by name is far from ideal so I'll definitely have a look at changing it to ID matching instead.

I've also just released an update (v1.2.3) that moves SetTitleText to the mixin.

commented

I've spent some time on this and the glyph ID can't be retrieved from the spell link in the way you described. It is true that there is another number there instead of 0 if the spell has an active glyph, but that number is sadly not the glyph ID. I've not been able to determine yet what that number does refer to.

Blizzard themselves use GetCurrentGlyphNameForSpell(spellID) if they want to retrieve the name of the glyph that is applied to the spell, but it doesn't also return the glyph ID and there is no separate method to do so directly from the spell book. The only way to get this is to use GetItemInfoInstant(itemName), but this doesn't work in this case because of the differences in upper/lowercase letters and apostrophes. I have logged a bug with Blizzard to address these inconsistencies.

In the meantime, I will update the addon so the glyph names are standardised on the frFR locale before they get compared.

commented

Blizzard also uses links to retrieve the glyph ID, but it isn't the item ID or the spell ID related to those glyphs, as I said there is no know list of those as far as I'm aware and you will need to grab them by hand.

However, these links seem to be a little different from the links you can retrieve with GetSpellLink or C_SpellBook.GetSpellLinkFromSpellID, they look more like spell strings, but since this ID seems to be unique after testing all of my glyphs, why not use it?

commented

I'd love to be able to use it but the method that would've allowed me to retrieve the glyph name from that particular ID type was removed from the API in 7.0.3. Before then, glyphs themselves had IDs once learned (aside from the crafted/tradeable glyph item ID and associated spell ID) and they were displayed in a separate interface window. It looks like what you're referring to is a remnant of this old interface and thus no longer usable.

commented

I had a similar locales issue in my own add-on, and I hardcoded those IDs to solve the problem entirely. Doing instanceID = encounterJournalID, you could do the same with itemID = glyphID for glyphs, right?

Dealing with locales is a real pain, and new issues always arise when you rely on them for a match.

commented

This is what I am talking about: https://github.com/Ylaana/GlyphList/compare/main...Meivyn:patch-1?expand=1
It does however require retrieving all glyph IDs as I said earlier, but it works and is not unreliable like the locales are.

commented

I'm not sure if it happens without GetSpellLink(slotIndex, BOOKTYPE_SPELL), but it seems that there is still some times where this line is nil even with the Flyout change:

local spellLink = GetSpellLink(spellID)

So we should probably do that again:

local glyphId = spellLink and tonumber(spellLink:match("%b::(%d+)")) or 0

About the images, it would indeed make the add-on bigger, but that shouldn't be an issue in 2021 with the storage we have. About the loading times, I am not sure, probably shouldn't impact much if you scale the images properly.

commented

I believe the inconsistencies have now been fully fixed, and I've opened new tickets for the suggestions you made.