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

マルチトラック:最終調整 #2176

Merged

Conversation

sevenc-nanashi
Copy link
Member

@sevenc-nanashi sevenc-nanashi commented Jul 23, 2024

内容

  • shouldPlayTracksの返り値をSetにします。
    • ついでにテストも足します。
  • マルチトラックを実験的機能にします。
  • overlappingNoteIdsをgetterにします。

関連 Issue

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

image
image

その他

(なし)

@sevenc-nanashi sevenc-nanashi requested a review from a team as a code owner July 23, 2024 21:53
@sevenc-nanashi sevenc-nanashi requested review from Hiroshiba and removed request for a team July 23, 2024 21:53
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/components/Dialog/ImportSongProjectDialog.vue Outdated Show resolved Hide resolved
src/components/Dialog/SettingDialog/ToggleCell.vue Outdated Show resolved Hide resolved
src/components/Dialog/SettingDialog/SettingDialog.vue Outdated Show resolved Hide resolved
src/components/Dialog/SettingDialog/SettingDialog.vue Outdated Show resolved Hide resolved
src/sing/domain.ts Outdated Show resolved Hide resolved
src/store/command.ts Outdated Show resolved Hide resolved
src/store/singing.ts Show resolved Hide resolved
tests/unit/domain/sing/shouldPlayTracks.spec.ts Outdated Show resolved Hide resolved
tests/unit/domain/sing/shouldPlayTracks.spec.ts Outdated Show resolved Hide resolved
@sevenc-nanashi
Copy link
Member Author

反映しました。

@sevenc-nanashi sevenc-nanashi requested a review from Hiroshiba July 24, 2024 10:33
@sigprogramming
Copy link
Contributor

以下を行うとエラーが発生しました、overlappingNoteIdsが更新されてないかも…?

  1. トラック1にノートを追加
  2. トラック2を追加
  3. トラック2のシンガーを変更
  4. トラック2にノートを追加
  5. 連続でUndo(Ctrl+Z押しっぱなし)

@sevenc-nanashi
Copy link
Member Author

どうしましょう、singingStorePluginでoverlappingNoteInfosを更新する...?
それかgetOverlappingNoteIdsが十分速くなったので毎回取るとか

@sigprogramming
Copy link
Contributor

sigprogramming commented Jul 24, 2024

@sevenc-nanashi
調べてみました、renderが走っている最中にoverlappingNoteIdsが更新されてエラーになっているっぽかったです。
なので、// レンダリング中に変更される可能性のあるデータをコピーするのところでoverlappingNoteIdsもコピーして、コピーしたoverlappingNoteIdsを使うようにすればエラーは出なくなると思います。

@sevenc-nanashi
Copy link
Member Author

修正しました。

@sigprogramming
Copy link
Contributor

sigprogramming commented Jul 24, 2024

トラック2を削除して、削除をUndoすると、トラック2が再生されなくなる不具合を見つけました。
AudioNodeの接続が切れてるかも…?

@sevenc-nanashi
Copy link
Member Author

RENDERでtrackChannelStripsをいじるようにしました。既存のwatch用のはコメントだけのこして空にしてます、消しても良さそうではある

@sevenc-nanashi
Copy link
Member Author

多分治ったと思います。

@sigprogramming
Copy link
Contributor

domain.tsgetNumMeasuresnotes引数はソートされているノーツを想定しています(ドキュメントを書いていませんでした…すみません…)が、ソートされていないノーツをSEQUENCER_NUM_MEASURESで渡しているかもです。
ソートしたノーツを渡すようにするか、getNumMeasuresの引数をnotesからtracksに変更するのが良いかなと思います。

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.

すみません、ちょっと細かいところなのですがコメントしました!

src/store/singing.ts Outdated Show resolved Hide resolved
src/store/singing.ts Outdated Show resolved Hide resolved
src/sing/domain.ts Show resolved Hide resolved
Comment on lines 1484 to 1486
// trackChannelStripsを同期する
const shouldPlays = shouldPlayTracks(tracks);
for (const [trackId, track] of tracks) {
Copy link
Contributor

Choose a reason for hiding this comment

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

今の実装だと「trackChannelStripsを同期する」と「フレーズを更新する」がセットで(連続して)実行される必要があり、「trackChannelStripsを同期」の直後にreturnthrowを行うとたぶんバグるので、そのことをコメントで書いておいた方が良いかもです。

Copy link
Member Author

Choose a reason for hiding this comment

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

正直よくわかってないです(trackChannelStripsの同期を忘れたら死ぬならまだしも)

Copy link
Member

Choose a reason for hiding this comment

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

横槍すみません!

グローバル変数として定義されてるtrackChannelStripsは現状RENDER内でしか使ってないので、多分途中でreturnなどが実行されても問題ないはず・・・・・・?
(offlineRender側は現状trackChannelStripsを別のローカル変数を定義して独自に同期を取ってるので)

Copy link
Contributor

@sigprogramming sigprogramming Jul 27, 2024

Choose a reason for hiding this comment

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

以下のケースを想定しています!

  1. トラック2にノートを打ち込んでrender完了を待つ
  2. トラック2を削除
  3. renderが実行され、「trackChannelStripsを同期する」でChannelStripが削除される
  4. 「フレーズを更新する」が完了する前にreturnthrowを行ってrenderを終了
  5. 「トラック2を削除」をUndo
  6. renderが実行され、「trackChannelStripsを同期する」でChannelStripが追加される
  7. 続けて「フレーズを更新する」が実行されるが、フレーズは変わっていないので音声がChannelStripに接続されず、トラック2が再生されなくなる

実際に以下のようなコードで試したところ、トラック2が再生されなくなりました。
image

Copy link
Member

@Hiroshiba Hiroshiba Jul 27, 2024

Choose a reason for hiding this comment

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

なーーーーーーるほどです!!!!!!!
問題意識がとてもよくわかりました、ありがとうございます!!!!

各phraseの音声をchannelStripにくっつける処理が、phraseがキャッシュされている場合に行われないのが原因・・・?
気づきませんでした、申し訳ないです。。

この解決、ちょっととんでもなく難しいですね・・・。全く思いつかない・・・。
ChannelStripは削除せずキャッシュしておく手がありそうですが、ちょっとワーキングアラウンド感がありますね・・・。

いやーーーだいぶ考えたんですがちょっと解決法は思いつかなかったです。。
#2176 (comment) でも @sigprogramming さんがおっしゃってましたが、音声のグラフをいじるのはRENDERと分けるとか、そうじゃなくても結構がっつり手を入れる必要がありそうに感じました。。
コメント書いておくのに賛成です。。

Copy link
Member Author

@sevenc-nanashi sevenc-nanashi Jul 27, 2024

Choose a reason for hiding this comment

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

AudioPlayerとにtrackIdとかを持たせてもう一回接続を張り直すようにする...?なかなか面倒そうなやつですねぇ

Copy link
Member Author

Choose a reason for hiding this comment

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

とりあえずコメントを追加しました・

Co-authored-by: Sig <sigprogrammer@gmail.com>
@sigprogramming
Copy link
Contributor

SELECT_ALL_NOTES_IN_SELECTED_TRACK: {
mutation(state) {
const selectedTrack = getSelectedTrackWithFallback(state);
const allNoteIds = selectedTrack.notes.map((note) => note.id);
state._selectedNoteIds = new Set(allNoteIds);
},
async action({ commit }) {
commit("SELECT_ALL_NOTES_IN_SELECTED_TRACK");
},
},

selectedTrackIdはアプリケーションレイヤーが持つ状態ではなくビュー(プレゼンテーション)レイヤーが持つ状態なので、actionで使用しない方が良いかもです。
trackIdを渡す形にした方が良いかも。

@sigprogramming
Copy link
Contributor

sigprogramming commented Jul 28, 2024

プロジェクトを読み込んだ時に歌声のレンダリングが走らなくなってるかも。
(「最近使ったプロジェクト」と「インポート」から読み込み時に確認しました)

@sevenc-nanashi
Copy link
Member Author

sevenc-nanashi commented Jul 28, 2024

あれ、再現できませんね…再現できたので直しました。

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

トラック1とトラック2にノートを追加してトラック2のピッチを編集した後にUndoするとSequencerPitch.vueでエラーが発生しますが、おそらく #1961 で実装した処理に問題があってそれで発生していると思うので、修正PRを出そうと思います…!

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

現状で気になった点とかはちょっとこちらで変えさせていただきます!

マルチトラックの実装、長期間ありがとうございました!!!
@sevenc-nanashi さんが実装してくださったということで、X でポストさせていただこうと思います!
とりあえず「実験的機能として追加します」みたいな感じで。

ぶっちゃけ過去の経験上、この規模と期間だと多分バグが2~3個ほど残っていると思います!!!
リリースまでにいろいろ試しまくって見つけたいところではあります!
がまあ結構難しいので、報告され次第すぐ直せればいいのかなと思ってます!
なのでリリース後もちょっと慌ただしいかもなのですが、もしよかったらお付き合いいただけると・・・・・・!! 🙏

src/components/Dialog/SettingDialog/SettingDialog.vue Outdated Show resolved Hide resolved

const setMultiTrack = (enableMultiTrack: boolean) => {
changeExperimentalSetting("enableMultiTrack", enableMultiTrack);
// 無効化するときはUndo/Redoをクリアする
Copy link
Member

Choose a reason for hiding this comment

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

こーーーーーーーーれってなんででしたっけ。。。
一応ここのコメントにメモだけ残しておきたく。。

Comment on lines 104 to +119
<SequencerPhraseIndicator
v-for="phraseInfo in phraseInfos"
v-for="phraseInfo in phraseInfosInOtherTracks"
:key="phraseInfo.key"
:phraseKey="phraseInfo.key"
:isInSelectedTrack="phraseInfo.trackId === selectedTrackId"
:isInSelectedTrack="false"
class="sequencer-phrase-indicator"
:style="{
width: `${phraseInfo.width}px`,
transform: `translateX(${phraseInfo.x - scrollX}px)`,
}"
/>
<SequencerPhraseIndicator
v-for="phraseInfo in phraseInfosInSelectedTrack"
:key="phraseInfo.key"
:phraseKey="phraseInfo.key"
isInSelectedTrack
Copy link
Member

Choose a reason for hiding this comment

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

これ2つに分けた理由って何でしたっけ。
phraseInfosInOtherTracksphraseInfosInSelectedTrackをtemplateの中でくっつけた後
:isInSelectedTrack="phraseInfo.trackId === selectedTrackId"で判定させる手もありそう。

なんか色々あった結果そういうこともできるようになったんでしたっけ。
それとも意図あってこうなったんでしたっけ。

Comment on lines +234 to +235
const noteEndPositions = notes.map((note) => note.position + note.duration);
const lastNoteEndPosition = Math.max(...noteEndPositions);
Copy link
Member

Choose a reason for hiding this comment

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

(別にこれでもいいんですが)
たぶんreduceでmax書けるはず。

src/store/singing.ts Outdated Show resolved Hide resolved
src/store/singing.ts Outdated Show resolved Hide resolved
src/store/singing.ts Outdated Show resolved Hide resolved
commit("COMMAND_SET_TRACK_MUTE", { trackId, mute });
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.

この設計方針、もしまとまりそうだったらissueに文章化しても良いかも?

@Hiroshiba Hiroshiba merged commit 2c87e5e into VOICEVOX:project-multitrack Jul 29, 2024
8 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.

3 participants