-
Notifications
You must be signed in to change notification settings - Fork 311
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
ソング:trackChannelStrips、sequences周りを変更 #2205
ソング:trackChannelStrips、sequences周りを変更 #2205
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
出先なので動作確認はできてませんがコード自体は大丈夫だと思います。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
なるほどな設計でした!!!
いくつかコメントはしていますが、この設計でマージするのに賛成です!!
SYNC_TRACKS_AND_TRACK_CHANNEL_STRIPS
を追加した部分や、RENDERを消した部分もう確認して、多分問題ないことを確認しました!
コメントにも書いてあるのですが、ちょっと組み合わせ爆発が起こりつつあるかもと感じました。
関数ごとにRENDER呼ぶか+SYNC呼ぶかを考える形になっているなと。
個人的にはwatchにできるところはそうしておいてあげると、これからコードを追加していきやすいかもと思いました。
(とはいえちょっと自信ないので今回はsync関数用意に賛成です!)
src/store/singing.ts
Outdated
// ChannelStripがある場合は接続する | ||
const channelStrip = trackChannelStrips.get(trackId); | ||
if (channelStrip != undefined) { | ||
getOutputOfAudioSource(sequence).connect(channelStrip.input); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
この処理はsync
関数内にもあるのですが、2箇所にあることが自明ではないので、片方だけ変更してもう片方の変更が忘れることがありえるかもとちょっと思いました!
この辺AudioNodeとかをmock化して、undo/redo含めてVuexのテストを書きたいかもですね・・・。
VuexがDIできないので相当難しそうですが。。。。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!!!
変更ありがとうございました!!!
内容
trackChannelStrips、sequences周りを変更して、
が同期的に行われるようにします。
関連 Issue
close #2197
その他