Closed y-chan closed 1 year ago
マイグレーション
現状でも24000Hzのときに"default"にする処理が↓に書いてあるので、ここ書き換えればOKだと思います! https://github.com/VOICEVOX/voicevox/blob/72b61d9d8dde0008f8090a7f1bcec3f5d59d81d5/src/background.ts#L535-L540
問題はVuex側・API側のデータ構造がオブジェクトになっていないという点ですね・・・。
現状、electron-storeとVuexはoutputSamplingRate: number | "default"
型で、APIはoutputSamplingRate: number
です。
Vuex←→APIでデータ構造の変換が必要ですが、これはここに書いてます。 https://github.com/VOICEVOX/voicevox/blob/65ad4137b8617d77da99b81085206ef54fa71517/src/store/proxy.ts#L36-L47
同様にelectron-store←→Vuexでデータ構造を変換するか、Vuex側をelectron-storeと同じオブジェクトにするか、という感じでしょうか。
ちなみに1回変換が必要なコードを書いたのですが、ややこしいなと思って保存側をUI側と同じ型にして型変換不要にしてみて、今に至ります!
f686ac7
(#973)
型の変換が不要で、かつjsonで素直に表現できると良さそう・・・?
@yamachu さんの意見も伺いたいです 🙏
個人的な温度感はこんな感じです
electron-store
←→Vuexでの型変換はなるべく避けたい
number | string literal
の型で保存するのはそこまで悪くないと思う
electron-store
が依存してるJSON SchemaもTypeScriptも対応してるため{isDefault: boolean, samplingRate: number}
は ややこしい気がする
{isDefault: false, samplingRate: number} | {isDefault: true, samplingRate: undefined}
とかになり、JSON Schemaがより複雑になりそう{isDefault: boolean, samplingRate: number}
でも別に良いかもsamplingRateに関する値をAPIとどのようにやり取りをしているのか、またengineのmanifestからどんな感じでengineが使用するdefaultのsamplingRateを取得するのかなど確認しました。 それを踏まえて…
number | 'default'
みたいな型でも別にいいんじゃないか
default
が何を指しているのかが分かりづらい気がするengineDefault
なのか…?うーん{ useEngineDefault: true } | { useEngineDefault: false, samplingRate: number}
にしそう
ってことで、number | 'default'
みたいな形で表現できるうちは、これで進めてしまって良いんじゃないかとは思いました。
ありがとうございます!!
engineDefault
、賛成です(たしかに何指すのかわかりづらそうだったので)
冗長かもですがそんなに複雑でもないので良さそう!
number | 'engineDefault'
で行くか{ useEngineDefault: true } | { useEngineDefault: false, samplingRate: number}
で行くかですが、
これで進めてしまって良いんじゃないか
に僕も賛成です!
とりあえずこの形で進めてみたいと思います。
engineDefault
にするPRを作ります!
@y-chan @yamachu 作成してみました!
内容
925 にて、エンジンのデフォルトサンプリングレートを使うようにする機能が実装されました。
default
文字列をelectron-storeに保存する実装が適切でない可能性が指摘されました。electron-store
における、音声出力サンプリングレートの保存方法の変更を議論します。Pros 良くなる点
現状のあまり良くないと思わしき仕様を改善することができる。
Cons 悪くなる点
おそらくマイグレーションを行うことが必須である。
VOICEVOXのバージョン
0.14.0 (Development)