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

Bug in debug output: was skipping every other phoneme #3

Merged
merged 3 commits into from
May 19, 2020

Conversation

tyomitch
Copy link

No description provided.

Copy link
Owner

@discordier discordier left a comment

Choose a reason for hiding this comment

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

I agree with you on the change for the debugging.

The change for the rising and falling inflection though we should discuss.
If I get you right, only the naming is the wrong way around.
See Original code here for question mark and here for periods.

Is there any reason you changed the order of the conditions instead of simply swapping the constant name in the AddInflection() calls?

@tyomitch
Copy link
Author

The change for the rising and falling inflection though we should discuss.
If I get you right, only the naming is the wrong way around.

Yes, the values 1 and 255 were used correctly, but they were assigned wrong names.
The original SAM docs say: "The period inserts a pause and also causes the pitch to fall. The question-mark also inserts a pause, but it causes the pitch to rise."

See Original code here for question mark and here for periods.

Yep, those two comments in the C code are wrong, too.

Is there any reason you changed the order of the conditions instead of simply swapping the constant name in the AddInflection() calls?

What I did was simply swapping the constant names in the AddInflection() calls, but I can see how the diff display is somewhat confusing. If you switch the display mode from "Unified" to "Split", it becomes a bit less confusing.

@tyomitch
Copy link
Author

See Original code here for question mark and here for periods.

Yep, those two comments in the C code are wrong, too.

I have now submitted a PR for it, too: s-macke#13

@tyomitch tyomitch removed their assignment May 1, 2020
@discordier discordier merged commit a27bf4d into discordier:master May 19, 2020
@discordier
Copy link
Owner

Released in 0.1.2

@tyomitch tyomitch deleted the pr3 branch May 19, 2020 10:10
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.

2 participants