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

Fix ZeroDivisionError when pitch is 0 announcing 'cap' #13921

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

seanbudd
Copy link
Member

@seanbudd seanbudd commented Jul 19, 2022

Link to issue number:

Fixes #10940

Summary of the issue:

NVDA fails to announce "cap" for capital letters when synthesizer pitch is set to 0.
This is because of a division by 0 error when scaling the pitch change for "cap".

Description of user facing changes

NVDA successfully announces "cap" for capital letters as if synthesizer pitch was set to 1.

Description of development approach

To fix this, use a value of 1 instead of 0 when calculating the pitch for announcing "cap".

Testing strategy:

Manually test STR with notepad

  1. Enable "Say 'cap' before capitals" in Speech
  2. Ensure that "capital pitch change percentage" is set to value different than 0
  3. Set synthesizer pitch to 0
  4. Try to read character by character "aA"

Known issues with pull request:

None

Change log entries:

Bug fixes

- Fix bug where NVDA failed to announce "cap" for capitals when synthesizer pitch was set to 0. (#10940)

Code Review Checklist:

  • Pull Request description:
    • description is up to date
    • change log entries
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • API is compatible with existing add-ons.
  • Documentation:
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English

@seanbudd seanbudd requested a review from a team as a code owner July 19, 2022 03:43
@seanbudd seanbudd requested a review from feerrenrut July 19, 2022 03:43
@AppVeyorBot
Copy link

See test results for failed build of commit 691b6d037d

@feerrenrut
Copy link
Contributor

May also be worth considering: #9811

@seanbudd
Copy link
Member Author

Using this approach, eSpeak fails to change pitch for 'cap', as eSpeak scales the pitch by multiplying by the base pitch of 0.
An issue/PR to eSpeak is required to inform them that when pitch is set to 0, pitch change commands fail to scale the pitch correctly.

An alternative approach is to make NVDA use the range 1-100 instead of 0-100.
This was previously blocked by changing the wx.Slider to the range 1-100, i.e. only 100 intervals not 101.

@feerrenrut
Copy link
Contributor

Next action: report issue to espeak.

@seanbudd seanbudd removed this from the 2022.4 milestone Aug 31, 2022
@seanbudd
Copy link
Member Author

seanbudd commented Jan 9, 2023

Using this approach, eSpeak fails to change pitch for 'cap', as eSpeak scales the pitch by multiplying by the base pitch of 0.
An issue/PR to eSpeak is required to inform them that when pitch is set to 0, pitch change commands fail to scale the pitch correctly.
Next action: report issue to espeak.

It turns out this bug was with NVDA. The eSpeak synth driver code to handle prosody commands did not handle the offset attribute, only the multiplier attribute. The only known usage of offset is for capPitchChange.
There also seems to be incorrect support on all other synthesizers. This PR will instead aim to fix the PitchCommand for all synthesizers.

@seanbudd seanbudd requested review from michaelDCurran and removed request for feerrenrut January 9, 2023 04:57
@seanbudd
Copy link
Member Author

seanbudd commented Jan 9, 2023

Related bugs:
#10214
#7514
#2195

@seanbudd seanbudd self-assigned this Jan 9, 2023
@seanbudd
Copy link
Member Author

seanbudd commented Jan 10, 2023

I'm going to look into dropping the offset attribute from all BaseProsodyCommands as it is not supported correctly in general

@codeofdusk
Copy link
Contributor

I'm going to look into dropping the offset attribute from all BaseProsodyCommands as it is not supported correctly in general

Given that 2024.1 is an API breaking release, might now be a good time to do this (or, ideally, to fix the broekn behaviour)? @seanbudd @jcsteh

@rmcpantoja
Copy link
Contributor

Hi,
I would like to take this opportunity to add that, in NVDA builds with Python 3.7, especially the stable versions, the division by zero errors are caused by an internal library, and it's not an error specifically with any of the core scripts or anything in the source code. I think it's something close to the pydecimal.py library. The strangest thing is that in the alpha versions that were built with Python 3.7 at the time, this error did not occur, and everything worked fine. I have had to experience this with the num2words add-on, and testing the num2words package locally this error doesn't occur and makes a conversion without errors. At the moment the add-on works with the latest alpha built with Python 3.11, including the decimal numbers that cause the division error in stable versions. So, I think it has to do with the build process of the artifacts.

@seanbudd
Copy link
Member Author

@codeofdusk - this can be fixed in a way which temporarily preserves backwards compatibility

@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Jul 2, 2024
@Qchristensen
Copy link
Member

Is this PR blocked or waiting on anything?

I (got here following a user still reporting the original issue)

@seanbudd
Copy link
Member Author

seanbudd commented Oct 2, 2024

@Qchristensen - I think a fix in eSpeak is required for part of the fix, but this PR could probably be revived

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NVDA fails to announce capital letters when pitch is set to 0
6 participants