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: VST版エディタをマージ #2521

Open
wants to merge 113 commits into
base: project-vst
Choose a base branch
from

Conversation

sevenc-nanashi
Copy link
Member

内容

自リポジトリのVST版エディタをマージします。

関連 Issue

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

(なし)

その他

(なし)

@sevenc-nanashi sevenc-nanashi requested a review from a team as a code owner February 6, 2025 04:22
@sevenc-nanashi sevenc-nanashi requested review from Hiroshiba and removed request for a team February 6, 2025 04:22
@Hiroshiba
Copy link
Member

あ、いったんprojectブランチへのマージができればと思ってます!
project-vstとかを作っていただければ!

理由はmainマージした後、技術選定や設計レベルで課題が見つかったときに、pcソフト側のリリースに影響が出ないようにしたいためです…!

@sevenc-nanashi sevenc-nanashi changed the base branch from main to project-vst February 6, 2025 04:23
@sevenc-nanashi
Copy link
Member Author

project-vstに向けました。

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 20 out of 35 changed files in this pull request and generated 1 comment.

Files not reviewed (15)
  • package.json: Language not supported
  • src/components/App.vue: Evaluated as low risk
  • src/components/Sing/ScoreSequencer.vue: Evaluated as low risk
  • src/components/Menu/MenuBar/MenuBar.vue: Evaluated as low risk
  • src/components/EngineStartupOverlay.vue: Evaluated as low risk
  • src/components/Dialog/VstRoutingDialog/Presentation.vue: Evaluated as low risk
  • src/components/Dialog/VstRoutingDialog/Container.vue: Evaluated as low risk
  • src/components/Dialog/AllDialog.vue: Evaluated as low risk
  • src/components/Dialog/Dialog.ts: Evaluated as low risk
  • src/backend/vst/vstConfig.ts: Evaluated as low risk
  • src/backend/vst/vstPlugin.ts: Evaluated as low risk
  • src/components/Dialog/VstRoutingDialog/index.stories.ts: Evaluated as low risk
  • src/backend/browser/sandbox.ts: Evaluated as low risk
  • .eslintrc.js: Evaluated as low risk
  • pnpm-lock.yaml: Evaluated as low risk
Comments suppressed due to low confidence (2)

src/backend/vst/ipc.ts:38

  • [nitpick] The RustError class could be more descriptive. Consider renaming it to something like VstRustError.
class RustError extends Error {

src/backend/electron/manager/windowManager.ts:90

  • Opening developer tools by default might not be intended for production. Consider removing or conditionally enabling it.
win.webContents.openDevTools();

src/backend/vst/ipc.ts 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.

コードなるほどでした!!

ちょっとまだ大まかにしか読めてないのですが、一旦レビューコメントまで・・・!

.eslintrc.js Outdated Show resolved Hide resolved
src/backend/browser/sandbox.ts Outdated Show resolved Hide resolved
src/backend/vst/ipc.ts Outdated Show resolved Hide resolved
src/backend/vst/ipc.ts Outdated Show resolved Hide resolved
src/backend/vst/vstPlugin.ts Outdated Show resolved Hide resolved
src/components/Sing/ScoreSequencer.vue Outdated Show resolved Hide resolved
Comment on lines 42 to +43
let engineInfos;
if (isElectron) {
if (isElectron && isNode) {
Copy link
Member

Choose a reason for hiding this comment

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

特にVSTに関係ない変更ですかね?

変更自体は良さそう感!
でもいつの間にかmain側でこの辺が変わって、projectブランチがマージされるころにはこのコードを忘れていて、マージしてバグるということが起きそうかも。
mainにマージできると嬉しそう!

Copy link
Member Author

Choose a reason for hiding this comment

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

特にVSTに関係ない変更ですかね?

  • vst版のプラグインを差し込むためにvst/vstPlugin.tsが読まれる
  • vstPlugin.tsはbrowser/contract.tsが依存にあるのでそれが読み込まれる
  • browser/contract.tsの時限爆弾(この行)が爆発する

って感じでした。

mainにマージできると嬉しそう!

後でPRします。

* trueが返されたらunwatchするwatch
* "unwatch"が返されたらunwatchするwatch
Copy link
Member

Choose a reason for hiding this comment

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

これもついでにmainに入れられると・・・!
(でも流石に変わらなそうなのでどっちでも良さそう)

@@ -535,14 +535,14 @@ if (window.AudioContext) {
}

const playheadPosition = ref(0); // 単位はtick
const phraseSingingVoices = new Map<SingingVoiceKey, SingingVoice>();
export const phraseSingingVoices = new Map<SingingVoiceKey, SingingVoice>();
Copy link
Member

Choose a reason for hiding this comment

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

exportしてるけど使ってない・・・?

const sequences = new Map<SequenceId, Sequence & { trackId: TrackId }>();
const animationTimer = new AnimationTimer();

const queryCache = new Map<EditorFrameAudioQueryKey, EditorFrameAudioQuery>();
const singingPitchCache = new Map<SingingPitchKey, SingingPitch>();
const singingVolumeCache = new Map<SingingVolumeKey, SingingVolume>();
const singingVoiceCache = new Map<SingingVoiceKey, SingingVoice>();
export const singingVoiceCache = new Map<SingingVoiceKey, SingingVoice>();
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
Member Author

Choose a reason for hiding this comment

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

こっちはvst/vstPlugin.tsで使っています。

Copy link
Member

Choose a reason for hiding this comment

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

おっとなるほどです。。
使ってるとなると、後々設計的にしんどくなってきそうな結合になってそうですねぇ。。。

ちなみにちょっと偉そうなことを言うのですが、これがよく言われる”コードの臭い”なので、感じれるようになるとプログラマとして成長したことになるかもです。
言語化すると、明らかにVuex.Store内でしか使われてなかった、つまりそういうふうに設計されていたであろうプライベートな概念を引きずり出して接続している形なのでまずい。

用途としてはたしか、VSTを開いたり閉じたりするたびにフロントが初期化されてキャッシュが消えちゃうから、毎回エンジンで音声合成しないといけなくなっちゃうのの防止ですよね。
ここの設計どういう手がありそうですかねぇ。。。

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.

2 participants