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

ソング:再生位置の表示形式を変更できるようにする #2306

Merged

Conversation

sigprogramming
Copy link
Contributor

@sigprogramming sigprogramming commented Oct 19, 2024

内容

以下を行います。

  • 再生位置を右クリックで表示形式を変更できるようにする
    • 分:秒.ミリ秒形式(デフォルト)
    • 小節.拍.100分形式
  • 表示形式を設定として保存するようにする
  • 再生位置の表示をコンポーネント化
  • MeasuresBeats型を追加
    • Measures, Beats(小節、拍)
  • playheadPosition周りを変更
  • isValidTpqnの処理を修正

関連 Issue

close #2305

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

2024-10-19.123403.mp4

その他

@sigprogramming sigprogramming requested a review from a team as a code owner October 19, 2024 02:42
@sigprogramming sigprogramming requested review from Hiroshiba and removed request for a team October 19, 2024 02:42
Copy link
Contributor

@romot-co romot-co left a comment

Choose a reason for hiding this comment

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

@sigprogramming
ありがとうございます!確かに小節単位でわかった方がよいことも多く、
わかりやすそうです!


取り急ぎ思ったこと:

  • もしかしたらdecimal部なくてもいいかも…?
  • いまどっち単位なのか表示あったほうがいいかも
  • 単位切り替えを右クリック以外でつけるのもありかも

たとえば以下のような…?

2024-10-20.0.22.33.mp4

その他、実装問題なさそうです!ありがとうございます!!

@sigprogramming
Copy link
Contributor Author

sigprogramming commented Oct 19, 2024

@romot-co
ありがとうございます!

もしかしたらdecimal部なくてもいいかも…?

少数部が無い場合、スナップが16分未満(32分など)のときに再生位置を正確に表せなくなるので、再生位置の表示を見ながらペーストを行うと、意図した場所にペーストされなかったりが起こるかも…と思いました!

いまどっち単位なのか表示あったほうがいいかも

確かに単位があった方が分かりやすいと思いますが、単位を横に表示する形だと、「小節」の場合は一番左の数字が小節を表しているということが伝わらないので、拍のところを小節と勘違いしたりが起こるかも…と思いました!
(表示を工夫すればどうにかできるかもですが、思いつかない…)
(右クリックメニューの表示も小節.拍.16分にした方が良いかも…?)

単位切り替えを右クリック以外でつけるのもありかも

上記の単位表示の問題が解決できれば、動画のようにボタンで切り替えられるようにするのもありだと思います!

@sevenc-nanashi
Copy link
Member

もしかしたらdecimal部なくてもいいかも…?

参考までに:

reaper.mp4

Reaperの場合は拍を100等分したものをdecimal部にして小節.拍.100等分のような形式でした。これでもいいかも?

@Hiroshiba
Copy link
Member

Hiroshiba commented Oct 21, 2024

機能追加良さそう!!

@romot-co さんのボタン案は、よくあるトグルボタン問題で、ラベルが「今表示されているもの」を表すのか、「押されたらそれになるもの」を表してるのかわからん問題も発生しそうに思いました!
まあ結構慣れやすいのですぐわかる気もしますが。あと間違えて押しちゃいそう!

まとめるとこかなと思ってます:

  • 右クリック切り替え
    • ちょっと気づきづらいはず。けどぶっちゃけ必要な人はギリギリたどり着けそうなのでこれでも良さそう。
  • ボタンにしろ右クリックメニューにしろ、確かに表示されているものが小節.拍.16分とは気づかないかも
    • ボタンの場合はホバーでツールチップを表示すれば解決しそう。
    • メニューアイテムの場合はラベルを「小節.拍.16分」にするか、あるいは長い説明を書くか、こちらもツールチップでも良さそう
  • 「小節」「秒」ボタン
    • 今の状態なのか、クリックした時の状態なのかわからない
    • 「ノート・ピッチ」みたくグループボタンにすれば解決はできるけど、面積取りそう
    • この切り替えが重要なら、ボタンよりも気づきやすいのでこっちの方が良さそう。そうでもないなら右クリックでも良さそう!

個人的には誰もが必ず欲しい機能ではないと思うので、右クリックが良さそうなのかなと思いました!
何が表示されているかわからない問題は、時計をホバーしたときにツールチップで案内でも良さそう。
メニューアイテムの名前を小節.拍.16分とかにするのは良さそう、たぶん小節・拍とかでも、この言葉がわかる人には時計が読める気がしました!

(あ、これはボタンか右クリックどちらかだと右クリック良さそうという気持ちです!)
(キャラクター切り替えみたく、左クリックでもいいのかもしれない)(ジャストアイディア)

@sigprogramming
Copy link
Contributor Author

@sevenc-nanashi
小節.拍.100分にしてみました。この形でも良さそう!
小節.拍.16分.100分の16分は初心者には分かりづらいかも)

小節拍100分の形式で表示

MBS型からMeasuresBeats型への変更も行いました。

@sigprogramming
Copy link
Contributor Author

@Hiroshiba
ありがとうございます!

右クリック切り替え
ちょっと気づきづらいはず。けどぶっちゃけ必要な人はギリギリたどり着けそうなのでこれでも良さそう

少し気づきづらいですが、切り替えたくなることもそんなにないと思うのと、切り替えたくなった人は再生位置のところを色々弄っているうちに気づくかもと思ったので、ひとまず右クリックで実装できればと思います!

ボタンにしろ右クリックメニューにしろ、確かに表示されているものが小節.拍.16分とは気づかないかも
メニューアイテムの場合はラベルを「小節.拍.16分」にするか、あるいは長い説明を書くか、こちらもツールチップでも良さそう

ひとまず右クリックメニューの表記を小節.拍分:秒にしました!

何が表示されているかわからない問題は、時計をホバーしたときにツールチップで案内でも良さそう

ツールチップも試してみたのですが、右クリックしたときに右クリックメニューと被って少しいまいちな感じでした…

キャラクター切り替えみたく、左クリックでもいいのかもしれない

左クリックだと、誤ってクリックしたときに戻し方が分からなくなるかもと思いました…!


デフォルト表示は小節.拍でも良いでしょうか?
(ひとまず小節.拍にしていますが、とっつきやすさを優先して分:秒でも良いと思います)

@sigprogramming
Copy link
Contributor Author

表示モードを設定として保存するようにしました。

@sevenc-nanashi
Copy link
Member

キャラクター切り替えみたく、左クリックでもいいのかもしれない

左クリックだと、誤ってクリックしたときに戻し方が分からなくなるかもと思いました…!

hover時にunderlineを引いてClickable感を出しつつ、左クリックでコンテキストメニューを出すとかを思いつきました。ワンクッション挟まるのでそんなに事故らない…はず?

@Hiroshiba
Copy link
Member

Hiroshiba commented Oct 22, 2024

@sigprogramming
右クリック良いと思います!

デフォルト表示は小節.拍でも良いでしょうか?

これは結構自信を持って、とっつきやすさ優先で分・秒のが良いと感じてます。
(将来作ることになる気がしてる「DAW慣れてる人用初期設定」は、デフォルト小説・拍そちらでも良いかもです!)

@sevenc-nanashi
このUIだけ見るとありかもですが、ソフト全体的に左クリックでメニューが出るものがないので、UX考えるといったんやめたほうが良さそうに感じました。
似たような考え方で、メニューが開きそうな下三角▼ボタンを置いて、それクリックでメニュー開くとかならありかも・・・?
(まあ初学者に早めに発見してほしい度はそこまでだと感じてるので、▼置いてごちゃついちゃうよりは右クリックのが良さそうかも)

@sigprogramming
Copy link
Contributor Author

sigprogramming commented Oct 23, 2024

@Hiroshiba
デフォルト表示を分:秒にしました!

似たような考え方で、メニューが開きそうな下三角▼ボタンを置いて、それクリックでメニュー開くとかならありかも・・・?

StudioOneはこの形ですが、スペースをとってしまう&複雑なUIに見えてしまうので、一旦右クリックで…!
(ツールバーは表示をカスタマイズできるようにした方が良いかも、VSCodeは表示するものを右クリックで選択可能)


@romot-co
表示形式を小節.拍.16分.100分から小節.拍.100分に変更しています!
#2306 (comment)

@sigprogramming sigprogramming changed the title ソング:再生位置を小節・拍が分かる形式で表示するようにする ソング:再生位置の表示形式を変更できるようにする Oct 23, 2024
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.

あ、もうレビューしちゃっても大丈夫そうでしょうか 👀

とりあえず見た目周りで気になったことだけコメントしてみました!

src/components/Sing/PlayheadPositionDisplay.vue Outdated Show resolved Hide resolved
<div v-if="displayFormat === 'MeasuresBeats'" class="playhead-position">
<div>{{ measuresStr }}.</div>
<div>{{ beatsIntegerPartStr }}</div>
<div class="beats-fractional-part">.{{ beatsFractionalPartStr }}</div>
Copy link
Member

Choose a reason for hiding this comment

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

(割とただのコメントです)

ここの更新速度が早すぎて、若干目が使えるなと感じました!
ただまあ、別にこれでいい感じなのかなーとも思ってます。どうしようもない!

@sigprogramming
Copy link
Contributor Author

sigprogramming commented Oct 24, 2024

@Hiroshiba
大丈夫です!レビューよろしくお願いします!

src/components/Sing/PlayheadPositionDisplay.vue Outdated Show resolved Hide resolved
src/type/preload.ts Outdated Show resolved Hide resolved
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.

新しく増えたコンポーザブルに関してコメントしました!!
ちょっとリファクタリング頑張ろうとしたんですが、2時間かけて挫折しました。。。。。

@@ -9,6 +9,7 @@ import {
PhraseKey,
Track,
EditorFrameAudioQuery,
MeasuresBeats,
Copy link
Member

Choose a reason for hiding this comment

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

(ただのコメントです)

domainからstoreの型をimportしてるの、逆な気がしますね!!

今回のこの型に関してはstore側で全く使ってないので、多分こっちに定義する方が良さそう感あります。
まあ、いずれ移動すると良さそう・・・!
(src/sing/type.tsとかにでも。)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TempoTimeSignatureの型もsrc/store/type.tsで定義していますが、domain.tsで型を定義してsrc/store/type.tsでインポートする形が良いでしょうか?

Copy link
Member

Choose a reason for hiding this comment

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

個人的にはそちらの方があってるように感じます!

解釈としては、

  • store/typeはVuexのStoreの中で出てくるtype
    • が理想な気がするけど、現状いろんなものが置かれまくってしまっている・・・!
  • domain/sing
    • 何をドメインとするかわからないけど、少なくとも音楽や歌系の単位や型はここに置いても良さそう
    • domainはレイヤードアーキテクチャ的にはいろんなものから参照されて良いものなので、こっちを上流にすると良さそう

みたいな感じかなと!
ただ色々移動になってしまうと思うので、一旦このプリリクエストじゃないほうが良いのかもな~と思い、とりあえずコメントしてみた感じです!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

なるほどです!
ひとまずMeasuresBeatsのみdomain.tsに移しました!

src/composables/usePlayheadPosition.ts Outdated Show resolved Hide resolved
Comment on lines -257 to +254
const playheadPosition = new FrequentlyUpdatedState(0);
const playheadPosition = ref(0); // 単位はtick
Copy link
Member

Choose a reason for hiding this comment

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

(ただのコメントです)

なるほどです、globalにrefを定義する感じなんですね!!
知らなかったんですが、Vue的にもglobalにref変数を作るのは合法っぽみ!
https://ja.vuejs.org/guide/scaling-up/state-management#simple-state-management-with-reactivity-api

Copy link
Member

Choose a reason for hiding this comment

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

ちょっとこちらのプルリクエストとは関係ないのですが、たしか @romot-co さんに以前globalなrefを定義していいのか分からないとお伝えした記憶が蘇りました。
ドキュメントで例として案内されているので、グローバルに定義しても問題なさそうな雰囲気でした・・・!!! 🙇

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
src/components/Sing/PlayheadPositionDisplay.vue Outdated Show resolved Hide resolved
src/components/Sing/ScoreSequencer.vue Show resolved Hide resolved
@@ -23,7 +29,7 @@ export const isTracksEmpty = (tracks: Track[]) =>
export const isValidTpqn = (tpqn: number) => {
return (
Number.isInteger(tpqn) &&
BEAT_TYPES.every((value) => tpqn % value === 0) &&
BEAT_TYPES.every((value) => (tpqn * 4) % value === 0) &&
Copy link
Contributor Author

@sigprogramming sigprogramming Oct 28, 2024

Choose a reason for hiding this comment

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

PRと関係ないですが間違いに気づいたので修正しました…!

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

リファクタリングによってコードがスッキリして嬉しいです!!

コンフリクトが起こっていますが、おそらくstore.discpatch("HOGE")store.actions.HOGE()に変わったあたりの影響だと思うので、こちらで解消させていただこうと思います!

@Hiroshiba Hiroshiba merged commit 17b5920 into VOICEVOX:main Oct 28, 2024
8 checks passed
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.

ソング:再生位置を小節・拍が分かる形式で表示するようにする
4 participants