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

キャラクター並び替えダイアログ: speakerUuidが異なる場合にstyleIdの重複を許容する #754

Merged

Conversation

aoirint
Copy link
Member

@aoirint aoirint commented Mar 14, 2022

内容

キャラクター並び替えダイアログ(CharacterOrderDialog)について、speakerUuidが異なるキャラクターが重複したstyleIdを持つことを許容する実装にします。

複数エンジン対応時に、スタイル付きのキャラクターを一意に識別する方法については、
engineIdstyleIdの2つをユニークキーにする方針 #609 (comment) がありますが、
簡易な修正で済むため、キャラクター並び替えUIについては、いったんspeakerUuidstyleIdで一意に識別するように実装してみました。

また、props.characerInfosが初回setup呼び出し後に書き換えられたとき、書き換えによって追加されたキャラクターのサンプルボイスを再生できない現象を修正します。

この現象は、selectedStyleIndexesが初回setup呼び出し時に計算された初期値(またはその後に変更された値)を保持するため、props.characterInfos書き換え時の再描画でselectedStylesが再計算されないことで発生します。

selectedStyleIndexesに初期値を与える実装を削除し、selectedStyleIndexes[speakerUuid]undefinedのとき、selectedStyleIndexes[speakerUuid]=0(0番目のスタイル)とみなす実装にして修正します(?? 0のところ)。

関連 Issue

スクリーンショット・動画など

その他

@aoirint aoirint changed the title キャラクター並び替えUI: speakerUuidが異なる場合にstyleIdの重複を許容する キャラクター並び替えダイアログ: speakerUuidが異なる場合にstyleIdの重複を許容する Mar 14, 2022
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.

なるほど・・・
ここのコードを書いたのは僕なのですが、styleIdが一意である前提で書いているという意識がありませんでした・・・。
修正ありがとうございます!!!

1箇所、undefinedableに関して少し折衷案を出してみました。

src/components/CharacterOrderDialog.vue Outdated Show resolved Hide resolved
Comment on lines +246 to +247
const selectedStyleIndex: number | undefined =
selectedStyleIndexes.value[characterInfo.metas.speakerUuid];
Copy link
Member

Choose a reason for hiding this comment

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

同上で、undefinedになる可能性が無いのであれば| undefinedは逆に混乱しちゃうかなと感じました。
(もしくはここはundefinedableだったりする・・・?)

Copy link
Member Author

@aoirint aoirint Mar 26, 2022

Choose a reason for hiding this comment

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

こちらについては、selectedStyleIndexはundefinedになる可能性があります。

このPRの変更前の実装では、selectedStyleIndexesにはprops.characterInfosに基づく初期値が設定されていましたが、このPRでは、初期値を空objectに変更しています。
その結果、selectedStyleIndexを更新する処理であるrollStyleIndexが一度も実行されていないキャラクターは、selectedStyleIndexがundefinedであるようになっています。

styleIdの重複許容に対して余計な差分が入っていてわかりにくく申し訳ないですが、これは複数エンジン対応時を想定した挙動の修正です。

従来の実装では、props.characerInfosが後で(初回setup呼び出し後に)書き換えられたとき、selectedStyleIndexesの値が再計算されない(refの初期値または変更された値が維持される)ため、書き換え後に新しく含まれるようになったキャラクターについてのみ、selectedStyleIndexがundefinedになる形でした。
setup実行時のprops.characterInfosの値によらず、挙動をそろえるためにselectedStyleIndexの初期値をundefinedに統一しています。

Copy link
Member

Choose a reason for hiding this comment

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

なるほどです!!

より良く意図を反映できるコードを思いつきました!!
たぶんなのですが、objectにキーがない場合にundefinedになる感じですよね。
であれば、キーの有無で処理を分岐させるのが一番意図が伝わりやすいかも…?

(でもまあここのコードはここでしか使われないので、正直どちらでもそこまで問題ないかなとも思いました…!)

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.

大丈夫だと思うのでマージします・・・!

@Hiroshiba Hiroshiba merged commit 1f199df into VOICEVOX:main Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants