-
Notifications
You must be signed in to change notification settings - Fork 310
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
ソング:pauが重ならないようにする #2056
ソング:pauが重ならないようにする #2056
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.
ほぼLGTMです!!
だいぶ細かいコメントばかりしちゃいましたが、クリティカルなのはなかったと思います!
合図いただければマージします!
あ、そういえば心なしか品質が上がって聞こえました。 |
伴奏と合わせてみたのですが、フレーズのタイミングがずれてるかも… |
伴奏の方がずれてました…!処理は問題なさそうです |
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!!!
変更ありがとうございます!!!
やっぱりかなり品質が上がって聞こえる気がします!!
前のpauの長さを調整していますが、もしかしたら後ろの長さも調整すると学習データに近くなって綺麗に歌えるようになるのではとかちょっと思いました!
人は後ろにあるpau、つまり休符がどれぐらいの長さかを想像しながら歌うので、同じく機械学習モデルも再現しようとするから、後ろの休符の長さをちゃんと与えた方がうまく歌えるのかもと・・・!!!
phraseFirstRestDuration = Math.max( | ||
phraseFirstRestDuration, | ||
phraseFirstNote.position - | ||
secondToTick( | ||
tickToSecond(phraseFirstNote.position, tempos, tpqn) - | ||
phraseFirstRestMinDurationSeconds, | ||
tempos, | ||
tpqn, | ||
), | ||
); |
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.
ちょっと変なこと言うかもなんですか、ここって直接↓じゃ駄目な感じでしょうか 👀
phraseFirstRestDuration = Math.max(
phraseFirstRestDuration,
secondToTick(phraseFirstRestMinDurationSeconds),
);
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.
なーーーるほどです!!!
連続時間を離散時間(tick・frame)にするとき、連続時間の長さを離散時間の長さにするときはこうする、みたいな概念図があると分かりやすそうですね・・・!!!!
// 1tick以上にする | ||
phraseFirstRestDuration = Math.max(1, phraseFirstRestDuration); |
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.
この上の処理で最小の長さ以上になってるから不要かも?
(phraseFirstRestMinDurationSeconds=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.
phraseFirstRestMinDurationSeconds
の単位は秒で、TPQNとテンポからtickに変換していて1tick以上になることを保証できないので、この処理を行っています!
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.
あーーーーーーなるほどです!!!!!
コメントから推察できた気がしますね。。すみませんありがとうございます!!
const searchPhrases = async ( | ||
notes: Note[], | ||
tempos: Tempo[], | ||
tpqn: number, | ||
phraseFirstRestMinDurationSeconds: number, | ||
) => { |
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.
(別にこのままでも大丈夫なのですが)
searchPhrasesにこの秒数を渡すの不思議かもと思いました!
const quarterNoteDuration = getNoteDuration(4, tpqn);
と同様に0.12
というマジックナンバーも、定数として関数内で宣言するのはどうでしょう。
あるいは定数なので定数が定義されてるファイルに書いてimportするか、あるいはもうRENDER関数の一番最初に定義して使います形もありかも。
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.
確かに、実行中に変更される値ではないので定数にして良さそうです。
関数の名前も、generatePhrases
の方が合ってるかも…?
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.
関数の名前も、generatePhrasesの方が合ってるかも…?
あっ ほんとですね!!
キャッシュを持ってることも示唆するのであればfindOrGenerate
とかもありかも。長いかもですが。。
const firstRestStartSeconds = tickToSecond( | ||
notes[0].position - firstRestDuration, | ||
tempos, | ||
tpqn, | ||
); | ||
const firstRestStartFrame = Math.round( | ||
firstRestStartSeconds * frameRate, | ||
); | ||
const firstRestEndSeconds = tickToSecond( | ||
notes[0].position, | ||
tempos, | ||
tpqn, | ||
); | ||
const firstRestEndFrame = Math.round(firstRestEndSeconds * frameRate); |
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.
もしかしたらticksToFrame関数を作っても良いかも・・・?
(どちらでも良さそう)
const calcPhraseFirstRestDuration = ( | ||
prevPhraseLastNote: Note | undefined, | ||
phraseFirstNote: Note, | ||
phraseFirstRestMinDurationSeconds: number, | ||
tempos: Tempo[], | ||
tpqn: number, | ||
) => { |
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.
パラメータ多くなってきたのでこの辺の値を指定し間違えて発見しづらいバグに繋がったりしやすいかもと感じました!
こんな感じで引数の型指定すると引数名を書く形で関数呼び出しできるので間違いづらくなるかも。
(その代わり引き数の型がややこしくなってしまうのですが。。)
const calcPhraseFirstRestDuration = ( | |
prevPhraseLastNote: Note | undefined, | |
phraseFirstNote: Note, | |
phraseFirstRestMinDurationSeconds: number, | |
tempos: Tempo[], | |
tpqn: number, | |
) => { | |
const calcPhraseFirstRestDuration = ({ | |
prevPhraseLastNote, | |
phraseFirstNote, | |
phraseFirstRestMinDurationSeconds, | |
tempos, | |
tpqn, | |
}: { | |
prevPhraseLastNote: Note | undefined, | |
phraseFirstNote: Note, | |
phraseFirstRestMinDurationSeconds: number, | |
tempos: Tempo[], | |
tpqn: number, | |
}) => { |
あ、いくつかコメントしていますがコンフリクト解消してマージさせていただきたいと思います!! |
@sigprogramming ちょっと突発的な思いつきで相談が・・・! ニーズのある機能が実装されたときにSNSで言及しておりまして、今回のプルリクエストもツイートしたいと思っています。 こんな感じを予定しています・・・!
|
紹介していただいて大丈夫です! |
ありがとうございます!! 今日の昼ぐらいにポストさせていただこうと思います!! |
内容
以下を行います。
1フレーム1tick以上になるようにするcreateNotesForRequestToEngine
内で行っていたので、shiftKeyOfNotes
で行うように変更関連 Issue
close #1943
その他