Skip to content

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