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

[project-s] シーケンサーの計算の整理と修正 #1602

Merged

Conversation

sigprogramming
Copy link
Contributor

@sigprogramming sigprogramming commented Oct 7, 2023

内容

以下を行います。

  • 計算の整理・修正
    • マジックナンバーを無くし、変換処理などを関数化する
  • 変数やクラスの名前をわかりやすいものに変更
    • なるべく単位をつけるように
    • midiからnoteNumberに変更
    • resolutionからtpqnに変更
  • 拍子やスナップが変わっても、グリッドが適切に描画されるようにする

値の変換

以下のように行っています。

flowchart LR;
    Ticks <-->|TPQN| QuarterNotes
    QuarterNotes <-->|BaseXPerQuarterNote| BaseX
    BaseX <-->|ZoomX| ViewX
    NoteNumber <-->|BaseYPerNoteNumber| BaseY
    BaseY <-->|ZoomY| ViewY
    style Hidden1 fill-opacity:0, stroke-opacity:0;
    Hidden1[" "] ~~~ NoteNumber
    style Hidden2 fill-opacity:0, stroke-opacity:0;
    ViewX ~~~~ Hidden2[" "]
Loading

TPQN=4分音符あたりのティック数(Ticks Per Quarter Note)です。

QuarterNote=4分音符です。4分音符の長さは拍子(小節の長さ)に依存しません。

BaseXBaseYはズーム適用前の値、ViewXViewYはズーム適用後の値です。
(ズーム適用前の値を保持する変数は、Baseをつけています)

関連 Issue

VOICEVOX/voicevox_project#15

その他

  • singing.tsの方も後で修正したいと思います

:x="`${(note.duration / 4) * zoomX - 4}`"
width="16"
:x="`${barWidth - 4}`"
width="12"
Copy link
Contributor Author

@sigprogramming sigprogramming Oct 7, 2023

Choose a reason for hiding this comment

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

左のドラッグハンドルが12、右のドラッグハンドルが16になっていたので、とりあえず両方とも12に合わせました。

Comment on lines 191 to 192
const gridColumnTicks = snapTicks;
const gridColumnBaseWidth = snapBaseWidth;
Copy link
Contributor Author

@sigprogramming sigprogramming Oct 7, 2023

Choose a reason for hiding this comment

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

ひとまずスナップ幅=グリッド幅としました。

Copy link
Member

Choose a reason for hiding this comment

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

ひとまずこうしていることを書いといてあげると誰かの役に立つかも?

Suggested change
const gridColumnTicks = snapTicks;
const gridColumnBaseWidth = snapBaseWidth;
const gridColumnTicks = snapTicks; // ひとまずスナップ幅=グリッド幅
const gridColumnBaseWidth = snapBaseWidth;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

コメントを追加しました!

@sigprogramming sigprogramming marked this pull request as ready for review October 7, 2023 16:22
@sigprogramming sigprogramming requested a review from a team as a code owner October 7, 2023 16:22
@sigprogramming sigprogramming requested review from y-chan, Hiroshiba, romot-co and a team and removed request for a team and y-chan October 7, 2023 16:22
@sigprogramming
Copy link
Contributor Author

@romot-co 結構変更してしまっているので、ご確認お願いいたします…!

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!!
マージは一旦 @romot-co さんをちょっとお待ちしたいと思っています!

gridとsnapは==な気がするのですが、どちらかがどちらかに依存すべきなのか、独立すべきなのかわからないの面白いですね。
様子見な今のコードが良さそう!

sequencerSnapSize: 120, // スナップサイズ 試行用で1/18(ppq=480)のmidi durationで固定
sequencerScrollY: 60, // Y軸 note number
sequencerScrollX: 0, // X軸 tick(仮)
sequencerSnapType: 16, // スナップタイプ 試行用で1/16で固定
Copy link
Member

Choose a reason for hiding this comment

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

(本質的じゃないコメントですが)
将来的には文字リテラルでsemiquaverとかにしたいですね!

Comment on lines 191 to 192
const gridColumnTicks = snapTicks;
const gridColumnBaseWidth = snapBaseWidth;
Copy link
Member

Choose a reason for hiding this comment

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

ひとまずこうしていることを書いといてあげると誰かの役に立つかも?

Suggested change
const gridColumnTicks = snapTicks;
const gridColumnBaseWidth = snapBaseWidth;
const gridColumnTicks = snapTicks; // ひとまずスナップ幅=グリッド幅
const gridColumnBaseWidth = snapBaseWidth;

src/components/Sing/ScoreSequencer.vue Outdated Show resolved Hide resolved
return tpqn * quarterNotesPerMeasure;
}

export function getMeasureNum(notes: Note[], measureDuration: number) {
Copy link
Member

Choose a reason for hiding this comment

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

measure、何を測ってるんだろうと思ってたんですが、小節なんですね!!学び。

src/helpers/singHelper.ts Show resolved Hide resolved
src/helpers/singHelper.ts Show resolved Hide resolved
@Hiroshiba
Copy link
Member

@romot-co さんお忙しそうなので、いったんマージさせていただくのが良いのかなと感じました!
たぶんどうしてもコンフリクトは大量発生すると思うので、お互い協力して解消していければ良いのかなと・・・!
(時間作ってペアプロとか。)

@Hiroshiba
Copy link
Member

Hiroshiba commented Oct 11, 2023

すみません、マージさせていただきます!
どちらにせよコンフリクトは大量発生すると思うので、もし難しければVSCodeのLiveShare辺り使ったりしてみんなで時間作ってペアプロとかできれば楽しいかな、とかちょっと思ってます。

@Hiroshiba Hiroshiba merged commit e1388cf into VOICEVOX:project-s Oct 11, 2023
@romot-co
Copy link
Contributor

@sigprogramming @Hiroshiba
遅くなり失礼いたします、
こちら内容の方、確認いたしました!
諸々残念な点も修正いただき、わかりやすく&拡張しやすくなっており、ありがとうございます!

コンフリクトにつきまして、
別Issue(モード整理からの操作系の整理)で行っているものは、無理にコンフリクト解消せず
こちらをマージ済みのものに対し、修正しつつ移植の形で実装できればと思います…!

明日(16日)から順次行なっていこうと思います。

それとは別にペアプロもできたらたしかに楽しそう…?

@sigprogramming sigprogramming deleted the refactor_and_fix_sequencer branch October 27, 2023 11:33
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.

3 participants