-
Notifications
You must be signed in to change notification settings - Fork 119
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
metas
出力時に話者情報をマージする
#728
metas
出力時に話者情報をマージする
#728
Conversation
&& !loaded.eq_except_styles(external) | ||
}) | ||
{ | ||
todo!("same `speaker_uuid` but different properties"); |
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.
エラーを一つ増設するか悩みます。別に検査しなくてもいいような気がしますし。
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.
どっちでもって感じかなと!
人為的なミスには気づきやすくなるので、実装があるならありがたいかもです。
todoが何するかわかってないのですが、エラー取り回し面倒という感じだと思うので、stderrにwarn吐くかpanicでも良いのかもとか思いました!
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.
todo!
はパニックを起こすものですが、これをやめてwarning止まりにするようにしました。
crates/voicevox_core/src/metas.rs
Outdated
pub fn merge<'a>(metas: impl IntoIterator<Item = &'a SpeakerMeta>) -> Vec<SpeakerMeta> { | ||
metas | ||
.into_iter() | ||
.into_grouping_map_by(|speaker| &speaker.speaker_uuid) | ||
.aggregate::<_, SpeakerMeta>(|acc, _, speaker| { | ||
Some( | ||
acc.map(|mut acc| { | ||
acc.styles.extend(speaker.styles.clone()); | ||
acc | ||
}) | ||
.unwrap_or_else(|| speaker.clone()), | ||
) | ||
}) | ||
.into_values() | ||
.map(|mut speaker| { | ||
speaker | ||
.styles | ||
.sort_unstable_by_key(|&StyleMeta { id, .. }| id); | ||
speaker | ||
}) | ||
.sorted_unstable_by_key(|SpeakerMeta { styles, .. }| { | ||
styles.first().map(|&StyleMeta { id, .. }| id) | ||
}) | ||
.collect() | ||
} |
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.
実装になんかコメントを付けようと考えましたが、別に要らないかも...
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.
この関数の役割兼、何するかの1行ドキュメントくらいはあると道標になるかもとは思いました!
よくよく見直すと、単純なarrayのconcatenateなのか、何かをkeyにして名寄せするのかとかは関数名からじゃわからないかもです。
「同じ話者IDのものを集約しつつ結合」とか…?
(まぁ、多少知識があるとspeaker idで名寄せなのはすぐ想像できるので、なくても良いという方針でもあまり問題ないかなとも思います。)
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.
書きました。
9d2b1a2
(#728)
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.
あ、話者がVVMを跨ぐ場合のテストパターン増やしとくと安心かもとか思いました!
どのレイヤーでテストできるかパッとわからないのですが、もしVVMがもう一つ必要ならsampleのVVM複製してmetas書き換えればいける気がします。重いけど。
あるいはあとでモック挿せる形にして(Rustでどう実現するのか全く想像つかないですが…)、VoiceModel辺りのモック作ってからテスト実装でも良いかなとか思いました!
compatible engineでは切った方が良さそうです、というのもずんだもんがたしか2(あまあま)->0(ノーマル)->4(セクシー)->6(ツンツン)みたいな感じ(後半二つは自信ない)のStyleIdだった記憶があるので |
あっ ほんとですね。。。ソートがあると不自然な並びになるかもです。 modelディレクトリのファイルをバージョンソートしてvvmリストを作るとして、指定されたlistの順に後ろに足されていく感じだと合うと思います🙇 |
今VVMの割り当てを見たのですが、VVMの順番をもって並び順を再現するのは少なくとも話者レベルでは厳しくないでしょうか? metas.jsonとかに |
あ、話者を今のPRの状態の
みたいな方向にして、スタイルもVVM単位でまとめながら各 |
おおお。。ほんとですね。。 順序に関してはスタイルIDの番号に依存せず持ってたいかもです。 あ、metas.jsonにorder持たせるのいいですね!! |
あと |
おっと何もしていないのに壊れた的な...? |
あーーーー -# This file is automatically @generated by Poetry 1.6.1 and should not be changed by hand.
+# This file is automatically @generated by Poetry 1.7.1 and should not be changed by hand. |
いや正確にはこっちか。Poetry 1.7.1は昨年11月に出てる。 @@ -808,6 +808,7 @@ files = [
{file = "PyYAML-6.0.1-cp311-cp311-win_amd64.whl", hash = "sha256:bf07ee2fef7014951eeb99f56f39c9bb4af143d8aa3c21b1677805985307da34"},
{file = "PyYAML-6.0.1-cp312-cp312-macosx_10_9_x86_64.whl", hash = "sha256:855fb52b0dc35af121542a76b9a84f8d1cd886ea97c84703eaa6d88e37a2ad28"},
{file = "PyYAML-6.0.1-cp312-cp312-macosx_11_0_arm64.whl", hash = "sha256:40df9b996c2b73138957fe23a16a4f0ba614f4c0efce1e9406a184b6d07fa3a9"},
+ {file = "PyYAML-6.0.1-cp312-cp312-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:a08c6f0fe150303c1c6b71ebcd7213c2858041a7e01975da3a99aed1e7a378ef"},
{file = "PyYAML-6.0.1-cp312-cp312-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:6c22bec3fbe2524cde73d7ada88f6566758a8f7227bfbf93a408a9d86bcc12a0"},
{file = "PyYAML-6.0.1-cp312-cp312-musllinux_1_1_x86_64.whl", hash = "sha256:8d4e9c88387b0f5c7d5f281e55304de64cf7f9c0021a3525bd3b1c542da3b0e4"},
{file = "PyYAML-6.0.1-cp312-cp312-win32.whl", hash = "sha256:d483d2cdf104e7c9fa60c544d92981f12ad66a457afae824d146093b8c294c54"}, |
fn key(order: Option<u32>) -> impl Ord { | ||
order | ||
.map(Into::into) | ||
.unwrap_or_else(|| u64::from(u32::MAX) + 1) |
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.
一応、「style_orderにないスタイルは一番後ろに足していく」みたいなことを書いておいた方がいいかも?
あとスタイルIDを足すようにしておくと順番が変わるのを抑えられる気がします。
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.
「style_orderにないスタイルは一番後ろに足していく」みたいなことを書いておいた方がいいかも?
merge
のdocstringに書いたやつでいいかなと思っています。
あとスタイルIDを足すようにしておくと順番が変わるのを抑えられる気がします。
無い/衝突した場合のフォールバック的な意味でしょうか? 私としてはそのような場合は元々の挿入順に対して安定ソートでいいかなと思っていたのですが、 @Hiroshiba どうしましょう?
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.
mergeのdocstringに書いたやつでいいかなと思っています。
おっと、書いてありました(ごめんなさい)
無い/衝突した場合のフォールバック的な意味で
自分が意味したのは無い時のフォールバックですね。衝突時はそもそも原理的に起こらなさそう?
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.
無い場合は元の順序にしましょう!
個人的には必須でも全然OKだと思います。ボイボはおそらく全部に付けるので。リリース前にミスに気づけるかもしれない。どっちでも!
衝突時も元の順序で良さそうかなと。
warn出るとちょっと嬉しいかも、くらい。(warn出すのどれくらい大変かわからずに言ってます 🙇 )
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.
VVMのロード時にwarn_diff_except_styles
内でwarnするようにしています。マージ時にもう一度出す必要は無いかなと。
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.
素晴らしいですね!!!!
相変わらずRustは型がしっかりしたコードが何でも書けて羨ましいです。
ちょっと1個設計周りでコメントしました!!
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!!
@y-chan vvmからスタイルにorderが入るので共有です。
song系は300番始まりくらいにしようかとか考えてます。
ちょっと迷い中です。
内容
次の2つにおいて、話者が分散しないようにします。
Synthesizer::metas
metas()
また話者とスタイルを、次の値をキーとしてソートするようにします。話者: 持っているスタイルのうち最小のスタイルIDスタイル: ID(追記) このPR内での議論の結果、話者およびスタイルに
order: Option<u32>
を持たせることにしました。関連 Issue
Fixes #727.
その他