Thoughts on the Inspection API

[toddimer]

Hello all,

I'm the author of a class profiling tool. At the core, it pidgeon-holes party members into "roles", before deciding how to best play nicely with those roles (by giving them blessings, choosing totems, etc). Picking the right role is based on data from their class, spec, dungeon role, pet selection, etc.

A lot of that critical player data is read from the Inspection API. Which isn't perfect; it's cumbersome, ambiguous, shared, and vague. To be fair...it works... but Blizzard taking steps to limit it's impact on servers tells me two things: (A) It's been WAY more popular/used than they expected, and (B) it's therefore a prime candidate for revision in Cataclysm.

I hope this thread can fuel some constructive discussion on if/how the Inspection API could be changed, to make our lives easier. and by "our", i mean "our users". Lean, efficient, well designed add-ons make users happy. We just need the tools to get there

[toddimer]

A few of my concerns with the current implementation:

Ambiguity: Asynchronous Completion

Goal: let addons know when the inspect API is ready, busy, abandoned, or failed.

Current Challenge: Authors must resort to timeouts, when the API fails or is abandoned. documentation on the proper timeout values isn't readily available

Kudos to whoever made this call asynchronous. That lets the server merrily collect data on my inspect target, without locking my UI. Fabulous. That process isn't as transparent as I'd like, though.

The current workflow is something like (a) request data via NotifyInspect, (b) cache the name of the player locally, then (c) wait for an event from INSPECT_TALENT_READY. If INSPECT_TALENT_READY is received (d) do stuff, and call ClearInspectPlayer when finished.

As far as i can tell, INSPECT_TALENT_READY will fail to fire if: the inspect target is bogus; another NotifyInspect is sent while the old one is processing; the inspect target is (or moves) out of inspect range while processing; or the server gets too many requests within a ~10 second window. "Fail to return" really sucks, because it's an undetectable event. It's entirely possible the call would return after, say, 20 seconds, or 5...that information isn't publicly known. So an author has to code an arbitrary timeout

Note that other addons can mess with each other. Firing NotifyInspect while another addon is waiting for INSPECT_TALENT_READY will severely mess with them. You can detect this behavior by hooking NotifyInspect, INSPECT_TALENT_READY, and ClearInspectPlayer; it's generally bad etiquette to call NotifyInspect between when anyone else has called notify inspect, and when they call ClearInspectPlayer. There's nothing to enforce this though... an addon can very well call back-to-back inspect data for multiple players without clearing the inspect player, effectively hogging the API.

To Solve: A unique ID could be assigned/returned from NotifyInspect, which is attached to the relevant INSPECT_TALENT_READY event. Add notifications for INSPECT_FAILED and/or INSPECT_ABANDONED, either as new events, or as a return code in INSPECT_TALENT_READY. I'm sure there are other things that could work.

[IQgryn]

It's possible to detect when someone else uses NotifyInspect during your waiting period and re-fire your event. Still a nuisance, though.

If Blizzard doesn't provide a better way to handle things, I may have to go write a LibQueueInspect that does this properly, once, so we don't have a bunch of separate implementations that step on each others' toes. Maybe I could get Ace or somebody to include it.

It would have to handle stuff not being returned, probably with a fairly short timeout, and re-send requests as necessary. I'd also have to add some sort of priority system so that an inspect-happy addon got pushed back in line if an addon that only requests a few now and again asks for an inspect.

Anyway, I'll give them some time first. Hopefully they'll get it right.

[IQgryn]

Another problem is it is hard to match which player you inspected with the notification. It would be much nicer to be able to inspect by name, and have that returned as the inspect ID, or something similar. It would be even better if they did name+requestID.

Oh, and it would be nice if NotifyInspect returned an error when you did too many requests at once, instead of silently failing. They could even do that client-side to avoid taxing the servers for it. It would let you handle another addon pumping out requests a little better.

[toddimer]

Originally Posted by IQgryn Another problem is it is hard to match which player you inspected with the notification. It would be much nicer to be able to inspect by name, and have that returned as the inspect ID, or something similar. It would be even better if they did name+requestID.

Shared resource makes kittens cry

Goal: let addons scan whatever inspect data is currently available.

Current Challenge: No facility to identify which unit the inspect data belongs to.

It's really awkward having to deal with the Inspect API, knowing that any addon at any time can interfere with your calls. Making a library would only fix half the problem; any other addon could still interfere with the Inspect API, and therefore mess with the entire Inspection library.

I think managing concurrent inspection requests is best accomplished in the root API itself. Not much would have to change: add either serialized requests, and pair "ready" notifications with unique identifiers (for the request or the unit)... or "not ready" events if the inspect API is busy. There just isn't enough transparency in the current implementation to allow either of those things. or, I'm a bad programmer.

It's also a real shame that the data returned from the Inspect API can't be shared between addons. "INSPECT_TALENT_READY" doesn't really help a concurrently running addon. It would know that somebody else is at some state in the inspect workflow...but it can't piggy back and/or use the currently available inspect data out of context; it needs a player GUID or a unitID. That data could be made available in a 3rd party library, or a revamped Inspect API implementation. without the identity data, though, sharing of inspect data results isn't really feasible :3

To Solve: include a unit GUID in the INSPECT_TALENT_READY notification; or, provide a function to query the talent group's owner: guid = TalentGroupOwner()

[ckaotik]

Originally Posted by IQgryn I may have to go write a LibQueueInspect that does this properly, once, so we don't have a bunch of separate implementations that step on each others' toes.

Definetely a good idea. Currently, as far as I know, there is LibTalentQuery which can be combined with LibGroupTalents, both written by Zeksie, the author of ZOMG!Buffs.
This only covers talents, though, and addons like GearScore need item information as well. Having one library keeping care of both might be a little too much, but having a base LibInspect and on top of that something for gear and talents would go a long way, I suppose.

Anyhow, I guess we just have to wait and see what Blizzard makes out of this.

[IQgryn]

Originally Posted by ckaotik Currently, as far as I know, there ist LibTalentQuery which can be combined with LibGroupTalents, both written by Zeksie, the author of ZOMG!Buffs.

Oh, I didn't know about that one. I'd probably just (help) update it to handle the timeouts properly. The gear can be grabbed at the same time as the talent data the way it appears to work currently, or I could add a function that gathered the appropriate data (specified with flags) and passed that to your callback function instead.

Actually, I rather like that second option. Why should each addon have to handle the complexities of the talent tree API when you could just return a table with subtables for each tree (indexed by both name and tree number) that had rank numbers for each talent (indexed by both name and talent number). Gear could be done as a table indexed by slot name and/or number, too.

...
-- the last two args specify whether to return gear/talent info, respectively
TalentQuery.RegisterCallback(self, "hasTitansGrip", false, true)
...
TalentQuery:Query(unit)

function hasTitansGrip(event, pname, prealm, unitid, talents, gear)
    return talents["Fury"]["Titan's Grip"] == 1
end

[ckaotik]

I just found that library last week myself. Also, besides talents and gear, there are also achievements and who knows what else. Maybe that's just me but I don't really like having one library that does too many things - e.g. I only care about talents, for buff suggestion, but it stores and manages gear as well. Depends on how much overhead this would create.