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

ソングエディタの編集状態を保存できるようにする #1829

Merged
merged 29 commits into from
Feb 17, 2024

Conversation

y-chan
Copy link
Member

@y-chan y-chan commented Feb 6, 2024

内容

題の通りです。ソングエディタの状態をプロジェクトとして保存できるようにします。
また、これに合わせてプロジェクトの構造を変更しました。

関連 Issue

close #1781

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

image

その他

zodのスキーマを有効活用するために一部リファクタリングを行っています。

また、現状ではソング部分のプロジェクトの変更検知を行っていない(変更検知がCOMMANDに依存しているため)ので、変更していてもプロジェクトを保存せずに閉じれてしまう問題があります。wipでもいいので、ソング部分のプロジェクト変更検知機能を付けるか迷ったのですが、変更前のプロジェクトを保持しておく以外に変更検知をする手段が思いつかなかったので、このPRでは取り組んでいません。

@y-chan y-chan requested a review from a team as a code owner February 6, 2024 22:12
@y-chan y-chan requested review from Hiroshiba and removed request for a team February 6, 2024 22:12
src/store/project.ts Outdated Show resolved Hide resolved
src/store/project.ts Outdated Show resolved Hide resolved
src/store/type.ts Outdated Show resolved Hide resolved
prevAudioKey = await context.dispatch("REGISTER_AUDIO_ITEM", {
prevAudioKey,
audioItem,
});
}

const { tpqn, tempos, timeSignatures, tracks, phrases } =
parsedProjectData.sing;
Copy link
Member

@Hiroshiba Hiroshiba Feb 7, 2024

Choose a reason for hiding this comment

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

あ、名詞のsongのが良さそうかもです!

(エディタ結構singになってるとこあるので、songにするissue作ってみました)

@y-chan y-chan changed the title シングエディタの編集状態を保存できるようにする ソングエディタの編集状態を保存できるようにする Feb 7, 2024
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.

とりあえず一旦コメントです・・・!

value.state = "WAITING_TO_BE_RENDERED";
context.commit("SET_PHRASE", { phraseKey: key, phrase: value });
}
context.dispatch("RENDER");
Copy link
Member

Choose a reason for hiding this comment

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

これを実行する責務はLOAD_PROJECT_FILEを呼んだ側かも

Copy link
Member Author

Choose a reason for hiding this comment

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

なるほどです、確かにそうですね...!

修正しようと思います....!

Comment on lines 425 to 426
value.state = "WAITING_TO_BE_RENDERED";
context.commit("SET_PHRASE", { phraseKey: key, phrase: value });
Copy link
Member

Choose a reason for hiding this comment

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

stateが入っているものをproejctとして保存するのちょっと違和感あるかもです!
phraseはレンダリング用に作られるキャッシュで、保存しなくても良いものかも・・・?
@sigprogramming さんが詳しいかも)

Copy link
Member

Choose a reason for hiding this comment

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

あ、phrase内にqueryがあるから違うかも。stateだけ保存しないようにするのが良い気がしました!
(たぶんphraseにidを持たせて、phraseStateだけメモリに持つ構成が合ってるかも)

Copy link
Contributor

Choose a reason for hiding this comment

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

ピッチやボリュームの編集データはqueryとは別で持つ形になると思うので、phraseは保存しなくても良いかなと思います!(キャッシュの保存についてはまた後ほど考慮できれば…!)

Copy link
Member Author

Choose a reason for hiding this comment

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

なるほどです...!
ピッチやボリュームのデータはリクエストする度に差異が発生する可能性があるので、できれば今の内から一度リクエストされたものは保存するようにと思っていたのですが、一旦phrasesは保存しない方向で行きたいと思います!

src/store/project.ts Outdated Show resolved Hide resolved
src/store/project.ts Show resolved Hide resolved
@y-chan
Copy link
Member Author

y-chan commented Feb 7, 2024

Discordで議論した結果をまとめておきます

議題: ソングとトークでプロジェクトを分けて保存すべきか?

議論の始まり: https://discord.com/channels/879570910208733277/893889888208977960/1204721860189757460
結論: https://discord.com/channels/879570910208733277/893889888208977960/1204745814375931924

  • 結論としては一旦両方とも同じプロジェクトに保存
    • 実装コストとか、データ消失リスクとか
    • リスクについて、両方持たせると
      • セーブ時の想定外が減る
      • ロード時に想定外(もう片方が空で上書きされた)になるかも?
    • 片方だけにすると
      • セーブ時に想定外(もう片方が保存されてなかった)になるかも?
      • ロード時に想定外にならない
    • 多分、どっちにしろ変更検知を実装すれば防げる、一旦はセーブ時想定外を減らす方がリスク的には小さそう(一旦不幸が少なさそうな方向)
  • 将来的に分けて保存する機能・片方だけ読み込みできる機能とか付けられるとよさそう
    • 大多数の人はどっちでもいいはず、片方だけ保存出来ないと困る人や、両方保存できないと困る人は両者ともに少数派な気がする
    • みんなを救うなら分けて保存を実装すればいい
  • 実装は、読み込み・保存を分けられるように組めるとよさそう
    • マイグレーションとか、関数化しておけると ⭕

src/store/project.ts Outdated Show resolved Hide resolved
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です!

プロジェクトファイル実装したい気持ちはあるのですが、ちょっとコードぐちゃっとしそうなので、少しリファクタリングしてから実装できればと思ってます。

src/store/project.ts Outdated Show resolved Hide resolved
src/store/project.ts Show resolved Hide resolved
src/store/project.ts Outdated Show resolved Hide resolved
src/store/project.ts Outdated Show resolved Hide resolved
src/store/project.ts Show resolved Hide resolved
src/store/project.ts Outdated Show resolved Hide resolved
src/store/singing.ts Outdated Show resolved Hide resolved
src/store/type.ts Outdated Show resolved Hide resolved
src/store/type.ts Outdated Show resolved Hide resolved
src/store/project.ts Outdated Show resolved Hide resolved
@y-chan y-chan force-pushed the feat/save-sing-project branch from f2d6289 to c82a4e5 Compare February 13, 2024 00:05
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!!! お疲れ様でした!!

1点、トーク側でソングを含むプロジェクトを読み込んだ後にソングに移動するとノートが表示されてない挙動を見つけました!
簡単に解決できそうなら倒して頂けると・・・!難しそうならissueで良いのかなと思ってます。

@sigprogramming プロジェクトロード部分のとこだけ再度レビューお願いできると心強いです・・・!!

@y-chan
Copy link
Member Author

y-chan commented Feb 14, 2024

トーク側でソングを含むプロジェクトを読み込んだ後にソングに移動するとノートが表示されてない

これは検証したところ、一度もソング画面を開かずにソングプロジェクトを読み込むと、ソング画面を開いた際にonMountedが走ってソング画面が初期化されてしまうのが問題なようです...
修正したいですが、ちょっと規模が大きくなりそうなので別PRで...!

あと、そこを見ていた際にvolumeが保存されないことに気づいたのですが、出力結果に影響を与えるvolumeをちゃんと保存した方がいいなと思いました。
ただ、ステート名がvolumeで、これがトークに対して影響を与えるのか、ソングに対して影響を与えるのかがわかりずらいので、ストアのリファクタを先にした方がいいかもです...

@Hiroshiba
Copy link
Member

Hiroshiba commented Feb 14, 2024

なるほどです! issue作成で良いと思います!!

そこを見ていた際にvolumeが保存されないことに気づいたのですが、出力結果に影響を与えるvolumeをちゃんと保存した方がいいなと思いました。

右上のvolumeですよね。
扱いが難しいですね・・・。たしか保存される音声には影響を与えないんですよね。アプリの音量みたいな。
なのでトークのsplitterの位置みたく、グローバルな設定としてエディタ全体で保存するとかでも良いかも・・・。

Copy link
Contributor

@sigprogramming sigprogramming left a comment

Choose a reason for hiding this comment

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

ほぼLGTMです!

#1842notesKeyShiftvoiceKeyShiftが追加されたので、これらも保存・ロードできると良いかもと思いました!(別PRで行っても良いと思います)

src/store/singing.ts Outdated Show resolved Hide resolved
src/store/singing.ts Show resolved Hide resolved
src/store/project.ts Outdated Show resolved Hide resolved
@y-chan y-chan force-pushed the feat/save-sing-project branch from a3d3147 to c83b1c7 Compare February 16, 2024 13:58
@y-chan
Copy link
Member Author

y-chan commented Feb 16, 2024

コンフリクトの解消と、それに合わせたnotesKeyShiftvoiceKeyShiftの保存・読み込み対応、あと忘れていたapply<XXX>ProjectToStore関数の追加を行いました!
再度レビューいただけると助かります 🙏

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

お疲れ様でした!!良い実装になったように感じます!!!

トーク側でソングを含むプロジェクトを読み込んだ後にソングに移動するとノートが表示されてない

こちらのissue 作成お任せしてしてもよろしいでしょうか 🙇
#1829 (comment) の件)

@Hiroshiba Hiroshiba merged commit 821d298 into VOICEVOX:main Feb 17, 2024
9 checks passed
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.

シングエディタの編集状態を保存できるようにする
4 participants