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: NOT_LOADED_OPENJTALK_DICT_ERRORのメッセージを修正 #281

Merged

Conversation

sevenc-nanashi
Copy link
Member

内容

NOT_LOADED_OPENJTALK_DICT_ERRORのエラーメッセージを修正しました。
voicevox_load_openjtalk_dictはもう存在しません。

関連 Issue

(なし)

その他

(なし)

@PickledChair
Copy link
Member

PickledChair commented Sep 16, 2022

PR ありがとうございます!

voicevox_load_openjtalk_dictはもう存在しません。

確かにその通りですね……! エラーメッセージを変えた方が良さそうです。

ただし、今回の PR では新しいエラーメッセージとして "OpenJTalkの辞書読み込みに失敗しました" が提案されていますが、このエラーの意図は「辞書の読み込み失敗」ではなく、単に「辞書が読み込まれていない状態である」というものです。このエラーを報告しうる関数がいくつかあって、実際にはなぜこのエラーが出たのかについての理由がそれぞれの関数で若干異なっています。

initialize 関数では以下の箇所でこのエラーが起こる可能性があります。

if let Some(open_jtalk_dict_dir) = options.open_jtalk_dict_dir {
self.synthesis_engine
.load_openjtalk_dict(open_jtalk_dict_dir)?;
}

ここでエラーが起こった場合は提案の通り「辞書の読み込み失敗」が起こっています。

一方、audio_query 関数では以下の箇所でこのエラーが起こる可能性があります。

if !self.synthesis_engine.is_openjtalk_dict_loaded() {
return Err(Error::NotLoadedOpenjtalkDict);
}

ここでエラーが起こった場合は提案の文言とは異なり、以下のような要因で「まだ辞書を読み込む操作が行われていない」という状態になっています:

  • 単に旧 API の initialize 関数を呼んだ場合
  • voicevox_initialize 関数を open_jtalk_dict_dir オプションなしで呼んだ場合
  • voicevox_initialize 関数を open_jtalk_dict_dir オプションありで呼んだがエラーが発生し、それを無視して audio_query 関数を呼んだ場合

これらのケースでは、audio_query 関数を呼ぶ前に辞書読み込みの操作すら行っていないので、辞書読み込みに失敗したわけではありません。これらのケースで出たエラーをユーザー側が解決する手段は、以前は voicevox_load_openjtalk_dict 関数を呼ぶことでしたが、これは削除されたので、現在の唯一の手段はあらかじめ open_jtalk_dict_dir オプションを指定しつつ voicevox_initialize 関数でコアを初期化し、それに成功することです。

この二箇所のエラー発生理由が若干異なっているので、もしかしたら別々のエラーとして定義した方が良いのかもしれません(initialize の方での失敗は FailedToLoadOpenjtalkDict にする等)。エラーをこのまま NotLoadedOpenjtalkDict のみにする場合は "OpenJTalkの辞書が読み込まれていません" などというメッセージの方が適切になると思います。

@Hiroshiba @qwerty2501 @qryxip このエラーについてご意見を聞きたいと思いました。よろしくお願いします……!

@sevenc-nanashi
Copy link
Member Author

sevenc-nanashi commented Sep 16, 2022

なるほど、とりあえず"OpenJTalkの辞書が読み込まれていません"にします。
initializeとかも考えると保留

@qwerty2501
Copy link
Contributor

確かFailedなどの失敗した単語はError enum内にあることから自明であるためつけない方針だったはずです。コメントで説明書いておいたほうが良いかも
そのため 分けるなら LoadOpenjtalkDict 及び NotLoadedOpenjtalkDict にしたほうが良さそうです。

@Hiroshiba
Copy link
Member

Hiroshiba commented Sep 16, 2022

エラーを分けるのが良さそうに思いました!

ただPRの内容的にエラーを増やすのは異なりそうなので、いったんOpenJTalkの辞書が読み込まれていませんでいったんマージしちゃうか、タイトルを変更してエラーを増やしていただくのが良いのかなと思います。
どちらの方針でも良い方向に進みそうです。 @sevenc-nanashi さんの好きな方で・・・!

@sevenc-nanashi
Copy link
Member Author

とりあえずOpenJTalkの辞書が読み込まれていませんにしましたー。
Rustもちょっとだけできるので後で後者もPRしようかなと思ってます

@qryxip
Copy link
Member

qryxip commented Sep 17, 2022

こういう話もあり、こっちも多分実装難易度は低いのでPRが被るかもしれません。
#275 (comment)

(ただそうなってもマクロ化PRの方がエラー追加PRに合わせる形でよさそうですが)

Copy link
Contributor

@qwerty2501 qwerty2501 left a comment

Choose a reason for hiding this comment

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

LGTM
@qryxip まあその程度の競合であればどちらが対応することになっても問題ないでしょう

Copy link
Member

@qryxip qryxip left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@PickledChair PickledChair left a comment

Choose a reason for hiding this comment

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

LGTM!

@Hiroshiba Hiroshiba merged commit 084bf63 into VOICEVOX:main Sep 17, 2022
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.

5 participants