Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CGE: Change declaration place and descriptions #3177

Merged
merged 3 commits into from Jul 21, 2021

Conversation

@taylorzhancher
Copy link
Contributor

@taylorzhancher taylorzhancher commented Jul 20, 2021

Some adjustments to the way TTS is done

  • remove unnecessary header
  • move void textToSpeech(const char *text) to talk engine
  • adjust tooltip description in detection.cpp
  • adjustments to the function void textToSpeech(const char *text)
@taylorzhancher taylorzhancher marked this pull request as ready for review Jul 20, 2021
@criezy criezy force-pushed the taylorzhancher:cge-tts-fix branch from 670714c to 2015f74 Jul 20, 2021
@criezy
Copy link
Member

@criezy criezy commented Jul 20, 2021

I rebased the branch on latest master for you to fix the conflict. Your branch was created on a 3-days old master that did not include the last commit from digitall on the CGE engine.

Since I was experimenting with the code, I also pushed a further commit with some changes. This was faster than commenting on the pull request to ask you to do those, but I encourage you to look at the commit as you might want to do similar changes for the CGE2 pull request. Also don't hesitate to ask questions if the purpose of some of those changes os not clear to you.
The changes include:

  • The textToSpeech(const char *) did not need to be virtual since you do not need to reimplement it in classes that derive from Talk (unlike update(const char *) that is reimplemented in InfoLine).
  • I moved the _lastText variable in the InfoLine class.
  • I moved the call to TextToSpeech() from the Talk constructor to the update() function. This change was done for consistency with what is done in the derived InfoLine class.
  • I added a check that the text is not a null pointer in `textToSpeech(). This avoids cutting off the speech immediately when moving the cursor over an object and not stoping on it. I think it sounds nicer that way.

Looking at this I also understood why the _oldText check already in place was not sufficient for the TTS and you needed to introduce a lastText variable. I actually suspect this is a bug, but I did not want to change the code without more tests. So for now both _oldText and _lastText are still there. You can see the comment I added in the code to explain this.

…e same text

There was a check in place to avoid redrawing the infoline when
calling the update() function with the same text multiple times.
Such calls happen when the cursor moves over an object (or menu
option) without leaving that object. The check was however
incorrect and did nothing because it was keeping the pointer
from the end of the previous text and not the start.

From the log it seems that the bug was present from the start,
and probably in the original source code. It was however fixed
in Sfinx (CGE2 engine), and this commit uses the same logic.
@criezy
Copy link
Member

@criezy criezy commented Jul 20, 2021

Looking at this I also understood why the _oldText check already in place was not sufficient for the TTS and you needed to introduce a lastText variable. I actually suspect this is a bug, but I did not want to change the code without more tests.

I have now done more test and more digging and I am almost certain it was indeed a bug in the original source code. I pushed an additional commit with a fix.

@criezy
Copy link
Member

@criezy criezy commented Jul 21, 2021

I see no reason to wait more for this one, so merging now.

@criezy criezy merged commit 5d3457a into scummvm:master Jul 21, 2021
8 checks passed
8 checks passed
@github-actions
Windows (win32, x86-windows, x86, --enable-faad --enable-mpeg2 --enable-discord --disable-fribidi...
Details
@github-actions
Windows (x64, x64, x64-windows, --enable-faad --enable-mpeg2 --enable-discord --disable-fribidi, ...
Details
@github-actions
Windows (arm64, arm64, arm64-windows, --enable-faad --enable-mpeg2 --enable-discord --disable-fri...
Details
@github-actions
Xcode (macosx, -scheme ScummVM-macOS, --disable-nasm --enable-faad --enable-mpeg2, a52dec faad2 f...
Details
@github-actions
Xcode (ios7, -scheme ScummVM-iOS CODE_SIGN_IDENTITY="" CODE_SIGNING_ALLOWED=NO, --disable-nasm --...
Details
@github-actions
Ubuntu (ubuntu-latest, sdl2-config, --enable-c++11, libsdl2-dev libsdl2-net-dev liba52-dev libjpe...
Details
@github-actions
Ubuntu (ubuntu-18.04, sdl-config, --disable-c++11, libsdl1.2-dev libsdl-net1.2-dev liba52-dev lib...
Details
@codacy-production
Codacy Static Code Analysis Codacy Static Code Analysis
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants