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

metas出力時に話者情報をマージする #728

Merged
merged 9 commits into from
Feb 9, 2024

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Jan 21, 2024

内容

次の2つにおいて、話者が分散しないようにします。

  • Synthesizer::metas
  • compatible engineのmetas()

また話者とスタイルを、次の値をキーとしてソートするようにします。

  • 話者: 持っているスタイルのうち最小のスタイルID
  • スタイル: ID

(追記) このPR内での議論の結果、話者およびスタイルにorder: Option<u32>を持たせることにしました。

関連 Issue

Fixes #727.

その他

&& !loaded.eq_except_styles(external)
})
{
todo!("same `speaker_uuid` but different properties");
Copy link
Member Author

Choose a reason for hiding this comment

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

エラーを一つ増設するか悩みます。別に検査しなくてもいいような気がしますし。

Copy link
Member

@Hiroshiba Hiroshiba Jan 21, 2024

Choose a reason for hiding this comment

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

どっちでもって感じかなと!
人為的なミスには気づきやすくなるので、実装があるならありがたいかもです。
todoが何するかわかってないのですが、エラー取り回し面倒という感じだと思うので、stderrにwarn吐くかpanicでも良いのかもとか思いました!

Copy link
Member Author

Choose a reason for hiding this comment

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

todo!はパニックを起こすものですが、これをやめてwarning止まりにするようにしました。

25a918c (#728)

Comment on lines 8 to 32
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()
}
Copy link
Member Author

Choose a reason for hiding this comment

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

実装になんかコメントを付けようと考えましたが、別に要らないかも...

Copy link
Member

@Hiroshiba Hiroshiba Jan 21, 2024

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で名寄せなのはすぐ想像できるので、なくても良いという方針でもあまり問題ないかなとも思います。)

Copy link
Member Author

Choose a reason for hiding this comment

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

書きました。
9d2b1a2 (#728)

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.

あ、話者がVVMを跨ぐ場合のテストパターン増やしとくと安心かもとか思いました!
どのレイヤーでテストできるかパッとわからないのですが、もしVVMがもう一つ必要ならsampleのVVM複製してmetas書き換えればいける気がします。重いけど。

あるいはあとでモック挿せる形にして(Rustでどう実現するのか全く想像つかないですが…)、VoiceModel辺りのモック作ってからテスト実装でも良いかなとか思いました!

@sevenc-nanashi
Copy link
Member

sevenc-nanashi commented Jan 21, 2024

また話者とスタイルを、次の値をキーとしてソートするようにします。

  • 話者: 持っているスタイルのうち最小のスタイルID
  • スタイル: ID

compatible engineでは切った方が良さそうです、というのもずんだもんがたしか2(あまあま)->0(ノーマル)->4(セクシー)->6(ツンツン)みたいな感じ(後半二つは自信ない)のStyleIdだった記憶があるので

@Hiroshiba
Copy link
Member

Hiroshiba commented Jan 21, 2024

あっ ほんとですね。。。ソートがあると不自然な並びになるかもです。

modelディレクトリのファイルをバージョンソートしてvvmリストを作るとして、指定されたlistの順に後ろに足されていく感じだと合うと思います🙇

@qryxip
Copy link
Member Author

qryxip commented Jan 22, 2024

今VVMの割り当てを見たのですが、VVMの順番をもって並び順を再現するのは少なくとも話者レベルでは厳しくないでしょうか?
(3.vvmが波音リツ, もち子さん, 中国うさぎという構成)

metas.jsonとかにspeaker_order: numberstyle_orders: Map<number, number>を持たせるとかしないといけない気がします。

@qryxip
Copy link
Member Author

qryxip commented Jan 22, 2024

あ、話者を今のPRの状態の

  • 話者: 持っているスタイルのうち最小のスタイルID

みたいな方向にして、スタイルもVVM単位でまとめながら各styles[0].idだけソートのキーにするようにすれば上手くいく? (初期のあまあま事件を除けばtypestyle_idで降順になっていると期待できそうなので)

@Hiroshiba
Copy link
Member

Hiroshiba commented Jan 22, 2024

おおお。。ほんとですね。。
model_indexのマップ辺りが順番も持ってたのを失念していました。

順序に関してはスタイルIDの番号に依存せず持ってたいかもです。
スタイルIDにID以外の情報をなるべく持たせたくないので…。

あ、metas.jsonにorder持たせるのいいですね!!
思いつきませんでした。個人的にはそれで十分な気がします。
@y-chan にも意見聞きたみです。

@qryxip
Copy link
Member Author

qryxip commented Jan 27, 2024

SpeakerMeta::{speaker_order,style_order}を導入しました。
178d5ac (#728)

あとmergeに対するユニットテストを一つ入れました。
e78dd4f (#728)

@qryxip
Copy link
Member Author

qryxip commented Jan 27, 2024

おっと何もしていないのに壊れた的な...?
https://github.com/VOICEVOX/voicevox_core/actions/runs/7679657706/job/20930795908?pr=728

@qryxip
Copy link
Member Author

qryxip commented Jan 27, 2024

あーーーー

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

@qryxip
Copy link
Member Author

qryxip commented Jan 27, 2024

いや正確にはこっちか。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"},

@qryxip
Copy link
Member Author

qryxip commented Jan 27, 2024

#735

fn key(order: Option<u32>) -> impl Ord {
order
.map(Into::into)
.unwrap_or_else(|| u64::from(u32::MAX) + 1)
Copy link
Member

Choose a reason for hiding this comment

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

一応、「style_orderにないスタイルは一番後ろに足していく」みたいなことを書いておいた方がいいかも?
あとスタイルIDを足すようにしておくと順番が変わるのを抑えられる気がします。

Copy link
Member Author

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 どうしましょう?

Copy link
Member

Choose a reason for hiding this comment

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

mergeのdocstringに書いたやつでいいかなと思っています。

おっと、書いてありました(ごめんなさい)

無い/衝突した場合のフォールバック的な意味で

自分が意味したのは無い時のフォールバックですね。衝突時はそもそも原理的に起こらなさそう?

Copy link
Member

Choose a reason for hiding this comment

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

無い場合は元の順序にしましょう!
個人的には必須でも全然OKだと思います。ボイボはおそらく全部に付けるので。リリース前にミスに気づけるかもしれない。どっちでも!

衝突時も元の順序で良さそうかなと。
warn出るとちょっと嬉しいかも、くらい。(warn出すのどれくらい大変かわからずに言ってます 🙇 )

Copy link
Member Author

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するようにしています。マージ時にもう一度出す必要は無いかなと。

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.

素晴らしいですね!!!!
相変わらずRustは型がしっかりしたコードが何でも書けて羨ましいです。

ちょっと1個設計周りでコメントしました!!

crates/voicevox_core/src/metas.rs Outdated Show resolved Hide resolved
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!!

@y-chan vvmからスタイルにorderが入るので共有です。
song系は300番始まりくらいにしようかとか考えてます。
ちょっと迷い中です。

@qryxip qryxip merged commit 0f2c3b2 into VOICEVOX:main Feb 9, 2024
33 checks passed
@qryxip qryxip deleted the merge-speakers branch February 9, 2024 14:10
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