[Accessibility] Text to Speech #2487
Conversation
dcvz
force-pushed
the
feature/tts
branch
3 times, most recently
from
d13e736
to
61dfb9d
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Overall this is looking really great! It's awesome to see how well the GI hooks can be utilized!
Some notes:
accessibility.cppseems to be only TTS stuff, might be worth using a more specific name there- there are a lot of cstring methods, leading to quite complex strdup etc. ops combined with a bunch of calls to std::string (including
.c_str()), would it be possible to keep things using std::string in most places instead? - this one i mentioned in a comment earlier, but would it be possible to register/deregister hooks on cvar change instead of just doing a cvar check in them and early returning?
there are also a few places throughout the PR where it's not clear how the change is related to the TTS stuff. I didn't see any changes like that I was unhappy with, and I completely understand the desire to clean up, but it did make it a little harder to wrap my head around everything this PR is doing
that being said, that may be more the fault of the GH PR review UI lagging with such a big one, if everything was loading smoothly this probably would have been easier to navigate
| "7": "Shadow Temple", | ||
| "8": "Bottom of The Well", | ||
| "9": "Ice Cavern", | ||
| "10": "", // Stairs to Ganondorf's Lair (No title card) |
There was a problem hiding this comment.
is there a plan to add speech for the scenes without title cards?
There was a problem hiding this comment.
I'm not sure.. might be worth it down the line. I left the possibility
| GameInteractor::Instance->RegisterGameHook<GameInteractor::OnSceneInit>([](int16_t sceneNum) { | ||
| if (!CVarGetInteger("gA11yTTS", 0)) return; |
There was a problem hiding this comment.
if i'm understanding this correctly, it means the hook is always registered, but it just doesn't do anything if we don't have the cvar set
i wonder if it makes more sense to have toggling the cvar register/deregister the hooks instead (just a thought)
There was a problem hiding this comment.
That is correct, I thought about this.. but I'm not sure. In general I'm more of a fan of mods being registered in one central place and being enabled on start. Rather than having to hook in each mod separately. The early return makes this not cost us much.. but open to other architectural thoughts.
The way I see it is you make a mod/feature, you register it via the InitMods method and don't have to worry about the game code at all.
There was a problem hiding this comment.
hmm, i see the benefit of that approach, but I also think needing to add a cvar check and early return to every hook method feels like a strange pattern
probably not something that needs to be addressed for this PR, but it'd be good to think about possible other options that allow us to init/deinit hooks in a clean way
There was a problem hiding this comment.
Ah I know what we can do! We can keep the same API but let you specify a cvar that the init depends on. The mod engine can take care of initing and deiniting mods when cvar they've identified themselves with change.
There was a problem hiding this comment.
I'll do this part as a follow up though - I have a few thoughts as well about other hooks being used
There was a problem hiding this comment.
Might be a good idea to pass the text as a std::string to the backend speech thread here: https://github.com/Kenix3/libultraship/blob/aa068f9196f79f466ee0fce5e52aac90e31709c9/src/speechsynthesizer/SAPISpeechSynthesizer.cpp#L50
That way SpeechSynthesizerSpeak won't need the buffer after it returned, and you can get rid of all the strdup calls for it.
|
Alright! Thanks to the suggestions it is now more memory safe, thank you Gary! |
# Conflicts: # libultraship # soh/soh/Enhancements/game-interactor/GameInteractor.h # soh/soh/Enhancements/game-interactor/GameInteractor_Hooks.cpp # soh/soh/Enhancements/game-interactor/GameInteractor_Hooks.h # soh/soh/Enhancements/mods.cpp
# Conflicts: # OTRExporter/OTRExporter/Main.cpp # soh/CMakeLists.txt # soh/soh/GameMenuBar.cpp
🚨REQUIRES REBUILDING OTR 🚨
Introduces an accessibility: text to speech option for in game content.
The content covered:
This PR also introduces some new assets used as the text description for some in game things that we don't have a name for in the game itself. They're used to power TTS in the Kaleidoscope menu, file select menu, when title cards appear, and timers.
The name of the four files are:
filechoose_xxx.json, kaleidoscope_xxx.json, scenes_xxx.json, misc_xxx.jsonWhat's Left
Build Artifacts