Questie

Questie

116M Downloads

QuestieNameplate.lua's "UpdateNameplate()" attempts to update nameplates too soon, leads to bugged plates.

Arcitec opened this issue ยท 3 comments

commented

Very simple bug report:

  1. Event handler triggers the "quest progress changed" event.
  2. It in turn instantly calls QuestieNameplate:UpdateNameplate()
  3. Which in turn instantly loops through all active nameplates and runs the _GetValidIcon() function for them, and changes icon based on its result.
  4. Which in turn calls the :Update() function to refresh the latest quest log data for each objective that's being tracked.
  5. Therein lies the problem. This is all happening too fast. By the time the first plates are being updated, the _GetValidIcon() still returns an icon. And by the time the final plates are being updated, it returns nothing.

This leads to lingering icons that "hang on" to some of the NPCs.

Let's say you are surrounded by 4 NPCs. All have the "loot item" icon. You kill 1 of them and loot it.

The code will then loop through the remaining 3 living NPCs, and it will look something like this:

  • NPC 1: _GetValidIcon() result = Keep "Loot" icon.
  • NPC 2: _GetValidIcon() result = Keep "Loot" icon.
  • NPC 3: _GetValidIcon() result = Delete icon.

So two of the three NPCs still have lingering icons.

Solution: Perhaps make QuestieNameplate:UpdateNameplate() start a 1 second timer of a processing callback (so that the quest log has time to refresh its cached state in the client) and THEN loop through all plates and update them.

In fact, an improved solution: Start ONE 1-second timer. And if any other UpdateNameplate() events happen while that timer is queued, de-queue it and start a new 1-second timer. Meaning, if the player loots multiple mobs really quickly, the nameplates won't be processed and refreshed until 1 second AFTER the final quest log update event. This cuts down on the amount of wasteful processing.

commented

The optimal solution can be achieved via https://wowpedia.fandom.com/wiki/API_C_Timer.NewTicker (read the C_Timer.NewTimer() notes), which creates a single-tick, cancelable timer.

  1. Just :Cancel() any previous timer (according to Blizzard, it's always safe to call that as much as you want, even after the timer has already finished its execution). Do this by having a single, global variable for your "updatenameplate timer".
  2. Create a new 1-second timer, and write its returned handle to the single, global variable. To make this timer code/call extra efficient, make its callback a pre-defined function so that it doesn't have to create anonymous callback functions each time you call NewTimer().
  3. Repeat step 1 and 2 every time the QuestieNameplate:UpdateNameplate() function fires.
  4. Eventually, when the quest log event updates have stopped firing, the final timer will be allowed to finish its countdown and will execute the nameplate updates. And the bug will be fixed!

Note: It's likely that the bug can also be fixed by simply using a 1-frame (0 ms) "C_Timer.After" instead, to simply update nameplates 1 frame after the quest log update event fired. But I still suggest a longer timer (1 second), so that you don't waste energy constantly re-processing all unitframes whenever the player is mass-looting corpses and rapidly triggering tons of "quest log update". It's better to just wait a proper amount of time, and 1 second seems logical.

The actual bugfix would look something like this:

function QuestieNameplate.SomeCallbackToUpdateAllNameplates()
    -- the code that used to be in UpdateNameplate()
end

local deferred_update_timer = nil

function QuestieNameplate:UpdateNameplate()
    if (deferred_update_timer) then
        deferred_update_timer:Cancel() -- Safe to call multiple times.
    end
    deferred_update_timer = NewTimer(1, QuestieNameplate.SomeCallbackToUpdateAllNameplates)
end
commented

After some testing, 1 second does feel a tiny bit sluggish. 0.5 seconds also fixes the bug and feels completely perfect. Therefore I recommend NewTimer(0.5, instead. Fixing the nameplate icon bug is the main concern, so 0.5 is proper. The reduction of processing when mass-looting (which 1 second handles better) was just a bonus.

commented

This was fixed with the refactoring of our event system in 997d1b4