-
Notifications
You must be signed in to change notification settings - Fork 346
[project-s] 鍵盤クリック時とノート編集時に音を鳴らす機能を追加 #1671
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
Conversation
| noteOn(contextTime: number) { | ||
| const t0 = contextTime; | ||
| const minContextTime = this.getMinContextTime(this.audioContext); | ||
| const t0 = Math.max(minContextTime, contextTime); |
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.
スケジュールが間に合わなかった場合(t0 < audioContext.currentTimeの場合)に音が鳴らなくなるので、スケジュールが間に合うようにt0を設定する処理を追加しました。
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.
ちょっと違和感あったので、minContextTimeとcontextTimeをconsole.logしてみました。
contextTimeは0が渡されている一方、minContextTimeはcurrentTime+微小値が計算されていました!
どちらか意図と違いそう・・・?
あるいは「0やマイナス値を指定した場合は即座に音がなる」という意図した設計になっている感じでしょうか 👀
であればより意図がわかりやすいように、専用のリテラル文字を用意するとわかりやすいかもです!
こんな感じとか・・・?
noteOn(contextTime: number | "immediately") {
if (contextTime === "immediately") {
contextTime = this.getMinContextTime(this.audioContext);
}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.
「0やマイナス値を指定した場合は即座に音がなる」という意図した設計になっている感じでしょうか
OscillatorNode.start()は「currentTimeより前の時刻が指定されると即座に音がなる」ようになっていて、それに合わせています!
おっしゃる通り、0ではなく"immediately"を指定する形にすると分かりやすくなると思うので、
後で修正したいと思います!
Hiroshiba
left a comment
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!!!
ドキュメント追加とシグネチャ変更の提案コメントをしてみました!
けど別に致命的ではないので、スピード優先でマージさせていただきます!!
| }; | ||
| // キーボードイベント | ||
| const handleNotesArrowUp = () => { |
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.
handleNotesArrowUpとhandleNotesArrowDown、処理共通化できそうに感じました!!
| noteOn(contextTime: number) { | ||
| const t0 = contextTime; | ||
| const minContextTime = this.getMinContextTime(this.audioContext); | ||
| const t0 = Math.max(minContextTime, contextTime); |
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.
ちょっと違和感あったので、minContextTimeとcontextTimeをconsole.logしてみました。
contextTimeは0が渡されている一方、minContextTimeはcurrentTime+微小値が計算されていました!
どちらか意図と違いそう・・・?
あるいは「0やマイナス値を指定した場合は即座に音がなる」という意図した設計になっている感じでしょうか 👀
であればより意図がわかりやすいように、専用のリテラル文字を用意するとわかりやすいかもです!
こんな感じとか・・・?
noteOn(contextTime: number | "immediately") {
if (contextTime === "immediately") {
contextTime = this.getMinContextTime(this.audioContext);
}| private getMinContextTime(audioContext: BaseAudioContext) { | ||
| if (audioContext instanceof AudioContext) { | ||
| const renderQuantumSize = 128; | ||
| const latency = (renderQuantumSize + 10) / audioContext.sampleRate; | ||
| return audioContext.currentTime + latency; | ||
| } else { | ||
| return 0; | ||
| } | ||
| } |
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.
getMinContextTimeという関数名から「即座に音がなる一番近い時間」だということが伝わらないかもなので、ドキュメントがあると良いのかなと思いました!
あ、あとAudioContextでなくBaseAudioContextにもsampleRateやcurrentTimeは生えているっぽいので、instanceofのifなくても良いかも?
内容
鍵盤クリック時とノート編集時に音を鳴らす機能を追加します。
また、以下も行います。
symbolを使わない形に変更関連 Issue
VOICEVOX/voicevox_project#15
その他