Do you think Libstub should have been made? After all, how many lines of codes does it take to version your own library? And then just put the entire library in one object in _G (instead of putting it in a table entry inside LibStub and then having to do a function call to retrieve it).
And?
Basically you're saying we should start making a library for every obscure little thing even if it's not needed or can be done in less code with less work? We already went through that mess with people making a library for every single thing without asking themselves "How many people actually need this?"
Can you actually name me how many addons need to inspect the entire raid for talents? It's fairly obscure, mostly things like Scrub it's not worth making a library out of it.
aX_RaidStatus, GridLayoutPlus, MoBuffs, PitBull, RaidBuffStatus, Scrub, and probably others all do or can scan the raid's talents for various reasons. I'm personally running 5 of those I just named. No, you don't need a library for talent inspection, any more than you need a library for localizing boss names, but if you have 5+ addons doing raid-wide talent inspection, I don't see how sharing this code in a library is any way not "worthwhile".
Can you actually name me how many addons need to inspect the entire raid for talents? It's fairly obscure, mostly things like Scrub it's not worth making a library out of it.
You do not need a library for everything the below is 45 lines I did this in 10 minutes, and I don't even need any additional code from LibStub or CallbackHandler-1.0 that adds extra resources or complexity, it's not complicated, and not enough addons are going to actually need something like inspect the entire raid (especially now that you're limited to 30 yards on talents) to make it a worthwhile library.
Any of the buff checking mods (there are about a dozon -- MoBuffs, XRaidBuffStatus, etc.). Any of the mods for group managment (GridLayoutPlus, etc.). Etc.
Your code is pretty good. But it could be better. Two things I noticed from a very quick glance. One, you never unhook the OnUpdate event handler, so even though it's small and fairly efficient, your event handler is a constant CPU leak. For another, inspectSent is never reset to false, so after if receives a response for the an inspect, if no other units are queue, it stops working (sendInspect(unit) queues up the units, but NotifyInspect() will never be called.)
Like I said, the difficulty is in the details, and having a good library like Peragor's really helps so that you don't have to reinvent the wheel each time.
Any of the buff checking mods (there are about a dozon -- MoBuffs, XRaidBuffStatus, etc.). Any of the mods for group managment (GridLayoutPlus, etc.). Etc.
Your code is pretty good. But it could be better. Two things I noticed from a very quick glance. One, you never unhook the OnUpdate event handler, so even though it's small and fairly efficient, your event handler is a constant CPU leak. For another, inspectSent is never reset to false, so after if receives a response for the an inspect, if no other units are queue, it stops working (sendInspect(unit) queues up the units, but NotifyInspect() will never be called.)
Like I said, the difficulty is in the details, and having a good library like Peragor's really helps so that you don't have to reinvent the wheel each time.
-Pach
Yes, shockingly enough thats what happens when you do a 10 minute write up to prove a point, I never said it was functional it's pointing out you don't need it.
Do you understand how OnUpdate works? When the frame is hidden, it stops calling OnUpdate, theres no CPU leak it only runs when it's needed.
Yes, shockingly enough thats what happens when you do a 10 minute write up to prove a point, I never said it was functional it's pointing out you don't need it.
Do you understand how OnUpdate works? When the frame is hidden, it stops calling OnUpdate, theres no CPU leak it only runs when it's needed.
Yes. Sorry I didn't catch your frame.hide/show. Shockingly enough, that's what happens when you do a 30 second review of code. I never said my review was complete. The review was meant to point out the advantages of having a library like Peragor's. :)
In fact, if anything a library that saves talents makes far more sense then one that does queuing.
All addons you mentioned would benefit more from something like what Remembrance does, especially with 2.4 changes making it so they have to be within 30 yards. Store their talent points, class, "compressed points" (aka Blizzard talent URL type of thing) and the time you saved the data. Then you can set certain features, say if their within inspect range and data is over 24 hours old you get new data. If an addon wants to figure out spec then they can pull from that data instead of making it more complicated with queuing, and unitid's potentially changing (Which is going to be an issue at times).
The fact that you still can't rely on unitid's being consistent especially 30s-60s later if you have 50 addons doing queuing would make saving and pulling from a central list better then the current queue system.
If you're going to review it and say somethings wrong, look it over at least then instead of spending 5 seconds and going "Nope, this is wrong you fail"
If you're going to review it and say somethings wrong, look it over at least then instead of spending 5 seconds and going "Nope, this is wrong you fail"
The main flaw I pointed out with inspectSent not being reset (which means it stops working), i think is still valid. :)
And yes, a lib that caches talents (short term) would be useful.
And my main point is still valid -- while it may be trivial to someone as talented as you Shadowed, I think it's still something that warrants a library.
Would likely have to redo things but the basic idea would be something like.
Whenever someone in your raid comes within range, inspect them and store the data using name-server in a regular table, once that inspect is sent recheck and see if people are still in range and repeat that until you can't find anyone else. If someone new joins the raid then you wait until their in range and you inspect them as well.
[edit note] You may want to get fancy and if they leave the zone and go back to a major city, you clear them and wait until you see them again to inspect in-case they respec mid raid.
I haven't really looked at how the addons work yet, but I assume some of them have to raid until the entire raid is inspected so you'd likely want an event fired basically saying were done in which case they can run their code. For things like wanting to know who has X buff, you add something like lib:GetTalents(name, server) which could return something like "##, ##, ##, <compressed points>", if they didn't have data yet it's more likely that the person hasn't come into our range so adding something like lib:SendRequest(unit) wouldn't do much good. If you really wanted you could always add an event that fires each time someone is inspected, but unless I'm mistaken you really care about the raid finished being inspected more then you do specific people.
One advantage to this kind of system is it speeds up inspection for everyone, Scrub can just pull the cached data and instantly tell you about the entire raid, and then you don't have to deal with multiple addons inspecting the same kind of idea.
The reason you'd be storing things as name-server instead of unitid is because you can't rely on raid# being the same 5 minutes later if someone else asks for data, so you just use name-server as a storage, that way it'll work for things like battlegrounds.
And yes I did come off harsh in the other posts, heres your constructive suggestive one :p
Basically you're saying we should start making a library for every obscure little thing even if it's not needed or can be done in less code with less work? We already went through that mess with people making a library for every single thing without asking themselves "How many people actually need this?"
My point is LibStub is unnecessary, and shouldn't have been made in the first place.
My point is LibStub is unnecessary, and shouldn't have been made in the first place.
Frankly, I don't see what the point was to libstub at all, other than a political one.
Now with all these micro-libraries things are even more complicated than before. Notice Ace3 has literally twice the number of libraries in the base distro, and they are so specific, that its easy to just say screw it and use your own. I think all these micro-libraries have created more bloat instead of reducing it.
Don't like the library, don't use it. Like the library, use it. Not sure how the existance of the library is of any threat to anyone. It at least serves as an example of how to fix a lot of the problems with Blizzard's talent inspect api.
On a more important topic... Is it better to Hide/Show the frame like in Shadowed's example or to SetScript OnUpdate to a function when needed (and nil it when unneeded). Seems like both ways do the exact same thing, but I wasn't sure if one had more advantages over the other. I will switch to the hide/show method if there is a case that it would make things more efficient.
Don't like the library, don't use it. Like the library, use it. Not sure how the existance of the library is of any threat to anyone. It at least serves as an example of how to fix a lot of the problems with Blizzard's talent inspect api.
On a more important topic... Is it better to Hide/Show the frame like in Shadowed's example or to SetScript OnUpdate to a function when needed (and nil it when unneeded). Seems like both ways do the exact same thing, but I wasn't sure if one had more advantages over the other. I will switch to the hide/show method if there is a case that it would make things more efficient.
Arguing is pretty much all that is really done anymore on the forums.
Use whichever method you like better, show/hide or SetScript
That's quite a big amount of data to be stored, I propose the format should just be the same as a Blizzard URL to their talent planner + GUID + class + name-server (you might want to retrieve the talents of someone you don't have the GUID of, say after a relog). A Blizzard URL-style string will allow data to be easily imported/exported as well as for an addon like Talented to harness.
After a patch that changes talents significantly to cause everyone of that class to have their talents unlearned, the library should also purge all data of that class.
Data should preferably be packed in a single string for storage/memory and only unpacked on request as data requesting is expected to be quite sparse and not highly used such as in an OnUpdate.
That's quite a big amount of data to be stored, I propose the format should just be the same as a Blizzard URL to their talent planner + GUID + class + name-server (you might want to retrieve the talents of someone you don't have the GUID of, say after a relog). A Blizzard URL-style string will allow data to be easily imported/exported as well as for an addon like Talented to harness...
... and obviously, you need to store date/time of when the talent data was retrieved.
What I'm currently thinking is we make a "LibTalentCache" that uses LibTalentQuery. That way people don't have to have a memory hogging library if they don't want it. Something along the lines of how LibMobHealth is working, so that it auto-prunes the info when a max number of units cached is hit.
If we were to use the blizzard armory style of storage, some addons would have to have static information about the talent trees. If we stored ALL the information (talentname, current rank, max rank, icon, etc), then all data would be available and the addons would still work after a talenttree change patch or expansion. The obvious bad thing would be the data would be extremely bloated with info that most addons don't need.
You're complicating it a bit more then you need to, you don't need to do inter library dependencies. Because it's a library, we really don't want to be storing data global data like that, if you want to be storing global data then don't make it an embeddable library, which isn't a bad thing. If you're following the proper usage of minor/major versions.
You don't need to do any fancy queuing, especially because they have to be within 30 yards you can build your queuing in with your ranged checking in a much simpler way. This doesn't handle unitid changing, it's a quick hacked together example.
The only people who want to get EVERY single piece of information are ones that want to re-build the entire tree visually, you'd be better off making them handle their own re-caching which you could make easier by adding a callback that we trigger whenever we inspect a single person. That way you only have to store full points, class, compressed tree. If you want to store really basic information like max points, name, icon you can make a simple class data cache.
local inspectData = {}
local inspectSent, inspectUnit, inspectGUID, inspectClass
local raidids = {}
local lib = {}
function lib:GetData(GUID)
if( not inspectData[GUID] ) then
return nil
end
local talents, class, compressedTree = string.split(":", inspectData[GUID])
return talents, class, compressedTree
end
-- Handle inspection
for i=1, 40 do
raidids[i] = "raid" .. i
end
local function checkNextInspect()
for id, unit in pairs(raidids) do
if( CheckInteractDistance(unit, 1) ) then
inspectSent = true
inspectUnit = unit
inspectGUID = UnitGUID(unit)
inspectClass = select(2, UnitClass(unit))
inspectTimeout = 3
NotifyInspect(unit)
break
end
end
end
-- Handle timeouts
local inspectTimeout = 0
local rangeTimeout = 0
local function OnUpdate(self, elapsed)
-- Inspect sent, time out after 3 times
if( inspectSent ) then
inspectTimeout = inspectTimeout + elapsed
-- Inspect timed out, meaning we have to re-add it to the list
if( inspectTimeout >= 3 ) then
inspectSent, inspectGUID, inspectClass, inspectUnit = nil
end
end
if( not inspectSent ) then
-- Timeout on checking if anyones nearby to inspect
rangeTimeout = rangeTimeout + elapsed
if( rangeTimeout >= 1 ) then
rangeTimeout = 0
checkNextInspect()
end
end
end
-- Hook NotifyInspect, if it's a unitid we didn't send then pause inspect
hooksecurefunc("NotifyInspect", function(unit)
if( unit ~= inspectUnit ) then
inspectSent, inspectGUID, inspectClass, inspectUnit = nil
end
end)
local function OnEvent(self, event)
-- Got stuff!
if( event == "INSPECT_TALENT_READY" and inspectSent ) then
local _, _, firstPoints = GetTalentTabInfo(1, true)
local _, _, secondPoints = GetTalentTabInfo(2, true)
local _, _, thirdPoints = GetTalentTabInfo(3, true)
local talents = string.format("%d/%d/%d", firstPoints or 0, secondPoints or 0, thirdPoints or 0)
-- Compress the entire tree into 63 char or so format, the same one used by Blizzards talent calculator
local compressedTree = ""
for tab=1, GetNumTalentTabs(true) do
for talent=1, GetNumTalents(tab, true) do
local name, path, tier, column, currentRank, maxRank = GetTalentInfo(tab, talent, true)
compressedTree = compressedTree .. (currentRank or 0)
end
end
inspectData[inspectGUID] = string.format("%s:%s:%s", talents, inspectclass, compressedTree)
-- Remove this unit from the queue
for id, unit in pairs(raidids) do
if( UnitGUID(unit) == inspectGUID ) then
raidids[id] = nil
break
end
end
inspectSent, inspectGUID, inspectClass, inspectUnit = nil
-- Stop scanning while outside of a raid
elseif( event == "RAID_ROSTER_UPDATE" ) then
if( GetNumRaidMembers() == 0 ) then
frame:Hide()
else
frame:Show()
end
-- Disable scans while in combat
elseif( event == "PLAYER_REGEN_ENABLED" and GetNumRaidMembers() > 0 ) then
frame:Show()
elseif( event == "PLAYER_REGEN_DISABLED" and GetNumRaidMembers() > 0 ) then
frame:Hide()
end
end
-- Setup
local frame = CreateFrame("Frame")
frame:RegisterEvent("PLAYER_REGEN_ENABLED")
frame:RegisterEvent("PLAYER_REGEN_DISABLED")
frame:RegisterEvent("INSPECT_TALENT_READY")
frame:RegisterEvent("RAID_ROSTER_UPDATE")
frame:SetScript("OnUpdate", OnUpdate)
frame:SetScript("OnEvent", OnEvent)
frame:Hide()
Rollback Post to RevisionRollBack
To post a comment, please login or register a new account.
And?
Basically you're saying we should start making a library for every obscure little thing even if it's not needed or can be done in less code with less work? We already went through that mess with people making a library for every single thing without asking themselves "How many people actually need this?"
aX_RaidStatus, GridLayoutPlus, MoBuffs, PitBull, RaidBuffStatus, Scrub, and probably others all do or can scan the raid's talents for various reasons. I'm personally running 5 of those I just named. No, you don't need a library for talent inspection, any more than you need a library for localizing boss names, but if you have 5+ addons doing raid-wide talent inspection, I don't see how sharing this code in a library is any way not "worthwhile".
Any of the buff checking mods (there are about a dozon -- MoBuffs, XRaidBuffStatus, etc.). Any of the mods for group managment (GridLayoutPlus, etc.). Etc.
Your code is pretty good. But it could be better. Two things I noticed from a very quick glance. One, you never unhook the OnUpdate event handler, so even though it's small and fairly efficient, your event handler is a constant CPU leak. For another, inspectSent is never reset to false, so after if receives a response for the an inspect, if no other units are queue, it stops working (sendInspect(unit) queues up the units, but NotifyInspect() will never be called.)
Like I said, the difficulty is in the details, and having a good library like Peragor's really helps so that you don't have to reinvent the wheel each time.
-Pach
Yes, shockingly enough thats what happens when you do a 10 minute write up to prove a point, I never said it was functional it's pointing out you don't need it.
Do you understand how OnUpdate works? When the frame is hidden, it stops calling OnUpdate, theres no CPU leak it only runs when it's needed.
Yes. Sorry I didn't catch your frame.hide/show. Shockingly enough, that's what happens when you do a 30 second review of code. I never said my review was complete. The review was meant to point out the advantages of having a library like Peragor's. :)
-Pach
All addons you mentioned would benefit more from something like what Remembrance does, especially with 2.4 changes making it so they have to be within 30 yards. Store their talent points, class, "compressed points" (aka Blizzard talent URL type of thing) and the time you saved the data. Then you can set certain features, say if their within inspect range and data is over 24 hours old you get new data. If an addon wants to figure out spec then they can pull from that data instead of making it more complicated with queuing, and unitid's potentially changing (Which is going to be an issue at times).
The fact that you still can't rely on unitid's being consistent especially 30s-60s later if you have 50 addons doing queuing would make saving and pulling from a central list better then the current queue system.
If you're going to review it and say somethings wrong, look it over at least then instead of spending 5 seconds and going "Nope, this is wrong you fail"
The main flaw I pointed out with inspectSent not being reset (which means it stops working), i think is still valid. :)
And yes, a lib that caches talents (short term) would be useful.
And my main point is still valid -- while it may be trivial to someone as talented as you Shadowed, I think it's still something that warrants a library.
-Pach
Whenever someone in your raid comes within range, inspect them and store the data using name-server in a regular table, once that inspect is sent recheck and see if people are still in range and repeat that until you can't find anyone else. If someone new joins the raid then you wait until their in range and you inspect them as well.
[edit note] You may want to get fancy and if they leave the zone and go back to a major city, you clear them and wait until you see them again to inspect in-case they respec mid raid.
I haven't really looked at how the addons work yet, but I assume some of them have to raid until the entire raid is inspected so you'd likely want an event fired basically saying were done in which case they can run their code. For things like wanting to know who has X buff, you add something like lib:GetTalents(name, server) which could return something like "##, ##, ##, <compressed points>", if they didn't have data yet it's more likely that the person hasn't come into our range so adding something like lib:SendRequest(unit) wouldn't do much good. If you really wanted you could always add an event that fires each time someone is inspected, but unless I'm mistaken you really care about the raid finished being inspected more then you do specific people.
One advantage to this kind of system is it speeds up inspection for everyone, Scrub can just pull the cached data and instantly tell you about the entire raid, and then you don't have to deal with multiple addons inspecting the same kind of idea.
The reason you'd be storing things as name-server instead of unitid is because you can't rely on raid# being the same 5 minutes later if someone else asks for data, so you just use name-server as a storage, that way it'll work for things like battlegrounds.
And yes I did come off harsh in the other posts, heres your constructive suggestive one :p
-
View User Profile
-
View Posts
-
Send Message
Curse PremiumMy point is LibStub is unnecessary, and shouldn't have been made in the first place.
-
View User Profile
-
View Posts
-
Send Message
Curse PremiumFrankly, I don't see what the point was to libstub at all, other than a political one.
Now with all these micro-libraries things are even more complicated than before. Notice Ace3 has literally twice the number of libraries in the base distro, and they are so specific, that its easy to just say screw it and use your own. I think all these micro-libraries have created more bloat instead of reducing it.
Don't like the library, don't use it. Like the library, use it. Not sure how the existance of the library is of any threat to anyone. It at least serves as an example of how to fix a lot of the problems with Blizzard's talent inspect api.
On a more important topic... Is it better to Hide/Show the frame like in Shadowed's example or to SetScript OnUpdate to a function when needed (and nil it when unneeded). Seems like both ways do the exact same thing, but I wasn't sure if one had more advantages over the other. I will switch to the hide/show method if there is a case that it would make things more efficient.
-
View User Profile
-
View Posts
-
Send Message
Curse PremiumArguing is pretty much all that is really done anymore on the forums.
Use whichever method you like better, show/hide or SetScript
-
View User Profile
-
View Posts
-
Send Message
Curse PremiumWhat would be the ideal cache data structure be? There is a lot of info:
NumTalentTabs
TalentTreeName, TalentTreeIconTexturePath, TalentTreePointsSpent, TalentTreeBackgroundImage
NumTalents
TalentName, TalentIconTexturePath, TalentTier, TalentColumn, TalentCurrentRank, TalentMaxRank
The current index into the cache should probably be the unitname(-server). Should it be changed to the GUID in 2.4 (this Tuesday??)
Would we ever need to re-inspect after the data is cached? (say someone in your raid needs to respec while remaining in the raid)
Any other thoughts/concerns that we should discuss?
-
View User Profile
-
View Posts
-
Send Message
Curse PremiumMaybe try to have a look at what Talented does to store talents.
-
View User Profile
-
View Posts
-
Send Message
Curse PremiumAfter a patch that changes talents significantly to cause everyone of that class to have their talents unlearned, the library should also purge all data of that class.
Data should preferably be packed in a single string for storage/memory and only unpacked on request as data requesting is expected to be quite sparse and not highly used such as in an OnUpdate.
... and obviously, you need to store date/time of when the talent data was retrieved.
-Pach
If we were to use the blizzard armory style of storage, some addons would have to have static information about the talent trees. If we stored ALL the information (talentname, current rank, max rank, icon, etc), then all data would be available and the addons would still work after a talenttree change patch or expansion. The obvious bad thing would be the data would be extremely bloated with info that most addons don't need.
You don't need to do any fancy queuing, especially because they have to be within 30 yards you can build your queuing in with your ranged checking in a much simpler way. This doesn't handle unitid changing, it's a quick hacked together example.
The only people who want to get EVERY single piece of information are ones that want to re-build the entire tree visually, you'd be better off making them handle their own re-caching which you could make easier by adding a callback that we trigger whenever we inspect a single person. That way you only have to store full points, class, compressed tree. If you want to store really basic information like max points, name, icon you can make a simple class data cache.
local inspectData = {} local inspectSent, inspectUnit, inspectGUID, inspectClass local raidids = {} local lib = {} function lib:GetData(GUID) if( not inspectData[GUID] ) then return nil end local talents, class, compressedTree = string.split(":", inspectData[GUID]) return talents, class, compressedTree end -- Handle inspection for i=1, 40 do raidids[i] = "raid" .. i end local function checkNextInspect() for id, unit in pairs(raidids) do if( CheckInteractDistance(unit, 1) ) then inspectSent = true inspectUnit = unit inspectGUID = UnitGUID(unit) inspectClass = select(2, UnitClass(unit)) inspectTimeout = 3 NotifyInspect(unit) break end end end -- Handle timeouts local inspectTimeout = 0 local rangeTimeout = 0 local function OnUpdate(self, elapsed) -- Inspect sent, time out after 3 times if( inspectSent ) then inspectTimeout = inspectTimeout + elapsed -- Inspect timed out, meaning we have to re-add it to the list if( inspectTimeout >= 3 ) then inspectSent, inspectGUID, inspectClass, inspectUnit = nil end end if( not inspectSent ) then -- Timeout on checking if anyones nearby to inspect rangeTimeout = rangeTimeout + elapsed if( rangeTimeout >= 1 ) then rangeTimeout = 0 checkNextInspect() end end end -- Hook NotifyInspect, if it's a unitid we didn't send then pause inspect hooksecurefunc("NotifyInspect", function(unit) if( unit ~= inspectUnit ) then inspectSent, inspectGUID, inspectClass, inspectUnit = nil end end) local function OnEvent(self, event) -- Got stuff! if( event == "INSPECT_TALENT_READY" and inspectSent ) then local _, _, firstPoints = GetTalentTabInfo(1, true) local _, _, secondPoints = GetTalentTabInfo(2, true) local _, _, thirdPoints = GetTalentTabInfo(3, true) local talents = string.format("%d/%d/%d", firstPoints or 0, secondPoints or 0, thirdPoints or 0) -- Compress the entire tree into 63 char or so format, the same one used by Blizzards talent calculator local compressedTree = "" for tab=1, GetNumTalentTabs(true) do for talent=1, GetNumTalents(tab, true) do local name, path, tier, column, currentRank, maxRank = GetTalentInfo(tab, talent, true) compressedTree = compressedTree .. (currentRank or 0) end end inspectData[inspectGUID] = string.format("%s:%s:%s", talents, inspectclass, compressedTree) -- Remove this unit from the queue for id, unit in pairs(raidids) do if( UnitGUID(unit) == inspectGUID ) then raidids[id] = nil break end end inspectSent, inspectGUID, inspectClass, inspectUnit = nil -- Stop scanning while outside of a raid elseif( event == "RAID_ROSTER_UPDATE" ) then if( GetNumRaidMembers() == 0 ) then frame:Hide() else frame:Show() end -- Disable scans while in combat elseif( event == "PLAYER_REGEN_ENABLED" and GetNumRaidMembers() > 0 ) then frame:Show() elseif( event == "PLAYER_REGEN_DISABLED" and GetNumRaidMembers() > 0 ) then frame:Hide() end end -- Setup local frame = CreateFrame("Frame") frame:RegisterEvent("PLAYER_REGEN_ENABLED") frame:RegisterEvent("PLAYER_REGEN_DISABLED") frame:RegisterEvent("INSPECT_TALENT_READY") frame:RegisterEvent("RAID_ROSTER_UPDATE") frame:SetScript("OnUpdate", OnUpdate) frame:SetScript("OnEvent", OnEvent) frame:Hide()