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

kana: boolをやめ、"_from_kana"を復活させる #577

Merged
merged 6 commits into from
Sep 23, 2023

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Aug 15, 2023

内容

経緯: https://discord.com/channels/879570910208733277/893889888208977960/1123249926143475722

関連 Issue

ref #545

その他

このPRはCおよびPythonのAPIを破壊的に変更します。
(追記) あとJavaも

@Hiroshiba
Copy link
Member

Hiroshiba commented Aug 15, 2023

ここら辺の辺りに関して、実装が振動しないようにするために議論をコピーしておこうと思います!

結論としては

  • 一長一短
    • ユーザー的には別れてたほうが良い
    • 開発的には一緒だったほうが良い(エンジンと乖離するので)
  • ユーザー目線を持つとやったほうが良さそう
  • ドキュメントをもう片方が参照する(「詳しくはこっちのAPI見てください」)形

qryxip — 2023/06/27 22:54

COREの0.13→0.14でtts_from_kana()がtts(kana=True)のようになりましたが、今からでもfrom_kanaを復活させるのはどうか、というのを考え始めてます。
なんというか、ユーザー視点でも[string, boolean]というよりは{ japaneseText: string; } | { kana: string; }を投げている感じの方が理解しやすいんじゃないかとドキュメントを書き始めて思ってきました。あと「Open JTalk抜きのSynthesizer」の挙動も説明しやすくなるんじゃないかと思っています。
追加の利点としてC APIのAudioQueryOptionsとAccentPhrasesOptions (↓)を消滅させられると思います。

typedef struct VoicevoxAudioQueryOptions {
  bool kana;
} VoicevoxAudioQueryOptions;

typedef struct VoicevoxAccentPhrasesOptions {
  bool kana;
} VoicevoxAccentPhrasesOptions;

ヒホ — 2023/06/28 00:22
正直ユーザー目線だとそっちのが良いとは思います!!
開発目線だとまあ今からわざわざ変えなくても良いかも、とも思います。。。

  • オプション消える代わりにfrom_kana用関数が3つ増える
  • ドキュメントもほぼ同じ説明を3回書くことになる

「Open JTalk抜きのSynthesizer」の挙動の説明はどこ変わるのか気になります。
API周りだけ考えると、ライブラリ利用者視点ではkanaの扱いが明確なので100点中プラス2~3点、ライブラリ開発視点だとメンテナンス対象が倍になるのでマイナス5~6点ってところかなぁ。。(片方のAPIだけ変更してもう片方忘れちゃうとか起こる)
インターフェイス周り以外の実装がどう変わるか次第!


qryxip — 2023/06/28 01:10

開発視点でマイナスが入るとは考えていませんでした... (むしろ実装側のことを考えてこの考えが浮かびました)
ドキュメントで日本語テキストとkanaの2通りの「使い方例」を書き、「textというパラメータは何なのか」を説明し、実装もこうなっていることを考えると3×2に増やした方がすっきりすると思ってました。

let accent_phrases = if options.kana {
self.synthesis_engine
.replace_mora_data(&parse_kana(text)?, speaker_id)?
} else {
self.synthesis_engine
.create_accent_phrases(text, speaker_id)?
};

片方のAPIだけ変更してもう片方忘れる、というシナリオがすぐに思い浮かばなかったです。例えばどういうのを...?
(画像)

「Open JTalk抜きのSynthesizer」

「Open JTalk無しだと日本語のテキストは受け付けられない。つまりoptions.kana = trueでなくてはならない」→「Open JTalk抜きではこの関数は使えない」
のようにシンプルになると思いました。ただよく考えると文章量についてはあんまり変わらないかも。


qryxip — 2023/06/28 01:45

インターフェイス周り以外の実装

voicevox_core#497の方に書いたようにSynthesizerをSynthesizer<()> | Synthesizer<impl Borrow>とし、Synthesizer<()>からは_from_kanaの方のメソッドしか生えないようにすれば実装の見通しがよくなるんじゃないかと思ってます (C/Python APIの方では"OpenJTalk is missing"的なエラーを用意しておけばお茶を濁せると思います)


ヒホ — 2023/06/28 02:15

なるほどです!! kanaのときはOpenJTalkが要らないという認識が漏れていました。
「変更しない方がいい」から、「どっちもあり」くらいの気持ちになりました。

ドキュメントに関しては、「詳しくはこっちのAPI見てください」という手もあるかもです。
ただドキュメントは何書くかの共通認識作らないと話が合わないかもです。議論してもいいですが、とりあえずkana=Falseのドキュメント書いて共通認識作るのどうでしょう?無駄にならないはず。

片方のAPIだけ変更してもう片方忘れる、というシナリオ

例えばAPIドキュメントを一部コピペすると思うのですが、その修正を片方だけして片方忘れるのが怖いかなと!
ただこれは片方のドキュメントをもう片方が参照する(「詳しくはこっちのAPI見てください」)形にすれば問題ない気がしました!

あとはまあ、エンジン側と実装が乖離するのが少し気になるくらいです。
総合してどっちもありかなと!

@qryxip qryxip mentioned this pull request Aug 16, 2023
69 tasks
@qryxip qryxip marked this pull request as ready for review September 22, 2023 18:28
@qryxip qryxip requested a review from Hiroshiba September 22, 2023 18:48
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

ほぼLGTMです!!

java周りは @sevenc-nanashi さんにコメントいただけると心強いです・・・!

crates/voicevox_core/src/voice_synthesizer.rs Outdated Show resolved Hide resolved
crates/voicevox_core/src/voice_synthesizer.rs Show resolved Hide resolved
crates/voicevox_core_c_api/src/lib.rs Show resolved Hide resolved
Copy link
Member

@sevenc-nanashi sevenc-nanashi left a comment

Choose a reason for hiding this comment

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

Java周りLGTMです!
tts_from_kanaがいらないのは同感です。

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

LGTM!! いよいよリリースに向けて、って感じがしてきました!!

@sevenc-nanashi さんにも同意頂いてたAPI周りの件、こんな感じになったので共有です 🙏
#577 (comment)

@Hiroshiba Hiroshiba merged commit 302d6b9 into VOICEVOX:main Sep 23, 2023
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants