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

話者ごとのスタイルの中のボイスサンプルやアイコンを必須じゃなくする #1052

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

Conversation

Hiroshiba
Copy link
Member

@Hiroshiba Hiroshiba commented Feb 6, 2024

内容

の解決です。

が先に必要になってます。

関連 Issue

fix #1051
resolve #337

その他

これは今までスタイルアイコンが取得できていたものがnullになりえるので、破壊的変更になりえます。
製品版VOICEVOXでは既存アイコンは消さない、かつスタイルごとにアイコンを必ず設置するように心がけたいと思います。

・・・忘れそうな気がしますが・・・。

マージはソングとかがいろいろ落ち着いてからでも良いかも。

@Hiroshiba Hiroshiba requested a review from a team as a code owner February 6, 2024 13:13
@Hiroshiba Hiroshiba requested review from y-chan and removed request for a team February 6, 2024 13:13
Copy link

github-actions bot commented Feb 6, 2024

Coverage Result

Resultを開く
Name Stmts Miss Cover
run.py 520 218 coverage-58%
voicevox_engine/init.py 1 0 coverage-100%
voicevox_engine/cancellable_engine.py 94 72 coverage-23%
voicevox_engine/core/init.py 0 0 coverage-100%
voicevox_engine/core/core_adapter.py 81 12 coverage-85%
voicevox_engine/core/core_initializer.py 60 30 coverage-50%
voicevox_engine/core/core_wrapper.py 225 157 coverage-30%
voicevox_engine/dev/init.py 0 0 coverage-100%
voicevox_engine/dev/core/init.py 0 0 coverage-100%
voicevox_engine/dev/core/mock.py 65 4 coverage-94%
voicevox_engine/dev/tts_engine/init.py 0 0 coverage-100%
voicevox_engine/dev/tts_engine/mock.py 28 0 coverage-100%
voicevox_engine/engine_manifest/EngineManifest.py 36 0 coverage-100%
voicevox_engine/engine_manifest/EngineManifestLoader.py 12 0 coverage-100%
voicevox_engine/engine_manifest/init.py 0 0 coverage-100%
voicevox_engine/library_manager.py 92 4 coverage-96%
voicevox_engine/metas/Metas.py 37 0 coverage-100%
voicevox_engine/metas/MetasStore.py 28 1 coverage-96%
voicevox_engine/metas/init.py 0 0 coverage-100%
voicevox_engine/model.py 180 3 coverage-98%
voicevox_engine/morphing.py 71 4 coverage-94%
voicevox_engine/preset/Preset.py 13 0 coverage-100%
voicevox_engine/preset/PresetError.py 2 0 coverage-100%
voicevox_engine/preset/PresetManager.py 80 2 coverage-98%
voicevox_engine/preset/init.py 0 0 coverage-100%
voicevox_engine/setting/Setting.py 11 0 coverage-100%
voicevox_engine/setting/SettingLoader.py 17 0 coverage-100%
voicevox_engine/setting/init.py 0 0 coverage-100%
voicevox_engine/tts_pipeline/init.py 0 0 coverage-100%
voicevox_engine/tts_pipeline/kana_converter.py 88 1 coverage-99%
voicevox_engine/tts_pipeline/mora_mapping.py 7 0 coverage-100%
voicevox_engine/tts_pipeline/phoneme.py 34 0 coverage-100%
voicevox_engine/tts_pipeline/text_analyzer.py 146 6 coverage-96%
voicevox_engine/tts_pipeline/tts_engine.py 267 9 coverage-97%
voicevox_engine/user_dict/part_of_speech_data.py 5 0 coverage-100%
voicevox_engine/user_dict/user_dict.py 146 12 coverage-92%
voicevox_engine/utility/init.py 0 0 coverage-100%
voicevox_engine/utility/connect_base64_waves.py 37 0 coverage-100%
voicevox_engine/utility/core_utility.py 6 0 coverage-100%
voicevox_engine/utility/core_version_utility.py 8 1 coverage-88%
voicevox_engine/utility/mutex_utility.py 13 0 coverage-100%
voicevox_engine/utility/path_utility.py 26 6 coverage-77%
voicevox_engine/utility/run_utility.py 10 7 coverage-30%
TOTAL 2446 549 coverage-78%

Copy link
Member

@y-chan y-chan left a comment

Choose a reason for hiding this comment

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

おおよそLGTMです!

一点コメントしましたが、私も正解がわからないのでまあ修正するかはどっちでもいいのかな...と思いました。

test/utility.py Outdated
@@ -19,3 +20,19 @@ def round_floats(value: Any, round_value: int) -> Any:
def pydantic_to_native_type(value: Any) -> Any:
"""pydanticの型をnativeな型に変換する"""
return json.loads(json.dumps(value, default=pydantic_encoder))


def hash_long_string(value: Any) -> Any:
Copy link
Member

Choose a reason for hiding this comment

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

ちょっと関数の名前がわかりづらいかも...?と思いました。
パッと見たとき、jsonが渡されたらそれ全体がハッシュされるのかと思いました。
でも内容によって返り値が変わる万能関数ではあるので、命名難しいですね...
process_large_dataとか...うーん...なんか違うかも...

https://chat.openai.com/share/21f7198f-f993-4901-a7e9-287bd365eca0

Copy link
Contributor

@tarepan tarepan left a comment

Choose a reason for hiding this comment

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

LGTM👍

軽量なスタイル情報になり、開発時などに小回りの効く実装となっています。good work!

変更概要

  • スタイルごとのvoice_samplesとiconをOptionalにする → StyleInfo.icon および StyleInfo.voice_samples を Optional 化
  • 話者ごとのiconを必須にする → SpeakerInfo.icon 属性を追加

Copy link
Member Author

@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.

レビューありがとうございます!

マージしようとしたのですが、RESOURCE側を変えないと製品版の起動に失敗しそうなことに気づきました。
スタイルごとのアイコン画像は不要になりましたが、代わりに話者ごとのicon.pngが必須になったためです。

RESOURCE側を更新してからmainにマージするのが良さそうに思ってます。
間違えてmergeしないようにdraft PRにしたいと思います。

(あるいは適当なブランチを作ってそちらにマージして置いとくとかもありかもです。とりあえずdraft化がすぐできるので一旦そうします 🙇 )

@Hiroshiba
Copy link
Member Author

これがマージされたときにやらないといけないことのメモ

  • prereleaseを作る
  • エディタ側をこのPRに対応する(openapiの更新、バグがないようにコード変更)

@Hiroshiba Hiroshiba marked this pull request as ready for review May 25, 2024 14:24
@Hiroshiba Hiroshiba requested a review from a team as a code owner May 25, 2024 14:24
@Hiroshiba
Copy link
Member Author

Hiroshiba commented May 25, 2024

リソース側を変更したので、このプルリクエストもマージしようかなとコンフリクトも解消したのですが、そもそもこのプルリクエストの形だと破壊的変更になってしまうことに気づきました。
詳細は↓のコメントに書いています。

issue側のコメントのように、破壊的変更が生じない形にこのプルリクエストを作り替えようかなと思うのですが、どうでしょうか 🙇

@Hiroshiba Hiroshiba marked this pull request as draft May 25, 2024 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants