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

Add audio/general/text_to_speech project setting to enable/disable TTS. #77132

Merged
merged 1 commit into from
May 19, 2023

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented May 16, 2023

Adds setting to completely disable TTS support.

The issue seems to be Linux specific, but for consistency, the setting is applied on all systems.

Fixes #77124

@lawnjelly
Copy link
Member

On principle the default should be false imo.

Users will not expect text to speech to be using resources and CPU when they have never requested it - slowing down their game or eating battery unnecessarily. We don't for example, render shadow maps just in case user might want shadows.

If this is to be used for editor accessibility (I don't know if it is currently), we could consider a separate editor setting. Also useful would be a runtime command line argument to override these settings (in case end user wants to disable text to speech).

Another approach once the project setting is working, as @bruvzg suggested previously, is simply allow deferring speech startup until a speech command is issued, and then the default could perhaps be changed to on, if there was no cost. But in any case having it switch-offable is eminently desirable.

@bruvzg
Copy link
Member Author

bruvzg commented May 16, 2023

On principle the default should be false imo.

I would set it to false, but this will break existing projects with TTS (but I do not think there are many, so might be ok for 4.1).

Users will not expect text to speech to be using resources and CPU when they have never requested it - slowing down their game or eating battery unnecessarily. We don't for example, render shadow maps just in case user might want shadows.

It seems to Linux specific, and might be specific to speech dispatcher or Pulse Audio version (I do not get more the 1.5% of usage).

If this is to be used for editor accessibility (I don't know if it is currently), we could consider a separate editor setting.

It's not used for anything in the editor, and not a good way to implement accessibility (I'm working on a proper screen reader support right now - #76829).

is simply allow deferring speech startup until a speech command is issued

Not sure if it's worth overcomplicating it.

@bruvzg bruvzg marked this pull request as ready for review May 16, 2023 13:25
@bruvzg bruvzg requested review from a team as code owners May 16, 2023 13:25
@lawnjelly
Copy link
Member

lawnjelly commented May 16, 2023

I would set it to false, but this will break existing projects with TTS (but I do not think there are many, so might be ok for 4.1).

Surely all it will take is those few users flipping a project setting when they next export the project? Provided this is included in the release notes, this seems unlikely to cause many problems (it should be fairly obvious if a feature they are using is no longer working).

There's no change to e.g. code needed. This seems imo by far the lesser of two evils, if the alternative is making everyone else pay for this oversight, in perpetuity.

BTW: Don't get me wrong, I'm fully appreciative of this feature and it is great work, especially for the blind (I'm actually partially sighted at the moment, hopefully only until June). This is why I'm keen for ways that are easy for the blind to control, but not in a way that regular users "pay the price".

@bruvzg
Copy link
Member Author

bruvzg commented May 16, 2023

Well, error messages directly say what to do, so I guess it's OK. Changed default to false.

Copy link
Member

@lawnjelly lawnjelly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok to me, I'm not an expert on all the platforms so any second pair of eyes to check would be good. 👍

Also reminder we need an 3.x version too, I guess it should mostly be the same.

@akien-mga akien-mga changed the title Add application/config/text_to_speech project setting to enable/disable TTS. Add audio/general/text_to_speech project setting to enable/disable TTS. May 16, 2023
@akien-mga
Copy link
Member

Looks good to me. The commit message should be amended like I just did with the PR title.

Also, I think @lawnjelly's concern also includes the CPU usage of the editor, which this wouldn't address as it's only for the project settings so project runtime. So this might also need an editor setting to disable editor TTS.
It could be disabled by default until we add accessibility features using it (in which case it should be enabled by default IMO).

Copy link
Contributor

@m4gr3d m4gr3d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Android section looks good, thanks for the update!

@YuriSizov
Copy link
Contributor

@bruvzg Please amend the commit message, it still incorrectly refers to the setting name. Should be audio/general/text_to_speech.

@YuriSizov YuriSizov modified the milestones: 4.x, 4.1 May 18, 2023
@akien-mga akien-mga merged commit aaa77d0 into godotengine:master May 19, 2023
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Would need a 3.x backport if we want to make it opt-in for 3.6 too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Text to speech draining CPU with Pulse Audio when not in use
6 participants