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

singing.tsのsearchPhrasesをリファクタリング #1850

Merged

Conversation

weweweok
Copy link
Contributor

@weweweok weweweok commented Feb 17, 2024

内容

題の通りです。

singing.tsのRENDERが結構複雑みたいなので、小出しでリファクタリングできればと思っています。

関連issue

@weweweok weweweok requested a review from a team as a code owner February 17, 2024 14:42
@weweweok weweweok requested review from Hiroshiba and removed request for a team February 17, 2024 14:42
@weweweok
Copy link
Contributor Author

weweweok commented Feb 18, 2024

動作確認ですが、何を試せば良いか検討ついていません。。。
処理追ってみたところ、ノートの追加、再生、削除、再生で正しく動作しているか試しかめれば良さそうでした。

@weweweok weweweok changed the title singing.tsのsearchphrasesをリファクタリング singing.tsのsearchPhrasesをリファクタリング Feb 18, 2024
@weweweok
Copy link
Contributor Author

動作確認してみました。

oss.mp4

@@ -855,9 +855,11 @@ export const singingStore = createPartialStore<SingingStoreTypes>({
phraseNotes.push(note);

const currentNoteEnd = note.position + note.duration;
const nextNoteStart = notes[noteIndex + 1].position;
const nextNoteStart =
noteIndex + 1 < notes.length ? notes[noteIndex + 1].position : null;
if (
noteIndex === notes.length - 1 ||
Copy link
Contributor Author

@weweweok weweweok Feb 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if( noteIndex === notes.length - 1 ||
    note.position + note.duration !== notes[noteIndex + 1].position)

↑このように書くと、notes[noteIndex + 1]が存在しない場合も、条件が通るので、nextNoteStart == nullと明示する形にしました。

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

src/store/singing.ts Show resolved Hide resolved
@weweweok
Copy link
Contributor Author

レビューを参考に具体例をちょっと書いてみました。

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

ちょっと細かい部分こちらで変更させていただきます!

src/store/singing.ts Outdated Show resolved Hide resolved
) {
const phraseFirstNote = phraseNotes[0];
const phraseLastNote = phraseNotes[phraseNotes.length - 1];
const hash = await generatePhraseHash({
const params = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

「generate params」だとパラメータを作るという意味になってしまうので、単純にparamsにしてみました!

@Hiroshiba
Copy link
Member

マージします! またよければリファクタリングに挑戦してみてください!!
(何度も何度もやっていると結構わかるようになってきて楽しいと思いますので・・・!!)

@Hiroshiba Hiroshiba merged commit 1790938 into VOICEVOX:main Mar 18, 2024
9 checks passed
@weweweok weweweok deleted the serchPhrasesのリファクタリング branch March 19, 2024 13: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.

None yet

2 participants