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

feat: ソング:音素タイミングの表示エリアを追加 #2472

Merged
merged 33 commits into from
Jan 23, 2025

Conversation

sigprogramming
Copy link
Contributor

@sigprogramming sigprogramming commented Jan 4, 2025

内容

音素タイミングを表示するエリア(パラメーターパネル)を追加します。
(エリアの追加のみで、音素タイミングの表示は未実装)
ピアノロールの下に表示が良いかなと思っていて、ひとまずそれで実装しています。

パラメーターパネルを表示するかどうかの設定は開発時のみ機能にしています。

また、position: fixedで実装されているコンポーネント(CharacterPortraitとズームのスライダー)がシーケンサーにあったので、QSplitterを使ったときに正しく表示されるようにposition: absolute等にして調整を行いました。
具体的には以下を行いました。

  • CharacterPortraitsequencerBodyの中にありposition: fixed以外にできなかったので、以下を行った
    • SequencerGridsequencerBodyの外に出して、スクロール位置をoffsetX, offsetYに渡す形に変更
    • sequencerBodyの中にSequencerDummyGridを置いて正しいスクロール範囲になるようにした
    • CharacterPortraitsequencerBodyの外に出して、正しく表示できるように調整
      • メディアクエリを使用しない形に変更(flexプロパティとspacer要素を使用)
  • ズームのスライダーをposition: absoluteにした

ついでに以下も行いました。

  • sequencer-guidelinesequencerBodyの外に出した

関連 Issue

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

パラメーターパネルの追加

その他

@voicevox-preview-pages
Copy link

voicevox-preview-pages bot commented Jan 4, 2025

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

更新時点でのコミットハッシュ:b58632d

@@ -27,6 +27,7 @@ const meta: Meta<typeof Presentation> = {
offset: 0,
numMeasures: 32,
sequencerSnapType: 16,
uiLocked: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

storybookで警告が出ていたので修正しました。

@sigprogramming
Copy link
Contributor Author

設定(開発時のみ機能)を追加したことでマイグレーションのテストが通らなくなりましたが、マイグレーションをどう実装したら良いか分かりません…

@sigprogramming sigprogramming marked this pull request as ready for review January 5, 2025 02:15
@sigprogramming sigprogramming requested a review from a team as a code owner January 5, 2025 02:15
@sigprogramming sigprogramming requested review from Hiroshiba and removed request for a team January 5, 2025 02:15
Comment on lines 55 to 84
// 画面右下に固定表示
// 幅固定、高さ可変、画像のアスペクト比を保持、wrapのwidthに合わせてheightを調整
// bottom位置はスクロールバーの上に表示
// 幅固定、高さ可変、画像のアスペクト比を保持、heightを調整
.character-portrait-wrap {
opacity: 0.55;
overflow: visible;
contain: layout;
position: relative;
height: max(100%, $portrait-min-height);
margin-right: $right-margin;
overflow: hidden;
display: flex;
flex-direction: column;
justify-content: flex-end;
align-items: flex-end;
pointer-events: none;
position: fixed;
display: grid;
place-items: end;
bottom: 0;
right: $right-margin;
}

.spacer {
flex: 0 1 $spacer-height;
width: 1px;
}

.character-portrait {
width: auto;
height: $portrait-max-height;
min-height: $portrait-min-height;
flex: 1 0;
min-width: $portrait-min-width;
max-width: $portrait-max-width;
overflow: visible;
min-height: $portrait-min-height;
max-height: $portrait-max-height;
opacity: 0.55;
backface-visibility: hidden;
object-fit: cover;
object-position: top center;
}

// ポートレートサイズが画面サイズを超えた場合、ヘッダーを考慮してポートレートを上部基準で表示させる
// ヘッダー高さ120px+ポートレート高さ500pxだとする
@media (max-height: #{calc(#{$portrait-min-height} + #{$header-margin})}) {
.character-portrait-wrap {
top: $header-margin; // ヘッダーの高さより下に位置させる
bottom: auto;
height: calc(100vh - #{$header-margin});
place-items: start end;
}
object-fit: contain;
object-position: bottom center;
}
Copy link
Contributor Author

@sigprogramming sigprogramming Jan 5, 2025

Choose a reason for hiding this comment

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

@romot-co
できるだけ以前と同じ挙動になるようにしましたが、正しく実装できているか自信が無いので、スタイル周り、ご確認いただけると助かります…!
contain: layout;など、効いていなさそうなプロパティを消しましたが、必要なものも消しているかもしれません…)

Copy link
Contributor

Choose a reason for hiding this comment

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

@sigprogramming
こちら確認いたします!

Copy link
Contributor

@romot-co romot-co Jan 17, 2025

Choose a reason for hiding this comment

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

メモ書きです…!

  • contain: layoutfixed以外で実装しようとした時の名残なので消しても大丈夫そうです(スクロール時のパフォーマンス上がることがあるので残しておいても問題ないはず)
  • コンテナ要素に対してposition: sticky+子にmax-width指定が一番楽そう
.portrait-wrap {
  position: sticky;
  bottom: 0;
  right: 0;
  width: 320px; /* 横長・縦長対応がきつそう... */
}

.portrait-image {
  width: 100%;
  height: auto;
  max-height: 300px /* 仮 */
  object-fit: contain;
  position: absolute; /* wrapに対し固定 */
  bottom: auto;
  bottom: 0;
}

// あとはContainer Queryなど?

ほか気になっているやつ:
https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_anchor_positioning

Copy link
Contributor

Choose a reason for hiding this comment

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

@sigprogramming
こちら以下で修正いたしました…!挙動他問題や影響ありましたらおしらせください!

sigprogramming@6c942e9

Copy link
Contributor Author

@sigprogramming sigprogramming Jan 18, 2025

Choose a reason for hiding this comment

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

@romot-co
ありがとうございます!!とても助かります…!

挙動を確認してみたのですが、今のmainブランチではシーケンサーのheightがある程度大きくなると立ち絵も拡大されるようになっていて、 6c942e9 では拡大されなくなっているかもです。ここの挙動、どうしましょう…

また、position: stickyありとposition: stickyなしの挙動の差があまり分かりませんでした…
調べてみたところ、position: stickyになっている要素がスティッキーアイテムで、その親がスティッキーコンテナになるようです。
https://coliss.com/articles/build-websites/operation/css/css-position-sticky-how-it-really-works.html
なので、 6c942e9 の実装だとcharacter-portrait-wrapがスティッキーアイテム、score-sequencerがスティッキーコンテナになりますが、character-portrait-wrapはグリッドアイテムでもあるので、もしかしたら上手く機能していないかも…?

Copy link
Member

Choose a reason for hiding this comment

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

ちょっと自分も試してみたんですが、原理は全く分かりませんがなんかこれでうまいことできた気がします!!
PRも作ってみました! sigprogramming#4

<style scoped lang="scss">
@use "@/styles/variables" as vars;
@use "@/styles/colors" as colors;

// 表示変数
$portrait-min-height: 500px;

// 画面右下に固定表示
// 幅固定、高さ可変、画像のアスペクト比を保持、heightを調整
.character-portrait-wrap {
  height: 100%;
  display: flex;
  flex-direction: column;
  margin-left: auto;
  overflow: hidden;
}

// 通常は下部基準だが、親要素が最小高さより小さい場合は上部基準とし頭を残して足から隠れさせる
.character-portrait {
  display: block;
  margin-top: auto;
  min-height: max(75%, $portrait-min-height);
  opacity: 0.55;
  backface-visibility: hidden;
  object-fit: contain;
}
</style>

ちなみにChatGPTに聞きました。o1君、結構CSSの強い味方かもしれない。
https://chatgpt.com/share/678d25ad-68f0-8008-b197-0c1b7da2b59b

Copy link
Contributor

Choose a reason for hiding this comment

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

@Hiroshiba
こちら確認しました!
簡潔な手法でだいぶベストに思えます…!

o1すごい…!

$portrait-min-height: 500px; // 最小高さ
$portrait-max-height: 60%; // 最大高さ 75%だと画面によってはでっかすぎるので60%ぐらいがよさそう

// 以下変更なし・動作のコメントだけ付与

// 画面右下に固定表示
// 幅固定、高さ可変、画像のアスペクト比を保持、heightを調整
.character-portrait-wrap {
  display: flex;
  flex-direction: column;
  // コンテナ要素に対し高さを100%にし、固定右寄せ
  height: 100%;
  margin-left: auto;
  // 内部スクロールを行わせない
  overflow: hidden;
}

// 通常は最下部に表示し、親要素が最小高さより小さい場合は上部基準とし頭を残して足から隠れさせる
.character-portrait {
  display: block;
  // wrapに対し下寄せ
  margin-top: auto;
  // 最小高さと最大高さのうち大きい方を適用
  min-height: max($portrait-min-height, $portrait-max-height);
  // アスペクト比を保持
  object-fit: contain;
  // 透明度
  opacity: 0.55;
  // 以下なくてもいいけどあるとGPU効きがち
  backface-visibility: hidden;
  transform: translateZ(0);
}

+たぶん
ポートレート幅 > 親のQSplitter幅の場合に
にスクロールバーが二重に出るかもなので

<QSplitter
     ...
    style="overflow: hidden" // スクロールさせない
>

みたいにすれば完了に思えます!
あとで修正PR送ります…!

@Hiroshiba
Copy link
Member

マイグレーションのテストが通らなく

これはunitテストのsnapshotを更新すれば良さそうです!
ローカル環境でnpm run test:unit -- --update # スナップショットの更新を実行していただければ!!

どういうタイミングで何のスナップショットを更新すれば良いのかわかりづらい・・・というよりわからないですね・・・・・・。
どうすれば良いのか考えてみます 🙇

@sigprogramming
Copy link
Contributor Author

@Hiroshiba
ありがとうございます!スナップショットを更新しました!

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.

レビューしました!
spliterを導入して、各ペインでdivが一枚ずつ挟まってる構造、良さそう!

かなりいろいろ試行錯誤があってこのCSSにたどり着いたんだと思います、ありがとうございます!!!
DummyGridを用意してスクロールイベントを同期させるの、複雑性が上がっているのでなんとかできないか調べてたのですが、親にcontain: layoutを指定すればfixedの固定位置がその親依存になるっぽいです!!!!!
https://developer.mozilla.org/ja/docs/Web/CSS/contain#%E3%83%AC%E3%82%A4%E3%82%A2%E3%82%A6%E3%83%88%E6%8B%98%E6%9D%9F

実際.score-sequencerの中にcontain: layout;入れたら、元のコードでも大部分動きそうでした!!
たぶんDummyGridをなくせて、fixedに戻せて、立ち絵・グリッド・ガイドラインももとに戻せる気がします。
(こっちのほうが構造的に正しいので可能ならこうしておくと機能継ぎ足しが簡単そう)

立ち絵のメディアクエリだけはどうしてもこのPRのように別解を探す必要がありそうな気がしました!

src/components/Dialog/SettingDialog/SettingDialog.vue Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

(メモ)
間違い探しレベルですが、右下の縮小スライダーの位置が微妙に変わってますね!
問題なさそう感!

src/components/Sing/SequencerParameterPanel.vue Outdated Show resolved Hide resolved
src/components/Sing/SequencerDummyGrid.vue Outdated Show resolved Hide resolved
src/components/Sing/ScoreSequencer.vue Show resolved Hide resolved
src/components/Sing/ScoreSequencer.vue Outdated Show resolved Hide resolved
@sigprogramming
Copy link
Contributor Author

@Hiroshiba
レビューありがとうございます!

親にcontain: layoutを指定すればfixedの固定位置がその親依存になるっぽいです!!!!!

知りませんでした…!ありがとうございます!

こっちのほうが構造的に正しいので可能ならこうしておくと機能継ぎ足しが簡単そう

構造的に正しいというのはDummyGridをなくせるからでしょうか?
確かに元の形に戻すとDummyGridをなくせますが、今後CanvasやWebGLにしたりスクロールの自作などを行うのであれば、今のPRの形が良いかなと思います。
(DummyGridが必要なくなるのと、立ち絵・グリッド・ガイドラインを今の形に変更することになるので)

@Hiroshiba
Copy link
Member

@sigprogramming

構造的に正しいというのはDummyGridをなくせるからでしょうか?

ですです!
でもだいぶ違うと思ってたのですが、改めて考えたらほんとにDummyGridがあるかないかぐらいでした。

整理すると、今のPRのコードはこうなっていて、

root
┗ 左上の角
┗ ルーラー
┗ 鍵盤
┗ グリッド
┗ キャラクター全身
┗ ノート入力のための補助線
┗ シーケンサ
 ┗ ダミーグリッド
 ┗ シャドーノート
 ┗ ノート
 ┗ 歌詞
┗ ピッチ
┗ 拡大率スライダー

元のコードはこんな感じかなと。

root
┗ 左上の角
┗ ルーラー
┗ 鍵盤
┗ シーケンサ
 ┗ グリッド
 ┗ ノート入力のための補助線
 ┗ キャラクター全身
 ┗ シャドーノート
 ┗ ノート
 ┗ 歌詞
┗ ピッチ
┗ 拡大率スライダー

グリッドから拡大率スライダーは全部同じパネルに乗っかってるので、シーケンサdiv以下にあるのが良いかなと思ってて、だとしたら一塊に近かった以前の構造が合ってそうだな~と思った次第です!
DOM的にも、シーケンサがスクロールを持って、パーツは全部そこに乗っかってる形が作りやすそうだなーとも。(contain: layout使えば・・・!)

まあcanvasとかに置き換えるなら今はどちらでも良いかもなのですが、こっちの方はDummyがないのもあって全体が把握しやすいかもです!
あとcontain: layoutならfixedを使えるので、absoluteよりも実装が楽かも。(これはあまり変わらないかも)

ただdisplay: gridで同じ場所に重ね起きするこのPRの実装だと、2つのエリアに貫通するUIとかを作りやすいので、お互い一長一短かもなのですが 😇

@Hiroshiba
Copy link
Member

Hiroshiba commented Jan 18, 2025

ダミーグリッドを使うかどうかもう少し考えてみました!
ちょっとどっちが楽か予測しきれないのと、一長一短なのとで、別にダミーグリッドを使う形でも良いのかな〜と思い始めました!
(戻すの大変とかもあると思うので 🙇 )

コードがコピペになってしまうのはちょっと避けたいので、provide/inject形式にだけはしておきたいかもです!
provide/injectはなんかだいぶややこしいので、不明な点があったら何でも聞いちゃっていただければ!!

@sigprogramming
Copy link
Contributor Author

ちょっと調整を行います!

@Hiroshiba
Copy link
Member

Hiroshiba commented Jan 22, 2025

あ、スクショがコンフリクトしていそうです!
たぶんmainブランチをマージして[update snapshots]というコミットメッセージを含めるか空コミットすれば良い感じに更新できるのかなと!

(コンフリクト解消すると、プレビュー用のページやStorybookの確認がpreview-pages通してブラウザで可能になるので便利です)

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です!!!あと少し!!

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・・・なのですが、mainブランチのマージコミットがsquashされて(?)差分がわからないようになっちゃって変更箇所が不明になってるかもです!
image

ちょっとこっちでmainブランチマージして、まだスクショがコンフリクトしているのでそれも直しちゃおうと思います!

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 さんすみません、たぶん大丈夫だと思うのでマージさせていただきます! 🙏

@Hiroshiba Hiroshiba changed the title ソング:音素タイミングの表示エリアを追加 feat: ソング:音素タイミングの表示エリアを追加 Jan 23, 2025
@Hiroshiba Hiroshiba enabled auto-merge January 23, 2025 17:31
@Hiroshiba Hiroshiba added this pull request to the merge queue Jan 23, 2025
Merged via the queue into VOICEVOX:main with commit 8c4f742 Jan 23, 2025
11 checks passed
@sigprogramming sigprogramming deleted the add_parameter_panel_area branch January 25, 2025 14:50
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