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

ENH: モーフィングUIの実装 #1030

Merged
merged 15 commits into from
Jan 16, 2023

Conversation

sabonerune
Copy link
Contributor

@sabonerune sabonerune commented Dec 1, 2022

内容

モーフィングを適用するためのUIを追加します。

関連 Issue

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.

PRありがとうございます!!

こちらもいくつかコメントしたのですが、修正確認するにはおそらくモーフィングUI自体が必要だと思うので、モーフィングUIのPRにこちらも含めていただいても大丈夫です!!

src/type/preload.ts Outdated Show resolved Hide resolved
src/store/audio.ts Outdated Show resolved Hide resolved
@sabonerune sabonerune marked this pull request as draft December 3, 2022 14:32
@sabonerune sabonerune force-pushed the morphing_ui/audioStore branch from d5a6d91 to a35afc6 Compare December 5, 2022 09:53
@sabonerune sabonerune force-pushed the morphing_ui/audioStore branch from ce49463 to 9a15da6 Compare December 20, 2022 10:09
@sabonerune sabonerune changed the title ENH: audioStoreにモーフィング適用するための操作を追加 ENH: モーフィングUIの実装 Dec 20, 2022
@@ -24,9 +24,9 @@
transition-show="none"
transition-hide="none"
>
<q-list>
<q-list style="min-width: max-content">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

右方向に十分スペースがなかったときスタイル名が改行されてしまう問題への対処

Comment on lines 697 to 710
VALID_MOPHING_INFO: {
getter: (state, getters) => (audioItem: AudioItem) => {
if (
!state.experimentalSetting.enableMorphing ||
audioItem.morphingInfo == undefined ||
audioItem.engineId == undefined
)
return false;
return (
getters.SELECTABLE_MOPHING_TARGET_ENGINES.includes(
audioItem.engineId
) && audioItem.engineId === audioItem.morphingInfo.targetEngineId
);
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

AudioItemがモーフィングを実行するか判定する関数。
命名が思い浮かばず。

Copy link
Member

Choose a reason for hiding this comment

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

validは形容詞らしいので、まあVALID_MOPHING_INFOかなと思いました。(自信ないです。。)

@sabonerune
Copy link
Contributor Author

最初はモーフィングが不可能なスタイルが設定された場合すぐに強制的に解除するように考えていましたが、
とりあえずどんな組み合わせも許容してAudioInfo上で警告、音声生成時はそのままモーフィングをせずにそのまま出力するという考え方になりました。

@@ -51,6 +52,7 @@ async function generateUniqueIdAndQuery(
audioQuery,
audioItem.engineId,
audioItem.styleId,
audioItem.morphingInfo, // FIXME: モーフィング非対応エンジンの場合morphingInfoが異なっていても同じ音声が出力されるが異なるIDが生成されてしまう
Copy link
Contributor Author

Choose a reason for hiding this comment

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

getters.VALID_MOPHING_INFO(audioItem) ? audioItem.morphingInfo : undefined,

で解決できそうだがgettersをどこから持ってくればいいのか分からず。

Copy link
Member

Choose a reason for hiding this comment

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

まあ非対応エンジンでは音声を生成できずキャッシュが作れない関係でUniqueIdを利用する機会無いので、FIXしなくても良いかもと思いました!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

非対応エンジンでも今のところ無視して通常の合成をしてしまうので無駄が出てしまいますね。

あと、実験的機能の設定でモーフィングが無効になっている場合でmorphingInfo が設定されている場合(プロジェクトファイルを読み込んだ等)モーフィングをせずに出力するようにしているのですがその後設定を変えるとキャッシュからモーフィングされていない音声が再生されてしまうことにきづきました。

モーフィングをしてしまえばこの部分の対処は楽ですがそうなるとモーフィングをかけていることに気づけるUIがないことが問題になりますね…

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
Contributor 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 8, 2023

Choose a reason for hiding this comment

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

あ、意図を把握しました!

モーフィングOFFにした場合、UIが消えるだけで、モーフィング済み結果を再生する形で良いかなと思います。
仰る通り気づくUIがないのは問題ですが、ぶっちゃけハマる人はかなりレアケースだと思いました。0人かもしれない。

まあでも問題を把握するのは大事なので、どこかにFIXMEコメントを書くか、issueを作るかが良いかなと思いました!

@sabonerune sabonerune marked this pull request as ready for review December 20, 2022 13:03
@Hiroshiba
Copy link
Member

とりあえずどんな組み合わせも許容してAudioInfo上で警告、音声生成時はそのままモーフィングをせずにそのまま出力するという考え方になりました。

エラー出そうと思うとかなり大変だと思う+まあまあなコーナケースなので、一旦その方針で問題ないと思います!
(どこかにFIXMEコメント残しとくと誰か気づけるかもですね)

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.

良いですね!!!
とても大変だったと思います、ありがとうございます!!

名称変更をいっぱい提案していますが、別にどれも致命的ではないのでそのままでも問題ないと思います。
気にいったものだけ取り込んで頂ければ・・・!

Comment on lines +372 to +391
<div class="text-body2 text-no-wrap ellipsis overflow-hidden">
{{
morphingTargetCharacterInfo
? morphingTargetCharacterInfo.metas.speakerName
: "未設定"
}}
</div>
<div
v-if="
morphingTargetCharacterInfo &&
morphingTargetCharacterInfo.metas.styles.length >= 2
"
class="text-body2 text-no-wrap ellipsis overflow-hidden"
>
({{
morphingTargetStyleInfo
? morphingTargetStyleInfo.styleName
: undefined
}})
</div>
Copy link
Member

Choose a reason for hiding this comment

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

スタイル選択ボタンの表記と合わせて横並びにすると馴染み深いかもと思いました!
image

Suggested change
<div class="text-body2 text-no-wrap ellipsis overflow-hidden">
{{
morphingTargetCharacterInfo
? morphingTargetCharacterInfo.metas.speakerName
: "未設定"
}}
</div>
<div
v-if="
morphingTargetCharacterInfo &&
morphingTargetCharacterInfo.metas.styles.length >= 2
"
class="text-body2 text-no-wrap ellipsis overflow-hidden"
>
({{
morphingTargetStyleInfo
? morphingTargetStyleInfo.styleName
: undefined
}})
</div>
<div class="text-body2 text-no-wrap ellipsis overflow-hidden">
{{
morphingTargetCharacterInfo
? morphingTargetCharacterInfo.metas.speakerName +
(morphingTargetStyleInfo &&
morphingTargetCharacterInfo.metas.styles.length >= 2
? ` (${morphingTargetStyleInfo.styleName})`
: "")
: "未設定"
}}
</div>

まあAudioInfoを最小幅にすると消えちゃうのですが・・・。
(デザインは全体的に見て最終調整するので、そのままにしていただいても大丈夫です!)

src/components/AudioInfo.vue Outdated Show resolved Hide resolved
@@ -51,6 +52,7 @@ async function generateUniqueIdAndQuery(
audioQuery,
audioItem.engineId,
audioItem.styleId,
audioItem.morphingInfo, // FIXME: モーフィング非対応エンジンの場合morphingInfoが異なっていても同じ音声が出力されるが異なるIDが生成されてしまう
Copy link
Member

Choose a reason for hiding this comment

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

まあ非対応エンジンでは音声を生成できずキャッシュが作れない関係でUniqueIdを利用する機会無いので、FIXしなくても良いかもと思いました!

Comment on lines 697 to 710
VALID_MOPHING_INFO: {
getter: (state, getters) => (audioItem: AudioItem) => {
if (
!state.experimentalSetting.enableMorphing ||
audioItem.morphingInfo == undefined ||
audioItem.engineId == undefined
)
return false;
return (
getters.SELECTABLE_MOPHING_TARGET_ENGINES.includes(
audioItem.engineId
) && audioItem.engineId === audioItem.morphingInfo.targetEngineId
);
},
Copy link
Member

Choose a reason for hiding this comment

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

validは形容詞らしいので、まあVALID_MOPHING_INFOかなと思いました。(自信ないです。。)

src/components/AudioInfo.vue Outdated Show resolved Hide resolved
src/store/audio.ts Outdated Show resolved Hide resolved
src/store/audio.ts Outdated Show resolved Hide resolved
src/components/AudioInfo.vue Outdated Show resolved Hide resolved
src/components/AudioInfo.vue Outdated Show resolved Hide resolved
src/components/AudioInfo.vue Outdated Show resolved Hide resolved
if (
!state.experimentalSetting.enableMorphing ||
Copy link
Contributor Author

Choose a reason for hiding this comment

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

VALID_MOPHING_INFOの判定からモーフィングの設定を外しました。


return dispatch("INSTANTIATE_ENGINE_CONNECTOR", {
engineId,
})
.then((instance) => {
if (
audioItem.morphingInfo != undefined &&
state.experimentalSetting.enableMorphing &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

モーフィングの設定は音声合成時に確認します

@sabonerune
Copy link
Contributor Author

一通り直しました。

モーフィングができないときの処理と表示について考える必要がありそうです。

@@ -83,7 +87,7 @@ export async function generateAndSaveAllAudioWithDialog({

const successArray: Array<string | undefined> = [];
const writeErrorArray: Array<WriteErrorTypeForSaveAllResultDialog> = [];
const engineErrorArray: Array<string | undefined> = [];
const engineErrorArray: Array<WriteErrorTypeForSaveAllResultDialog> = [];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

暫定的に既存の型をそのまま流用。
エラーダイアログを出す方針が確定したらWriteErrorEngineError両方で使える型名に変更するか新しく型を定義する。

Comment on lines +27 to +30
<q-item-section>
<q-item-label>{{ value.path }}</q-item-label>
<q-item-label v-if="value.message"
>詳細:{{ value.message }}</q-item-label
Copy link
Contributor Author

Choose a reason for hiding this comment

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

すみません。
何故かcrlfの変換をかけてしまいました。

エンジンエラーの詳細を追加しました。

</template>

<script lang="ts">
import { WriteErrorTypeForSaveAllResultDialog } from "@/store/type";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

型付けのためのインポート追加

Comment on lines 62 to 74
props: {
successArray: {
type: Array as PropType<string | undefined[]>,
required: true,
},
writeErrorArray: {
type: Array as PropType<WriteErrorTypeForSaveAllResultDialog[]>,
required: true,
},
engineErrorArray: {
type: Array as PropType<WriteErrorTypeForSaveAllResultDialog[]>,
required: true,
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

propsを型付け

Comment on lines 71 to 80
export type GenerateAudioResultObject =
| {
result: "SUCCESS";
blob: Blob;
}
| {
result: "VALID_MOPHING_ERROR" | "EDITOR_ERROR" | "ENGINE_ERROR";
errorMessage?: string;
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GENERATE_AND_SAVE_AUDIOにならってGenerateAudioResultObject型を追加してエラーの内容とメッセージを追加。

エラーの種別は必要なかったかもしれない?

Copy link
Member

@Hiroshiba Hiroshiba Jan 3, 2023

Choose a reason for hiding this comment

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

AND_SAVEではないGENERATE_AUDIO系は一旦throwのままでも良いかもと思いました!
現状なにか値を返すことがなく、型とメッセージで完結してるので・・・!
new Error("hoge")で指定した文字列はe.messageで取得できるはず)

どこかでエラーハンドリングし忘れてthrowできてない可能性を極力なくしたいためです。
(なのですが、そもそもエラーハンドリングをどうすべきかまだ結論が出せてなくて、将来は気持ちが変わってるかもです・・・ 🙇‍♂️ )

@sabonerune
Copy link
Contributor Author

暫定的にエラーダイアログにモーフィングの設定が間違っていることが表示されるようにへんこうしてみました。
よさそうなら進めてみます。

@Hiroshiba
Copy link
Member

完了したら合図頂ければ・・・!

@sabonerune
Copy link
Contributor Author

すいません、コメントしたつもりになって送信できていませんでした。
修正しましたので確認お願いします。

@sabonerune sabonerune requested review from Hiroshiba and removed request for y-chan January 7, 2023 04:10
@Hiroshiba Hiroshiba mentioned this pull request Jan 8, 2023
2 tasks
@sabonerune
Copy link
Contributor Author

とりあえずFIXMEコメント付けました。

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!!!!!!!!!!

お疲れ様でした!!!!
VOICEVOXエディタはなかなか複雑な構造をしているのですが、問題を的確に把握し、ガッツを持って取り組んでくださってとても嬉しかったです!!!
よければまたプルリクや提案issue頂けると嬉しいです、ありがとうございました!!

@Hiroshiba
Copy link
Member

あ、zod化にともなってコンフリクトが起こっているようです 🙇‍♂️
uuidなんかは確か string().uuid() とかで指定可能なので短縮できるかもです!

エンジン側でモーフィングを実装してくださった @mes51 さん、せっかくなのでもしよければ実際にUIを触ってみたときのレビュー頂けませんか 👀
(コードはかなり事前知識のいる内容なため、UIを結構見て頂けると心強いです!!)

@mes51
Copy link
Contributor

mes51 commented Jan 14, 2023

見させていただきまして、概ね問題無さそうでした。
ただ、(おそらくこれに限らずなので別件ではありますが)1点、ウインドウサイズによってはキャラクター選択のメニューが上方向に伸びてしまうため、一番上の選択肢がタイトルバーの領域と被ってしまい、操作がタイトルバーに吸われてしまうようです。

Desktop.2023.01.14.-.20.46.59.02_1.mp4

この問題に関しては、おそらくポップアップで出てくるUI共通の問題だと思う(MenuBar-webkit-app-region: dragがマウス操作を全部吸ってしまっている)ので別でIssueを立てるほうが良いかもしれません
おそらく関連するIssue: electron/electron#1354

@Hiroshiba
Copy link
Member

お試しありがとうございます!!

とりあえずいったんは表示される方向を下にするとかで解決可能なのかなと思いました!
キャラクター並び替えで一番上にお気に入りのキャラが表示されるので、カーソルの近さ的にもそっちのがユーザビリティが高いのかな~と!!

@sabonerune
Copy link
Contributor Author

マージできました。

@sabonerune
Copy link
Contributor Author

とりあえずいったんは表示される方向を下にするとかで解決可能なのかなと思いました! キャラクター並び替えで一番上にお気に入りのキャラが表示されるので、カーソルの近さ的にもそっちのがユーザビリティが高いのかな~と!!

メニューの方向を下に固定することが思ったより難しそうです。
QMenuは展開方向にスペースが足りないとき、自動的に展開方向を反転するのですがその動作を止める方法が見つけられませんでした。

QMenuのmax-heightに下方向の空きスペースのサイズを入れるという手もありそうですが自分には難しそうな感じがします。

@Hiroshiba
Copy link
Member

なるほどです!!anchorを強制する方法、確かになさそうですね。。。
max heightのアイデア面白いです、思いつきませんでした!

うーん、仕方ない気がします!諦めましょう…!
キャラクターの非表示機能が必要かもですね…。
調査ありがとうございました!!!

@Hiroshiba
Copy link
Member

あ、コンフリクト解消でプリセット保存回りの形情報が抜けたかもです!

type/preload.tsにある
postPhonemeLength: z.number(),
の下あたりに設定が必要かなと!

これ型不一致エラーどこにも出てないっぽいですね。。難しい。。

@sevenc-nanashi
Copy link
Member

強引に、q-menuの要素に-webkit-app-region: no-dragを設定する、でも対策できそう?(未検証)

@Hiroshiba
Copy link
Member

@sevenc-nanashi
あ!
それでメニューバーに重なる部分は解決なのですが、個人的にはお気に入りキャラが遠い位置に表示されるのが特に問題だと考えてます!

@Hiroshiba
Copy link
Member

zod周りも問題なさそうだったのでマージしたいと思います!!

かなり長い期間の調整、広範囲の影響にも関わらず取り組んでいただき本当にありがとうございました!!!
こちらはなかなかに面白い機能なので、プレビュー版先行テストのアピールの直前辺りで報告して興味を持ってもらおうと思います!

改めて @mes51 さん、 @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.

モーフィング機能のUI実装
4 participants