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

ソング:レンダリング処理をリファクタリング #2429

Merged
merged 10 commits into from
Dec 23, 2024

Conversation

sigprogramming
Copy link
Contributor

@sigprogramming sigprogramming commented Dec 22, 2024

内容

#2261 でレンダリングの流れを変更しますが、変更する前に一度リファクタリングを行っておいた方が良さそうなので、リファクタリングを行います。

以下を行います。(分けてコミットしています)

  • PhraseRenderContextPhraseRendererを一旦無くす
    • レンダリングの流れを変更しやすいように
  • 以下の処理を分ける
    • 新しく作られたフレーズとstateにある既存のフレーズをマージ
    • レンダリング開始ステージの決定
    • phrase.stateの更新
  • 以下の処理を分ける
    • 重なっているノートを削除する処理
    • フレーズの生成
  • searchPhrasesからgeneratePhrasesに変更
  • スナップショットを作成する処理を関数化
  • コメントを整理

関連 Issue

その他

@sigprogramming sigprogramming requested a review from a team as a code owner December 22, 2024 13:08
@sigprogramming sigprogramming requested review from Hiroshiba and removed request for a team December 22, 2024 13:08
@voicevox-preview-pages
Copy link

voicevox-preview-pages bot commented Dec 22, 2024

🚀 プレビュー用ページを作成しました 🚀

更新時点でのコミットハッシュ:5e3183c

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

1箇所コメントしています、判断コメントいただければマージしたいと思います!

コードは一通り眺めましたが、処理が妥当なのかどうか完璧には見れてないです 🙇
実際にちょっと動かしてみて問題点は見つからないことは確認しました!

やっぱりこの辺りテストが書きにくいのが難しいところですね!
ステージのレンダリングが完了したみたいな、イベントが起こるたびにどこかで状態のスナップショットを取って検査するとかはありかもですが、まあ難しそう・・・!

src/store/singing.ts Outdated Show resolved Hide resolved
@Hiroshiba
Copy link
Member

マージします!

@Hiroshiba Hiroshiba merged commit 57f2583 into VOICEVOX:main Dec 23, 2024
10 checks passed
@sigprogramming sigprogramming deleted the refactor_rendering_process_2 branch December 23, 2024 21:49
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.

2 participants