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

モーフィング機能の制限に対応 #1128

Closed
wants to merge 23 commits into from

Conversation

Segu-g
Copy link
Member

@Segu-g Segu-g commented Jan 19, 2023

内容

元のUIとの差分として

  • 選択肢に許可されたスタイル +既に選択されているキャラクターしか表示されない
  • 複数エンジンの時に跨ったエンジンのキャラを表示しない

が生じます。

関連 Issue

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

image
image
image
image

その他

@Segu-g Segu-g requested a review from a team as a code owner January 19, 2023 10:30
@Segu-g Segu-g requested review from Hiroshiba and removed request for a team January 19, 2023 10:30
@Segu-g Segu-g marked this pull request as draft January 19, 2023 10:31
@Segu-g
Copy link
Member Author

Segu-g commented Jan 19, 2023

@Hiroshiba
このPRで用いているopenapi.json
VOICEVOX/voicevox_engine#582
において0.14.0-preview.10のリリースを打つと仮定して__version__0.14.0-preview.10に変えて生成したものです。
typo修正を含まない方が良い場合はそのように言ってください

@Segu-g Segu-g marked this pull request as ready for review January 19, 2023 12:49
},
};
}).filter((characters) => characters.metas.styles.length >= 1),
];
});
Copy link
Member

Choose a reason for hiding this comment

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

もう1つUXレベルの判断です。 (実装してくださった @sabonerune さんの感謝も込めて・・・! )
今まででは全キャラ全スタイルが表示されていたのが、ここで可能なキャラだけになります。

そもそもですが、ボイボは8割のキャラが8割のキャラとモーフィング可能で、2割のキャラはほぼ不可となりそうです。
仕様としてどの形が良いか考えてみました。

最高なのはこうだと思います。

  • 選択可能なキャラが上に表示
  • 不可能なキャラが下に表示
  • 選択不可能な理由が書いてある

このパターンだと3番目の選択不可能な理由を書けるUIの用意が大変だと思いました。
次はこうかなと思います。

  • 選択可能なキャラだけが表示されている
  • 表示されないキャラクターがいることの理由がわかる

理由の表示はここのUIでできるとベターだと思いますが、なかなか案内が難しそうに感じます。
良い方法がない場合はQ&A+使い方で案内が良いかなと思いました。

なぜ全キャラを選択できるUIが厳しいのかというと、全キャラが選択不可能なキャラもいることがわかったためです。
このキャラを選んでいた場合の運の悪いユーザーのパターンだと、全キャラの選択を試して、全キャラダメだったとわかるというなかなかしんどそうなUXになってしまうためです。
元のUIがよくないわけではなく、想定が変わったことが今回の判断の変更理由になります。

もっと良い形もあるかもしれません、アイデア募集しています・・・!!!

(characterInfo) => characterInfo.metas.styles
);
const styleIds = styles.map((style) => style.styleId);
const morphablePairInfo = Object.fromEntries(
Copy link
Member

Choose a reason for hiding this comment

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

ここの二重ループはちょっとなんとかしたみがありますね 😇
とりあえず実際のエンジンで動かしてみて、まあ数秒で終わるならとりあえずこの実装で良いのかなと思いました!

そうじゃなかった場合は、エンジンAPIのisMorphableIsMorphableGetに、複数ペアを投げられるようにするのが良いのかなと思いました!

Copy link
Member

Choose a reason for hiding this comment

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

それでもGETクエリがとんでもない長さになりますね・・・・。

複数ペア投げられるようにし、キャラやスタイルが変わるたびにGETを投げるのはどうでしょうか。(N^1になる)
もしエンジン側の処理が重ければエディタ側でキャッシュするのも考えて良いかもです。

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

@Segu-g Segu-g Jan 21, 2023

Choose a reason for hiding this comment

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

試してみたところ、2000近いリクエストが同時に投げられる為一部のリクエスト以外失敗してしまうようでした.
/is_morphableAPIのようなスタイル-スタイル間のモーフィング可否ではなく、1スタイルに対してエンジン内の各スタイルがモーフィング可能かどうか返すmorphable_targetsAPIを生やしてリクエスト数を減らすとともに、固定サイズのLRUキャッシュで遂次取ってくる形に変えたいと思います.

Copy link
Member

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 left a comment

Choose a reason for hiding this comment

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

実装ありがとうございます!!!

いろいろコメントしましたが、ペアを作る部分に関しては実際に動かしてみてからかなと思いました!!

@Hiroshiba
Copy link
Member

@Segu-g モーフィング周り追加でお任せしちゃっても大丈夫ですか🙇‍♂️
お忙しかったり、当初から複雑度上がってしんどくなってたりしたら僕側でやっちゃうので遠慮なく言っていただければ!!

@@ -1117,7 +1237,18 @@ export const audioStore = createPartialStore<AudioStoreTypes>({
let blob: Blob;
Copy link
Member

Choose a reason for hiding this comment

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

この関数の上でgettersが使われてないというwarningが出てました。(githubの仕様でそこにコメントできず。。)

Comment on lines +111 to +112
morphableTargetsInfo: Record<string, MorphableTargetsInfo>;
morphableTargetsCacheKey: Record<string, Array<number>>;
Copy link
Member

Choose a reason for hiding this comment

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

numberになっているため何の値が入ってるか混乱しそうです。こうするとちょっとマシかも。

Suggested change
morphableTargetsInfo: Record<string, MorphableTargetsInfo>;
morphableTargetsCacheKey: Record<string, Array<number>>;
morphableTargetsInfo: Record<string, MorphableTargetsInfo>;
morphableTargetsCacheStyleIds: Record<string, Array<number>>;

@@ -310,6 +310,16 @@ export type PresetConfig = {
keys: string[];
};

export type MorphableTargetsInfo = {
Copy link
Member

Choose a reason for hiding this comment

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

s要らないかも?

Suggested change
export type MorphableTargetsInfo = {
export type MorphableTargetInfo = {

Comment on lines +384 to +409
SET_MORPHABLE_TARGETS: {
mutation(state, { engineId, baseStyleId, morphableTargets }) {
const prevIndex = state.morphableTargetsCacheKey[engineId].findIndex(
(styleId) => styleId === baseStyleId
);
if (prevIndex < 0 && morphableTargets) {
state.morphableTargetsCacheKey[engineId].splice(0, 0, baseStyleId);
// キャッシュ上限を超えた場合は削除する
if (
state.morphableTargetsCacheKey[engineId].length >
MORPHABLE_CACHE_LIMIT
) {
const postStyleId = state.morphableTargetsCacheKey[engineId].pop();
if (postStyleId !== undefined) {
delete state.morphableTargetsInfo[engineId][postStyleId];
}
}
state.morphableTargetsInfo[engineId][baseStyleId] = morphableTargets;
} else {
state.morphableTargetsCacheKey[engineId].splice(prevIndex, 1);
state.morphableTargetsCacheKey[engineId].splice(0, 0, baseStyleId);
if (morphableTargets)
state.morphableTargetsInfo[engineId][baseStyleId] = morphableTargets;
}
},
},
Copy link
Member

@Hiroshiba Hiroshiba Jan 24, 2023

Choose a reason for hiding this comment

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

このままでも全く問題ないのですが(正直ちゃんと追えてません。。)、ちょっと調べてみた感じMapは順序を持てることが保証されてるので結構簡単にキャッシュ機構を作れるっぽかったです。
ご興味あれば・・・!

Comment on lines +386 to +388
const prevIndex = state.morphableTargetsCacheKey[engineId].findIndex(
(styleId) => styleId === baseStyleId
);
Copy link
Member

Choose a reason for hiding this comment

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

indexOfでも良さそう?

Suggested change
const prevIndex = state.morphableTargetsCacheKey[engineId].findIndex(
(styleId) => styleId === baseStyleId
);
const prevIndex = state.morphableTargetsCacheKey[engineId].indexOf(baseStyleId);

Comment on lines +813 to +815
return morphableTargets === undefined
? !strictCache
: morphableTargets[targetVoice.styleId]?.isMorphable;
Copy link
Member

Choose a reason for hiding this comment

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

morphableTargets[targetVoice.styleId]?.isMorphableのところ、undefined | booleanが返ってそうですね 😇
意図をわかりやすくするとこうですかね(あまり変わってないかも)

Suggested change
return morphableTargets === undefined
? !strictCache
: morphableTargets[targetVoice.styleId]?.isMorphable;
if (morphableTargets == undefined) { return !strictCache; }
return morphableTargets[targetVoice.styleId]?.isMorphable == true

Comment on lines +796 to +800
/**
* 引数の2つのVoiceがモーフィング可能かどうか返す。
* strictCacheがtrueのときキャッシュが存在しない場合は不可とされる。
*/
IS_A_VALID_MOPHING_PAIR: {
Copy link
Member

Choose a reason for hiding this comment

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

「キャッシュが無いと不可とされる」の意図がわからなかったのですが、つまり「エンジンに問い合わせなくても無効だとわかる」という意味だったんですね!!!!!

Copy link
Member Author

Choose a reason for hiding this comment

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

strictCacheはキャッシュが存在しない場合に楽観的にvalidと判断するか、それとも悲観的にinvalidと判断するかを変更するフラグです。
strictCacheをfalseにした場合楽観的に判断され、trueにした場合悲観的に判断されます。

"""楽観的な判断でinvalidと判断された場合、キャッシュが存在したとしてもtrueになることはないのでエンジンに問い合わせなくても無効だと分かる"""、という意味で書かれているのがこの部分のコードです。

Copy link
Member

Choose a reason for hiding this comment

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

なるほどです。
意図はわかるのですが、キャッシュの仕様がこの関数内に無いので、キャッシュstateによってこの関数の挙動が変わることになりそうです。
この関数の利用者はキャッシュにあまり興味が無いはずなのに、キャッシュの有無による挙動の制御をすることになり大変だなと思った次第です。

たぶん役割の違う関数2つがまとまっているから難しくなっているのかなと思いました!

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.

ちょっと全部見きれてないです! 🙇‍♂️

このままでもたぶん良いのかなと思いつつ、流石にちょっと複雑かもと感じました。
またあとで見てみますが、ちょっと設計を考えてみたのでもし気が向いたら考えてみて頂けると・・・・・・・!!


INVALIDなのを弾くのではなく、VALIDなのを集める設計が良いのかなと思いました。
となると、同一エンジンでmophableTargetを取ってきたのを表示すればいいだけになりそうです。
IS_A_VALID_MOPHING_PAIRを不要にできるはず)

あとINITIALIZE_MORPHABLE_TARGETSSET_MORPHABLE_TARGETSするときに空だったら足す処理書くと消せるかもです。(好みがありそう)

いまさらなのですが、キャラ選択ボタンは選択後にモデルを読み込むからloading機能があることに気づきました。

これらを組み合わせると

  • キャッシュがあれば取得しなければ問い合わせてmophableを返すaction
  • キャッシュをセットするmutation
  • キャッシュを管理するstate

の3つで実現できるかもです。
・・・だいぶ自信ないです。
またあとで見たいと思います。もし興味あったら考えてみていただければ!!!

@Segu-g
Copy link
Member Author

Segu-g commented Jan 24, 2023

@Hiroshiba

こちらのPRですが今日明日に学業の方で喫緊のタスクが入り手を付けられそうにないので、中途半端になりますが残りの修正お任せしたいです🙇‍♂️

@Hiroshiba
Copy link
Member

@Segu-g おお!!連絡ありがとうございます、承知しました!!
通常であればお待ちしたいところなのですが、もうリリース予定まで1週間切ってる関係でちょっと急がせていただこうと思います 🙇‍♂️
PRを頂けてかなりこの辺りの解像度が上がったので、僕が続きを書いてみたいと思います!

@Hiroshiba
Copy link
Member

@Segu-g さん
改変させていただいたプルリクの方を取り込ませていただきました 🙇
(お忙しいかと思ってレビューは @.sabonerune さんにお願いさせていただきました。)

ちょっと思ってた実装と離れちゃってたらすみません。
でもとても助かりました、ありがとうございます・・・!

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.

3 participants