Questie

Questie

116M Downloads

QuestieQuest.lua's tooltip registration logic seems wasteful CPU-wise

Arcitec opened this issue ยท 3 comments

commented
_RegisterObjectiveTooltips = function(objective, questId)
    Questie:Debug(DEBUG_INFO, "Registering objective tooltips for " .. objective.Description)
    if objective.spawnList then
        for id, spawnData in pairs(objective.spawnList) do
            if spawnData.TooltipKey and (not objective.AlreadySpawned[id]) and (not objective.hasRegisteredTooltips) then
                QuestieTooltips:RegisterObjectiveTooltip(questId, spawnData.TooltipKey, objective)
            end
        end
    else
        Questie:Error("[QuestieQuest]: [Tooltips] ".. l10n("There was an error populating objectives for %s %s %s %s", objective.Description or "No objective text", questId or "No quest id", 0 or "No objective", "No error"));
    end
    objective.hasRegisteredTooltips = true
end

Step by step, what it does:

  1. Loop through the objective's list of spawns.
  2. For every spawn, check if it has a designated tooltip key, and if not objective.hasRegisteredTooltips.
  3. If so (key exists, and the objective has not been processed yet), register that sub-objective-key (spawn key) in the QuestieTooltips.lookupByKey table.
  4. Finally, set objective.hasRegisteredTooltips = true

Here is what it probably should be doing instead:

  1. Check if not objective.hasRegisteredTooltips then do the work, else do nothing at all.
commented

To clarify: Right now, the code is looping through a bunch of data and sub-tables, and taking a code path that contains an if-statement which ensures that the final code inside the loop never runs if objective.hasRegisteredTooltips = true. So I don't see any reason whatsoever to keep the old code in this function. There's no reason to begin a loop at all, if that value is already true.

commented

I totally agree, this looks absolutely wasteful.

The objective code of Questie in particular is in pretty bad shape and fragile as well. That is one of the main reasons we are very careful touching it and didn't start the required full rewrite already. There were other way more important topics as well towards the release of TBC.

But simply moveing the check a layer up looks quite easy to achieve. Would you mind creating a PR for this and test the change? ๐Ÿ™๐Ÿป

commented

@BreakBB I apologize for not getting involved after reporting a bunch of Questie issues. I quit WoW around the same time.

Everything came from me reverse engineering Questie to make a Plater nameplate addon that replaces Questie's nameplate code:

https://wago.io/0mQNBUdpV

I hope the issue reports are still a form of contribution even though I didn't have time to rewrite anything for Questie...