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] ピアノロールにノートを表示/追加/削除できるようにする #1102

Merged
merged 22 commits into from
Feb 5, 2023

Conversation

romot-co
Copy link
Contributor

@romot-co romot-co commented Jan 13, 2023

内容

  • ピアノロール表示
  • ノートおよび歌詞の表示
  • ノートの追加(ノートのクリック)
  • ノートの削除(ノートのダブルクリック)
  • X/Yズーム

関連 Issue

ref #986
close #986

スクリーンショット・動画など

2023-01-17.18.06.03.mov

その他

  • 見た目は仮となります
  • シーケンサのノートおよび入力部は分割予定です

@romot-co romot-co changed the title ピアノロールにノートを表示/追加/削除できるようにする [project-s] ピアノロールにノートを表示/追加/削除できるようにする Jan 13, 2023
Copy link
Contributor

@sigprogramming sigprogramming left a comment

Choose a reason for hiding this comment

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

ピアノロールの作成、お疲れ様です!!
ノートの追加もできるようになっていて凄いです…!

リファクタ中かもですが、いくつかコメントをしましたので、ご確認よろしくお願いいたします!

src/helpers/singHelper.ts Outdated Show resolved Hide resolved
src/helpers/singHelper.ts Outdated Show resolved Hide resolved
src/store/singing.ts Outdated Show resolved Hide resolved
src/store/singing.ts Outdated Show resolved Hide resolved
@romot-co romot-co force-pushed the feature/986_display_pianoroll_from_score branch from 9037df4 to 6b8f301 Compare January 16, 2023 18:12
@romot-co
Copy link
Contributor Author

@Hiroshiba @sigprogramming @y-chan
おつかれさまです、いったん実装しましたのでおてすきでレビューお願いいたします…!
また、諸々作業できずお時間いただき失礼いたしました、
このあとノートの長さ+位置変更や歌詞入力の方行えればと思います。

sigさまには先にレビューいただきありがとうございます!
その後、ご指摘点修正とあわせ、パフォーマンスもっさり問題があったためSVGでグリッド表示に修正しております。

@romot-co romot-co force-pushed the feature/986_display_pianoroll_from_score branch from a624ec2 to 8dcc536 Compare January 18, 2023 11:06
@romot-co romot-co force-pushed the feature/986_display_pianoroll_from_score branch from a3bf133 to df58348 Compare January 18, 2023 14:03
Romot and others added 3 commits January 19, 2023 02:05
Copy link
Contributor

@sigprogramming sigprogramming 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
Copy link
Contributor Author

@sigprogramming
ありがとうございます!後から諸々修正して大変失礼しました!

@sigprogramming
Copy link
Contributor

@Hiroshiba さん、 @y-chan さん、おてすきでレビューお願いいたします…!

@Hiroshiba
Copy link
Member

@sigprogramming すみません大変失礼しました!! 🙇‍♂️
近々レビューさせて頂きたいと思います 🙇‍♂️

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

形になってきてワクワクしますね!!!
SVGってpatternというのがあるんですね、知りませんでした。

細かい部分がちょっと追えてないかもですが、今はガシガシ実装のフェーズかなと思うのでスピーディにマージしていくのが良いのかなと思いました!
いくつかコメントしているのでマージはおまかせしたいと思います 🙏

src/components/Sing/ScoreSequencer.vue Show resolved Hide resolved
const gridY = midiKeys;
const gridX = computed(() => {
const resolution = store.state.score?.resolution || 480;
// NOTE: 最低長: 仮32小節...MIDI長さ(曲長さ)が決まっていないため、無限スクロール化する or 最後尾に足した場合は伸びるようにするなど?
Copy link
Member

Choose a reason for hiding this comment

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

無限スクロール化はなかなか腕の見せ所な気がしますね!
理論上は画面上に映ってるものだけを制御できれば良いはずですが、パッと良い実装は思いつきませんでした。。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Hiroshiba
こちら再生でスクロール移動などもあるので、virtural scroll化がいいかななどと考えておりますが、
今後考慮できれば…!

@romot-co
Copy link
Contributor Author

romot-co commented Feb 5, 2023

@Hiroshiba @sigprogramming
レビューありがとうございます!
諸々ありますが、次のプルリクの元にもなるのでいったんマージいたします!

@romot-co romot-co merged commit 495b9d4 into project-s Feb 5, 2023
@sevenc-nanashi sevenc-nanashi deleted the feature/986_display_pianoroll_from_score branch July 1, 2024 12:53
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