-
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
ソング:フレーズのレンダリング処理をリファクタリング #2248
ソング:フレーズのレンダリング処理をリファクタリング #2248
Conversation
/** | ||
* リクエスト用のノーツ(と休符)を作成する。 | ||
*/ | ||
const createNotesForRequestToEngine = ( |
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.
以下の関数と変数はsinging.ts
のRENDER
から持ってきたもので、ほとんど変更していません。
createNotesForRequestToEngine
shiftKeyOfNotes
getPhonemes
shiftPitch
shiftVolume
muteLastPauSection
singingTeacherStyleId
lastRestDurationSeconds
fadeOutDurationSeconds
プルリクエストありがとうございます! もしよかったら @sevenc-nanashi さんもレビューいただけると・・・・!! |
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.
すみません、+-2000行が圧巻で、かつアーキテクチャ結構変わっているのでレビューの手がつけづらく 🙇
ということでいくつかお聞きできると・・・!
あ、質問に番号振ってますが、特に順番とかは意識しておらず、知りたい観点を分けてみた次第です!
- どこから追っていくのがおすすめでしょうか 👀
RENDER()
action? src/sing/phraseRendering.ts
?
上から見ていく? render()
から? Stageから?
- 既存のRENDER関数で行っていた処理の流れは変わったか
(ほぼ)変わってないという認識だけど念の為
- phraseRenderingの責務範囲
PRのdescriptionに書いてくださってるレンダリングの流れ
の中のどこがどっちなのかわからない感じです!
RENDER関数側にも結構処理が残ってるので、「フレーズレンダリングの処理全部を移した」ということではなさそう?
キャッシュ用のキー作成部分・キャッシュする部分・エンジンリクエスト等はどちらかなど。
そもそもそういった一連の処理を行うのか、データの依存(順番)だけ制御するのかとか。
- Stageとは
phraseごとに持つ? メインプロセスが1つずつ持ってる?
ここがデータの依存関係をうまいこと解決する?
RENDER関数側からはあんまり気にしなくていい?
いろいろ聞いてしまってすみません!設計思想だけ教えていただけると・・・!
(どういう処理を追加・消えたのかや最終的に流れがどうなったかに加え、どういうやり方で整理をつけたのか知りたい感じです)
@Hiroshiba
これらのステージがこの順序で保持されていることを前提に、フレーズレンダラーが各ステージの処理を呼び出す感じになっています。 以前は
例えば、ピッチの編集を行ったときは以下を行います。
以下の2つは
ひとまず フレーズレンダラーと各ステージは、
以前は音声の再合成が必要な場合、フレーズ更新のタイミングで音声の削除を行っていましたが、このPRではフレーズレンダリングの直前で行うように変更しています。 |
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です!!
解説ありがとうございます!!!よくわかりました!!
かなり初見で迷いやすいコードになっているので、ドキュメント足しを提案してみました。
あとはコードのちょっとした提案が主です!
そろそろdocs辺りにソング周りの全体設計を書いた方が良さそうという思いが強くなってきました。
処理の細かい部分は実装を見てもらうことにして、なぜこういう実装になっているのかを数行ずつで紹介していく、みたいなのを想像しました!
(ドキュメントではなく、コードのファイルの一番上に書いてもいいかも。)
Co-authored-by: Hiroshiba <hihokaruta@gmail.com>
Co-authored-by: Hiroshiba <hihokaruta@gmail.com>
@Hiroshiba |
@sigprogramming ドキュメントに関して了解です!! 議論を重ねてより良いドキュメントを作ればというのもなるほどです! 再度レビューしたいと思います! |
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!!!
リファクタリングありがとうございます!!
内容
以下を行います。
PhraseRenderer
を追加QueryGenerationStage
を追加SingingVolumeGenerationStage
を追加SingingVoiceSynthesisStage
を追加phraseRendering.ts
に移動createNotesForRequestToEngine
shiftKeyOfNotes
getPhonemes
shiftPitch
shiftVolume
muteLastPauSection
PhraseRenderer
を使用して行うように変更startTime
)をPhrase
に持たせ、算出をフレーズ生成時に行うように変更PhraseState
にSINGER_IS_NOT_SET
を追加レンダリングの流れ
searchPhrases
を実行state
をSINGER_IS_NOT_SET
に設定state
がCOUND_NOT_RENDER
の場合、レンダリング開始ステージを一番最初のステージに設定state
をWAITING_TO_BE_RENDERED
に設定state
がSINGER_IS_NOT_SET
またはWAITING_TO_BE_RENDERED
のフレーズに対して、以下を行うstate
がWAITING_TO_BE_RENDERED
のフレーズに対して以下を行う関連 Issue
ref #2247
その他