feat: [project-sequencer-statemachine] ステートマシンに置き換える #2544

sigprogramming · 2025-02-18T10:26:46Z

内容

ScoreSequencer.vueのプレビュー処理のところをステートマシンに置き換えます。
コメントの整理も行います。

その他

sigprogramming · 2025-02-18T11:13:38Z

src/components/Sing/ScoreSequencer.vue

          <div
-            ref="rectSelectHitbox"
+            v-if="previewRectForRectSelect != undefined"
            class="rect-select-preview"
            :style="{
-              display: isRectSelecting ? 'block' : 'none',


表示/非表示の切り替えをdisplayからv-ifに変更しました。

Hiroshiba

ほぼLGTMです！！

これをマージしたら完成でしょうか！！
であればマージ後、issue側で色んな人による動作確認を募集する流れが良さそうだなーと！！

Hiroshiba · 2025-02-18T13:03:09Z

src/components/Sing/ScoreSequencer.vue

+const idleStateId = computed((): IdleStateId => {
+  if (state.sequencerEditTarget === "NOTE") {
+    if (state.sequencerNoteTool === "SELECT_FIRST") {
+      return "selectNotesToolIdle";
+    } else if (state.sequencerNoteTool === "EDIT_FIRST") {
+      return "editNotesToolIdle";
+    } else {
+      throw new ExhaustiveError(state.sequencerNoteTool);
+    }
+  } else if (state.sequencerEditTarget === "PITCH") {
+    if (state.sequencerPitchTool === "DRAW") {
+      return "drawPitchToolIdle";
+    } else if (state.sequencerPitchTool === "ERASE") {
+      return "erasePitchToolIdle";
+    } else {
+      throw new ExhaustiveError(state.sequencerPitchTool);
+    }
+  } else {
+    throw new ExhaustiveError(state.sequencerEditTarget);
+  }
+});


ここ結構危ない気がしますね！

まず目的が２つになってそうです。
useSequencerStateMachineに渡してステートマシンの初期状態を決める計算に使われてるのと、watchしてステートマシンを置き換えてるのと。

「フラグ３種からIdleStateIdを計算する」関数を切り出して２箇所から関数を叩く形がより良いかもです！

「フラグ３種からIdleStateIdを計算する」関数を切り出して２箇所から関数を叩く形がより良いかもです！

以下のような形でしょうか？
（関数にするとwatchする変数を別で指定しないといけなくなるので、computed（今の形）の方が良い気がしています）

const computeIdleStateId = (): IdleStateId => { if (state.sequencerEditTarget === "NOTE") { if (state.sequencerNoteTool === "SELECT_FIRST") { return "selectNotesToolIdle"; } else if (state.sequencerNoteTool === "EDIT_FIRST") { return "editNotesToolIdle"; } else { throw new ExhaustiveError(state.sequencerNoteTool); } } else if (state.sequencerEditTarget === "PITCH") { if (state.sequencerPitchTool === "DRAW") { return "drawPitchToolIdle"; } else if (state.sequencerPitchTool === "ERASE") { return "erasePitchToolIdle"; } else { throw new ExhaustiveError(state.sequencerPitchTool); } } else { throw new ExhaustiveError(state.sequencerEditTarget); } }; const { stateMachine, previewMode, previewNotes, previewRectForRectSelect, previewPitchEdit, cursorState, guideLineTicks, } = useSequencerStateMachine(store, computeIdleStateId()); watch(() => [ state.sequencerEditTarget, state.sequencerNoteTool, state.sequencerPitchTool, ], () => { const IdleStateId = computeIdleStateId(); if (stateMachine.currentStateId !== IdleStateId) { stateMachine.transitionTo(IdleStateId, undefined); } });

watchのとこはこういう感じを想像してました！

const idleStateId = computed(() => computeIdleStateId()); watch(idleStateId, (value) => { if (stateMachine.currentStateId !== value) { stateMachine.transitionTo(value, undefined); } });

けどまあ、ほとんど変わらない気もします。
（watchが不要になるのが一番良さそう感？）

なるほどです、ひとまずこのPRではcomputed（現在の形）で実装しようと思います…！

とりあえず了解です。

ちなみに一つのものに複数の意味を持たせるのは、SOLID原則のS（単一責任の原則）に違反してたりします。
ちょくちょく本読んでるのでまたわかったらお伝えします！

Hiroshiba · 2025-02-18T13:06:45Z

src/components/Sing/ScoreSequencer.vue

+watch(idleStateId, (value) => {
+  if (stateMachine.currentStateId !== value) {
+    stateMachine.transitionTo(value, undefined);
+  }
 });


ここ、やっぱりステートマシンの外からステートマシンの操作をしてるので、なかなか危なそうだなと感じます！
変な状態遷移が発生し得るので。例えば範囲選択中にツールが切り替えられちゃうとか。

おそらく「ツールを切り替える」というイベントをStateMachineのprocessに流し、ツールの状態変数もステートマシンのContextで管理するのが良いんだろうなーと。

ただ結構大変だと思うのと、より良い設計も思いつけるかもなので、いったんTODOコメントを書き足すというのはどうでしょうか？

例えば範囲選択中にツールが切り替えられちゃうとか。

これは想定しています。（範囲選択中にツール切り替えのショートカットを押したときなど）
操作中にツールの切り替えが発生した場合は操作をキャンセルする、という仕様でひとまず実装しています。

おそらく「ツールを切り替える」というイベントをStateMachineのprocessに流し、ツールの状態変数もステートマシンのContextで管理するのが良いんだろうなーと。

複合状態（ステートの中にステートがある）を扱える設計にすれば可能だと思います。

また、イベントは、なるべくステートによって扱いが変わるもの（マウスイベントとキーボードイベント）のみ扱うようにするのが良いかなと思っています。
ツールの切り替えはステートマシンで扱わず、ツールごとにステートマシンを用意して、ツールが切り替わったらステートマシンも切り替える、という設計も良いかもと思っています。

このPRではひとまずTODOコメントの追加のみ行おうと思います。

操作中にツールの切り替えが発生した場合は操作をキャンセルする、という仕様でひとまず実装しています。

なるほどです！
この辺りどこかのドキュメントに書いてまとめて行った方がいいかもですね。
StateMachineディレクトリとかにREADME置くとか、ファイルの先頭にメモを書くとか。

あと操作がキャンセルできないものも将来的にはありえるかも･･･？
適当ですが、自動生成中とか。
でもなんかキャンセルするしかないかもですね！

複合状態（ステートの中にステートがある）を扱える設計にすれば可能だと思います。

「ToolButtonが押された」というイベントを流すだけなので、複合状態が扱えなくても可能かも･･･？

このPRではひとまずTODOコメントの追加のみ行おうと思います。

了解です！

src/components/Sing/ScoreSequencer.vue

voicevox-preview-pages · 2025-02-19T12:29:14Z

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

更新時点でのコミットハッシュ：6aa863f

Co-authored-by: Hiroshiba <[email protected]>

sigprogramming · 2025-02-19T13:19:17Z

src/sing/stateMachine.ts

    this.context = context;

    this.currentState = stateFactories[initialStateId](undefined);
+    this.currentState.onEnter(context);


onEnterを呼んでいなかったので修正しました。

src/composables/useSequencerStateMachine.ts

Hiroshiba

LGTM！！

ステートマシンは完成でしょうか！！
であればみんなで試してみるフェーズかなと！！

置き換えとコメントの整理

c69723e

sigprogramming changed the title ~~feat: [project-sequencer-statemachine] 置き換え~~ feat: [project-sequencer-statemachine] プレビュー処理のところをステートマシンに置き換える Feb 18, 2025

sigprogramming marked this pull request as ready for review February 18, 2025 10:49

sigprogramming requested a review from a team as a code owner February 18, 2025 10:49

sigprogramming requested review from Hiroshiba and removed request for a team February 18, 2025 10:49

sigprogramming changed the title ~~feat: [project-sequencer-statemachine] プレビュー処理のところをステートマシンに置き換える~~ feat: [project-sequencer-statemachine] ステートマシンに置き換える Feb 18, 2025

sigprogramming mentioned this pull request Feb 2, 2025

ソング：シーケンサーのプレビュー処理をステートパターンで実装して置き換える #2403

Open

24 tasks

sigprogramming commented Feb 18, 2025

View reviewed changes

Hiroshiba reviewed Feb 18, 2025

View reviewed changes

src/components/Sing/ScoreSequencer.vue Outdated Show resolved Hide resolved

sigprogramming added 2 commits February 19, 2025 19:57

初期ステートのonEnterを実行していなかったので修正

a610a37

idleStateIdとwatchを移動

4ebb74f

sigprogramming and others added 3 commits February 19, 2025 22:06

processWithStateMachineを返すようにした

6123d97

TODOコメントを追加

d74ac90

Update src/components/Sing/ScoreSequencer.vue

9d5b31b

Co-authored-by: Hiroshiba <[email protected]>

sigprogramming commented Feb 19, 2025

View reviewed changes

Hiroshiba reviewed Feb 19, 2025

View reviewed changes

src/composables/useSequencerStateMachine.ts Outdated Show resolved Hide resolved

chore: stateMachineProcessに

6aa863f

Hiroshiba merged commit bf4cf38 into VOICEVOX:project-sequencer-statemachine Feb 19, 2025
10 checks passed

Hiroshiba reviewed Feb 19, 2025

View reviewed changes

sigprogramming deleted the replace branch February 19, 2025 15:51

feat: [project-sequencer-statemachine] ステートマシンに置き換える #2544

feat: [project-sequencer-statemachine] ステートマシンに置き換える #2544

Uh oh!

Conversation

sigprogramming commented Feb 18, 2025

内容

関連 Issue

その他

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hiroshiba left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sigprogramming Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

voicevox-preview-pages bot commented Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Hiroshiba left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sigprogramming Feb 19, 2025 •

edited

Loading

voicevox-preview-pages bot commented Feb 19, 2025 •

edited

Loading