AdiButtonAuras

AdiButtonAuras

404k Downloads

Stop using LibSpellbook-1.0

mbattersby opened this issue ยท 2 comments

commented

This is not necessarily a good/the right idea, but I wanted to put it out there.

See also AdiAddons/LibSpellbook-1.0#12 and #388.

Whether by bug or design SPELLS_CHANGED is no longer a rare event, which makes the LibSpellbook-1.0 approach of keeping a spellbook info cache no longer workable without huge performance issues.

It should be possible to modify ABA to not require this approach. The uses of LibSpellbook-1.0 in ABA are:

  1. LibSpellbook-1.0:Resolve() - I think this can be replaced by a wrapper over GetSpellInfo() which is guaranteed to return for spellbook spells.
  2. LibSpellbook-1.0:HasSpells() - Not needed if not using LibSpellbook-1.0
  3. LibSpellbook-1.0:IsKnown() - ABA doesn't use the booktype param so IsSpellKnown might be a 1-1 replacement.
  4. LibSpellbook_Spells_Changed event. This is the problem but also the thing that doesn't work any more.

A replacement event needs to be found to re-run the builders, though there are no obvious candidates I can see.

The alternative is a change to approach where this is not required, where somehow each button is responsible for knowing if it changed and requesting a rules update for just itself. In that case there are hooks (ActionButton:UpdateAction) or events (ACTIONBAR_SLOT_CHANGED) that could work.

commented

I don't see this as a possibility tbqh. LSB original purpose was to abstract away inconsistencies in APIs like IsPlayerSpell and IsSpellKnown. Since then it also covered every odd spell system Blizz threw at us like artifacts and soulbinds and also handled other oddities like upgrade spell ranks not being in the spellbook.

Besides that LSB abstracts changes to the known spells away. Interested parties only need to listen to a single callback instead of figuring out events and cases themselves. ABA uses this to evaluate which rules to activate. Without that, all rules will need to be run on UNIT_AURA which might be much more of a hog than what we currently have.

We need to find out how the empowered action in torghast behaves and how to trotle it. Another possibility would be use the SpellAdded|Removed callbacks instead of SpellsChanged. Or the cheap one - unregister SPELL_CHANGED in torghast :)

commented

I will close this as I don't consider this an option. Moving towards a model where rules are updated based on the availability of their providers rather than in bulk is probably the best solution in this case.