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

デフォルトプリセット周りのリファクタ #1247

Closed
k-chop opened this issue Mar 10, 2023 · 6 comments · Fixed by #1274
Closed

デフォルトプリセット周りのリファクタ #1247

k-chop opened this issue Mar 10, 2023 · 6 comments · Fixed by #1274
Assignees

Comments

@k-chop
Copy link
Contributor

k-chop commented Mar 10, 2023

内容

#1228 のPRがだいぶ長くなってしまったため、
クリティカルではないソースコード上の問題をいくつか残したまま #1288 をマージしていただく予定です。
そこで残した問題を解決するissueです。引き続き @k-chop が取り組みます。

問題一覧

  • mutation内で渡した引数を直接書き換えているコードをどうにかする
  • determineNextPresetKey の引数を整理する
  • defaultPresetか判定する関数がcomposablesに存在するが、vuexでも同様の判定をしていて重複している
    • デフォルトプリセットを追加 #1228 (comment)
    • getterにO(1)で判定できるようにしたデータの中間表現を置いて、それを使って判定できるようにすると良さそう
    • composablesはコンポーネント上の処理を適切な粒度で切り出せる先としてぜひ使ってほしいので、上記措置で内容が薄くなってもそのまま残す...残したい...

VOICEVOXのバージョン

latest

@Hiroshiba
Copy link
Member

Hiroshiba commented Mar 10, 2023

issue作成ありがとうございます!!

@k-chop
Copy link
Contributor Author

k-chop commented Apr 1, 2023

元PRのやりとりが長かったので、対応してないまま忘れてる問題がないか再度チェックしたものメモ:
(メモなのであとでissue立てたり結局何を明確にすべきなのか等改めてまとめます)


#1228 (comment)

↑の仕様にすると、プリセット値に戻らないことが不便かもなので、「プリセット値に戻す」機能があると良さそうかなと思いました。
これは別issueが良いかも・・・?(アイコン選びなどが必要そうなので)

issue立てておいた方がよさそう


#1228 (comment)

パラメータ引き継ぎ の対象にプリセットも含む

これ周知が必要かも?
次リリースverまでに「今までの挙動」に合わせられるようなオプション追加しておくべきか?
それとも↓で解決できるか?


#1228 (comment)

このPRではオーバーな気がしますが、理想としてはコピー時・キャラ変時の挙動を別々に選んでカスタマイズできるようにするのが良いのかなと思いました。
たぶん、望まれる処理はパラメータ↓の4パターンを選べることかなと。

  • 引き継ぐ
  • デフォルト値に戻す
  • デフォルトプリセットを適用する
  • コピーしてプリセットを適用する
    これをコピー・キャラ変でそれぞれ選べる、みたいな・・・。

これ次リリースまでにあった方が良い?
今まで台本読み込んだときにプリセット1つずつ当てるのつらい→最初のアイテムにプリセット当ててから読み込めば全部それが当たるよ!!という案内を何度か観測している
次リリースで「パラメータ引継ぎがOFF」の場合はデフォルトプリセットが当たるようになる
パラメータ引継ぎOFFで今までと同じ操作をしようとしたときできなくなるユーザが現れそう

という問題がこれで解決できるのかどうかよくわかっていない

「コピー」はユーザに伝わらないので適切なワードを選ぶ必要がある
(+ボタンを押したときは「コピー」されるが、AudioCellをコピーしているというのは開発者しか知りようがない)

それはそれとして

台本読み込んだときにプリセット1つずつ当てるのつらい

これテキスト読み込みや長文コピペしたときに、どのキャラとプリセットを選択するか選べるUIが出てきたら便利かも?と思っている
image


#1228 (comment)

今確認したらリリースされてました!! 🙌
https://github.com/colinhacks/zod/releases/tag/v3.21.0
(直すのは別PRが良さそう感)

これ別PRで試してみたのですが直ってなさそう・・・?(Partialついたまんま)
忘れそうなので別issueにしとくのがいいかも
後のコミットで実装が変えられてるけどこれが影響している? colinhacks/zod@defdab9


@Hiroshiba
Copy link
Member

これ周知が必要かも?

パラメータ引き継ぎの仕様変更の周知に関して、とりあえずリリース段取りでメモってるとこに追加しておきました!
VOICEVOX/voicevox_project#22

これ次リリースまでにあった方が良い?

あーーー。
パラメータ引き継ぎの仕様変更にこまる方がいるかもなので、あった方が良さそうに思いました。
マストではない・・・かなぁ・・・。うーん。
あったほうが絶対便利に思いました。

テキスト読み込みや長文コピペしたときに、どのキャラとプリセットを選択するか選べるUIが出てきたら便利かも?

便利だと思います!
一方で、プリセットを使う人&コピペやテキスト読み込みを用いる人が対象なので、どれくらいのニーズがあるのか(優先度)が読めなそうだけど、変更量が結構大掛かりになっちゃうなーーーと思いました。
そう考えると、ほしいのはもしかしたらプリセットを一括で変更する機能かも・・・?

これ別PRで試してみたのですが直ってなさそう・・・?(Partialついたまんま)

謎ですね。。。
もしそうだったらしれっと戻すPR出しますか!

@k-chop
Copy link
Contributor Author

k-chop commented Apr 5, 2023

雑メモのまま直さず忘れてしまってました...失礼しました 🙇


パラメータ引き継ぎの仕様変更にこまる方がいるかもなので、あった方が良さそうに思いました。

了解です!issue立ててさっと取り組んでみます。
設定UI作ったらdetermineNextPresetKeyの中で設定値見て分岐するだけなのでそれほど重くはなさそうです。

  • AudioItemのコピー時は「コピーしてプリセットを適用する」
  • ボイス変更時は「引き継ぐ」

でたぶん現行と同じ操作になるはずという認識...

どれくらいのニーズがあるのか(優先度)が読めなそう

あーーたしかに!
twitter見ててその話してる人がそこそこ見つかるので、これ結構使う人が多いのか~需要あるのかな?と思ってたんですが、
単にそれを使ってる人が困りごとに当たって発信しやすかっただけで、ニーズがでかい訳ではなさそうな気もしますね。
ひとまずおいとこうと思います。

zodのやつ

ちょっと実験してみたのですが、
現verのzodでもrecord自体のinferは特に問題なかったようです。
objectやarrayの子になると謎のPartialに包まれる感じでした。
このinfer結果はVOICEVOXが使ってる現verも、Hiroshibaさんの対応が入った最新版も変わらなかった...
ZodObjectはだいぶ複雑で修正沼りそう 😇

aaa
このinfer結果は現ver(3.20.2)のものです

@k-chop
Copy link
Contributor Author

k-chop commented Apr 5, 2023

@Hiroshiba
Copy link
Member

Hiroshiba commented Apr 8, 2023

ちょっと実験してみたのですが、
現verのzodでもrecord自体のinferは特に問題なかったようです。
objectやarrayの子になると謎のPartialに包まれる感じでした。

おお・・・・・。なるほど・・・・・・・・・・・・・・。

issue立てました、やっていきます 💪

issue内容完璧でした、ありがとうございます!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants