Add ambient and noise subtitle options #853
Conversation
- Add flag `showSubtitlesAmbient` and `showSubtitlesNoise` for enabling/disabling ambient and noise subtitles. - Modify `DialogMenu::setupSettings` to set the new flags - Modify `DialogMenu::haveToShowSubtitles` to use the new flags to decide if subtiltes need to be shown. Changes are made similar to #PR810 Try#810
|
Hi, @Tavlin ! I've noticed you been working on this draft for quite a bit. Is it something you still looking into, or should I close this PR? |
|
Hey @Try , Sorry for the long pause. I tried to do this PR on my Mac, but I could not get the compilation running. I am now working on it on a linux machine. Once I get the main build working I will continue updating this PR with a working version. Cheers |
- Fix build crashes
- fix `case default:` -> `default:`
|
So I got the build running, however, even on the main master branch Gothic is immediately crashing for me. :/ Its also not working baseline from steam using Proton GE for me, so I dunno whats wrong there. |
Do you have a crash-log? |
No, it was basically just telling me the process got killed. I think the issue was that I did not reboot after installing the vulkan drivers. It seems to be working now. 👍 Cheers |
| Gothic::inst().onSettingsChanged.ubind(this,&DialogMenu::setupSettings); | ||
| } | ||
|
|
||
| void DialogMenu::setupSettings() { |
There was a problem hiding this comment.
missing alignment for both lines in this function
|
|
||
| bool DialogMenu::haveToShowSubtitles(bool isPl) const { | ||
| return showSubtitles && (showSubtitlesPlayer || !isPl); | ||
| if(!showSubtitles){ |
There was a problem hiding this comment.
code-style: { } have to be similar to other code. No need to wrap single-lines into backets
| if(isPl){ | ||
| return showSubtitlesPlayer; | ||
| } else if(other != nullptr) { | ||
| switch (other->processPolicy()) { |
There was a problem hiding this comment.
Have you check on how those option are function in vanilla? processPolicy is only for large distances, what is not a thing in original game.
There was a problem hiding this comment.
I was not sure how to check that. Using the processPolicy was my best guess, since I did not see any other flag that would distinguish ambient talks and surroundings npc ambient infos and talks.
If you could point me to the right direction I can have another look.
Also I just realized in the Issue you wrote:
subTitlesAmbient=1
; ... set to 1 if you dont want to have subtitles for ambient talks (default: 1) [disabled if subTitles is off]
and I implemented it the other way around (0 is off, 1 is on). Should I change this behavior? It just feels weird that this one is the only one where 1 means off.
There was a problem hiding this comment.
If you could point me to the right direction I can have another look
You need to check how this option affect the base game. Intuitively, it probably about npc-to-npc dialogs. Maybe also about throw-away phrases, such as "Thief!!", "I shall get you!!" and such.
There was a problem hiding this comment.
I will try to test and see if I can figure this out.
| } | ||
| if(isPl){ | ||
| return showSubtitlesPlayer; | ||
| } else if(other != nullptr) { |
There was a problem hiding this comment.
So should I set a variable in the if else block and return it at the end?
| if(isPl){ | ||
| return showSubtitlesPlayer; | ||
| } else if(other != nullptr) { | ||
| switch (other->processPolicy()) { |
There was a problem hiding this comment.
??
you can remove else and wont change anything in how this code works
There was a problem hiding this comment.
Ah yes ofc. Sorry for the brain lag.
2 participants
showSubtitlesAmbientandshowSubtitlesNoisefor enabling/disabling ambient and noise subtitles.DialogMenu::setupSettingsto set the new flagsDialogMenu::haveToShowSubtitlesto use the new flags to decide if subtitles need to be shown.Since the distinction between ambient and noise was not 100% clear to me, I thought making it based on the ProcessPolicy seemed like a good estimate to me.
Related PR: #810
Fixes: #713 (hopefully)