sersorrel / WhosTalking

See Discord voice activity indicators directly on your party list.
3 stars 5 forks source link

Adding IPC system #7

Closed Tischel closed 1 year ago

Tischel commented 1 year ago

That allows other plugins to query the discord user state for a given in-game character name.

I did this so I can add support for your plugin in DelvUI. I already have the DelvUI side ready as well: https://github.com/DelvUI/DelvUI/pull/1154

sersorrel commented 1 year ago

This looks super cool! tysm for writing both sides of the IPC, it's been on my to-do list for ages but I've not had the time to actually do it (in the same vein, I'm probably not going to get round to properly reviewing/merging til at least the weekend, sorry - in the meantime, have a pile of random comments before I forget them all)

how performant is plugin IPC? I guess this method requires DelvUI to poll the state of each player in the party list every frame, as opposed to Who's Talking tracking state across time and sending updates over IPC as necessary - how much or a performance impact does that polling have? (and does DelvUI display the other alliances too? or just your own 4/8 party members?)

...and is reading ClientState from within an IPC handler actually safe? I have a very vague memory of something like this coming up in goat place, the result of the conversation being (iirc) that you have to write IPC handlers defensively enough that they won't cause problems if run on a random thread or whatever, though I could be wrong about that and I have no idea if ClientState is ever actually unsafe to look at

also, do you have any thoughts on how to distribute the testing/maintenance for this, as it were? I have no idea which of us people will complain at if the integration ever breaks, and I also don't know how able I'll realistically be to test DelvUI to make sure I didn't break it before I release Who's Talking updates (I can try though!)

Tischel commented 1 year ago

I didn't notice a hit in performance when I tested this, but if anything the hit would be from DelvUI's side depending on how frequently it polls

The ClientState stuff, actually I've no idea about that. The testing I did was all calls from the Draw function call from Dalamud so in that case it should always be safe. But I can't speak for other use cases. Admittedly the only reason why I added that is because when I was testing alone and not in a party I wasn't getting any data, then I notcied you were handing the "solo party" case differently (using Connection.Self directly instead of checking the dictionary); and figured I'd have to do the same thing. We could remove this extra check with ClientState and it'd only break when someone is in a "solo" party, which doesn't sound like a big deal to me.

Regarding maintenance, I'll obviously take care of the DelvUI side, but I can also help here if needed. If you ever are pushing a release that you think might break something, just shoot me a DM and I can take a look before you release. That being said, I don't think there's anything that you can do that would break DelvUI because I have the relevant code inside a try/catch.

Tischel commented 1 year ago

Also here's an example of how it can look in DelvUI

image

It can also change the borders for each unit frame but I didn't have it enabled here.

sersorrel commented 1 year ago

ok, I'm merging this: if problems come up, we can tackle them as and when they do :) thank you again for putting in the effort to get this implemented!