-
Notifications
You must be signed in to change notification settings - Fork 312
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
複数エンジン対応: DEFAULT_ENGINE_INFOS
に登録されたエンジンのプロセスをすべて起動する(Callback版)
#741
複数エンジン対応: DEFAULT_ENGINE_INFOS
に登録されたエンジンのプロセスをすべて起動する(Callback版)
#741
Conversation
src/background.ts
Outdated
log.info("ENGINE killed. Quitting app"); | ||
onAllKilled: () => { | ||
// executed asynchronously to catch all process closed event | ||
log.info("All ENGINE killed. Quitting app"); | ||
app.quit(); // attempt to quit app again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VOICEVOXエディタがエンジンのプロセスを管理していない場合(ターミナルでエンジンを起動しているなど)、app.quitが呼ばれない問題が発生しそう(修正したい)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
こちらの修正作業をするか、Promise版 #750 を代わりにレビューしてもらうか悩み中なのでDraftにしています
(Promise版は手元での動作確認中)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
いま db81c2b はbefore-quitからの非同期なapp.quitの呼び出しがループするようになっている...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#654 の段階で外部で起動されたエンジンがあるときにapp.quitが呼ばれない不具合が入っている気がする
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ちょっと状況を性格に把握できていないかもですが、自分で起動したエンジンは自分で終了するロジックにすると綺麗かもと思いました!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
自分で起動したエンジンは自分で終了するロジック
自分で起動していないエンジンは終了させないようにしています(プロセスを検索する必要がある+勝手に終了させるのは他のアプリケーションにとってよくなさそう)。
app.quitの呼び出しがループ
修正を入れました。直ったと思います。
(プロセスを1つもキルする必要がないときにapp.quitを呼び出して無限ループしていた)
DEFAULT_ENGINE_INFOS
に登録されたエンジンのプロセスをすべて起動するDEFAULT_ENGINE_INFOS
に登録されたエンジンのプロセスをすべて起動する(Callback版)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!
制御変数がなかなか多くなってきましたね・・・!
別途エンジンマネージャを作って、そちらで状態変数などを全部管理しちゃうとコードがかなり読みやすく・書きやすくなる気がしました!
もしよければ切り出しをお願いしたいかもです。
background.tsにエンジンプロセスの制御のための実装が多くなっているので、とりあえずファイル分けはできるといいなと思いました! |
内容
これまで、PR分割のため仮の実装として、エディタ起動時に
DEFAULT_ENGINE_INFOS
の0番目に登録されたエンジンだけが起動するようになっていました。このPRでは、エディタ起動時にすべてのエンジンのプロセスを起動するようにします。
この挙動は、今後エディタから各エンジンのAPIにアクセスする実装を開発するための暫定的なもので、変わる可能性があります。
将来的に、エンジンプロセスを並列起動できる可能性がありますが、実装の簡略化のため行っていません。
CPU/GPUフラグの個別化も考えられますが同様に省略し、設定UI上で決めた1つの設定がすべてのエンジンで使われます。これは別途Issueを立てて方針を検討したいです。
エディタ終了時は、すべてのエンジンのプロセスを終了するようにします。
また、Electronのメインプロセス(background.ts)で複数のエンジンプロセスを扱えるようにするため、
エンジンプロセスの起動関数runEngineAll/runEngine、再起動関数restartEngineAll/restartEngine、終了関数killEngineAll/killEngineを整備します。
すべてのエンジンを操作する*All関数は、既存のエンジンプロセス操作関数を置き換える形で簡易に実装するための暫定的なものです。今後、UI側のActionもいったん同様の実装で進めることを考えています。
動作テスト手順
プロセスの起動をテストするには、COEIROINK 0.2.0のエンジンのWindowsプレビュービルドをシロワニさんが作成してくださっています。
いまのところ、COEIROINKのエンジンAPIは
http://127.0.0.1:50031
で待ち受けるようになっているようです。VOICEVOXエンジンは、以下のリリースが利用できます(製品版同梱のものでも問題ないはず)。
Windows上で、VOICEVOXエンジン・COEIROINKエンジンを適当なディレクトリに配置後、.envファイルを以下のように書き換えて、VOICEVOXエンジン・COEIROINKエンジンのプロセスの起動を確認できます(engineKeyはランダム)。
エディタ起動・初期設定後、閉じるボタンを押して以下のようなログになれば成功です。
エディタ終了時に二重にエンジンプロセスのkillが走ってしまっていますが、このPR以前からそういう実装になっています(エディタの終了・エンジンプロセスの終了をできる限り保証するためのはずです。今後改修できそうならしたいです)。
関連 Issue
スクリーンショット・動画など
その他