karashiiro / TextToTalk

Chat TTS plugin for Dalamud. Has support for triggers/exclusions, several TTS providers, and more!
MIT License
48 stars 28 forks source link

Added a new Race field for SayRequest messages, and WebSocket messages. #201

Closed Cidan closed 7 months ago

Cidan commented 7 months ago

This PR adds a new field to SayRequest and the associated WebSocket JSON payload for the Race of the speaker. This resolves #200.

Cidan commented 7 months ago

@karashiiro PTAL. I noticed you previously used unsafe character pointers to get gender for a character. I would also like to remove that code and instead use the safer Customize field, which significantly reduces the complexity of gender lookups and eliminates the need for unsafe.

If that's alright, I'll go ahead and make the change in this PR, along with some more properties I want to add.

karashiiro commented 7 months ago

LGTM overall, thanks for this!

I would also like to remove that code and instead use the safer Customize field, which significantly reduces the complexity of gender lookups and eliminates the need for unsafe.

This would be appreciated, I added the original pointer dereferences before Customize had a managed wrapper, which is why it's like that. Now that Dalamud provides a better API around that, these can be removed.

Cidan commented 7 months ago

Thanks! Unfortunately upon further inspection, it looks like unsafe can't be removed as the model ID is not available via other means I can see.

I'll add some more features here and send this up for an actual review soon.

Cidan commented 7 months ago

So after talking to a bunch of Dalamud dev folks, it turns out using DrawData is the preferred way of getting this data. This is because other addons might change model information, which wouldn't be reflected in the GameObject, causing mis-matched voices for players with draw data plugins.

I've adjusted the code as such, and added an Age type, based on the model data.

karashiiro commented 7 months ago

This is because other addons might change model information, which wouldn't be reflected in the GameObject, causing mis-matched voices for players with draw data plugins.

I was thinking about this while adding the NPC ID field, it might be better to just use that instead of model IDs. Model IDs were a bit of a hack that wound up working when I was first trying to implement the gender overrides. The only issue with that is I'm not sure if enemies use ENpcBase, so it'd only work for regular NPCs.

Anyways, probably out of scope for this, I think.

Cidan commented 7 months ago

I think for now it probably makes sense to keep doing what you're doing here.

I've added a new enum for body type, renamed the field to body type, and let the JSON mapping take care of converting the name to a string (I had no idea this would work, neat!).

Cidan commented 7 months ago

I think this is all for now. I have some other ideas, but I want to iterate on what's already implemented.

karashiiro commented 7 months ago

LGTM, will give it another pass though and merge within the next few hours.