-
Notifications
You must be signed in to change notification settings - Fork 206
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
base: master
Are you sure you want to change the base?
The head ref may contain hidden characters: "\u30B9\u30BF\u30A4\u30EB\u3054\u3068\u3067\u306F\u306A\u304F\u3001\u8A71\u8005\u3054\u3068\u306B\u30A2\u30A4\u30B3\u30F3\u3092\uFF11\u3064\u6301\u3066\u308B\u3088\u3046\u306B\u3059\u308B"
話者ごとのスタイルの中のボイスサンプルやアイコンを必須じゃなくする #1052
Conversation
There was a problem hiding this 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: |
There was a problem hiding this comment.
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
There was a problem hiding this 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
属性を追加
There was a problem hiding this 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化がすぐできるので一旦そうします 🙇 )
これがマージされたときにやらないといけないことのメモ
|
リソース側を変更したので、このプルリクエストもマージしようかなとコンフリクトも解消したのですが、そもそもこのプルリクエストの形だと破壊的変更になってしまうことに気づきました。 issue側のコメントのように、破壊的変更が生じない形にこのプルリクエストを作り替えようかなと思うのですが、どうでしょうか 🙇 |
内容
の解決です。
が先に必要になってます。
関連 Issue
fix #1051
resolve #337
その他
これは今までスタイルアイコンが取得できていたものがnullになりえるので、破壊的変更になりえます。
製品版VOICEVOXでは既存アイコンは消さない、かつスタイルごとにアイコンを必ず設置するように心がけたいと思います。
・・・忘れそうな気がしますが・・・。
マージはソングとかがいろいろ落ち着いてからでも良いかも。