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

Add: manifest.jsonを追加 #426

Merged
merged 12 commits into from
Jun 26, 2022

Conversation

sevenc-nanashi
Copy link
Member

内容

manifest.jsonを追加します。

関連 Issue

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

その他

@github-actions
Copy link

github-actions bot commented Jun 25, 2022

Coverage Result

Resultを開く
Name Stmts Miss Cover
voicevox_engine/init.py 1 0 coverage-100%
voicevox_engine/acoustic_feature_extractor.py 75 0 coverage-100%
voicevox_engine/dev/synthesis_engine/init.py 2 0 coverage-100%
voicevox_engine/dev/synthesis_engine/mock.py 36 2 coverage-94%
voicevox_engine/full_context_label.py 162 3 coverage-98%
voicevox_engine/kana_parser.py 86 1 coverage-99%
voicevox_engine/model.py 149 7 coverage-95%
voicevox_engine/mora_list.py 4 0 coverage-100%
voicevox_engine/part_of_speech_data.py 5 0 coverage-100%
voicevox_engine/preset/Preset.py 12 0 coverage-100%
voicevox_engine/preset/PresetLoader.py 34 1 coverage-97%
voicevox_engine/preset/init.py 3 0 coverage-100%
voicevox_engine/synthesis_engine/init.py 5 0 coverage-100%
voicevox_engine/synthesis_engine/core_wrapper.py 199 159 coverage-20%
voicevox_engine/synthesis_engine/make_synthesis_engines.py 48 39 coverage-19%
voicevox_engine/synthesis_engine/synthesis_engine.py 127 11 coverage-91%
voicevox_engine/synthesis_engine/synthesis_engine_base.py 66 9 coverage-86%
voicevox_engine/user_dict.py 131 10 coverage-92%
voicevox_engine/utility/init.py 3 0 coverage-100%
voicevox_engine/utility/connect_base64_waves.py 35 3 coverage-91%
voicevox_engine/utility/engine_root.py 9 2 coverage-78%
TOTAL 1192 247 coverage-79%

@sevenc-nanashi sevenc-nanashi marked this pull request as ready for review June 25, 2022 22:11
@Hiroshiba
Copy link
Member

PRありがとうございます!!

iconはパスの方が取り回ししやすそうだなと感じました。
(デバッグしやすい・generate manifestしなくて済むなど)

base info.jsonと役割が完全に被ってるので、base info.jsonをmanifest.jsonに置き換えて、機能も継承するのはどうでしょう👀

あとちょっと迷ってるのですが、たしかEngineManifestクラス内でbase info.jsonの情報を読み込んでいます。
今はPython内でファイルを読んでAPIで返す形式になってますが、これをmanifest.json等から直接ファイルを読んでもらう形式にすべきか迷ってます…。
@takana-v さんや @aoirint さんの意見伺いたいかもです🙇‍♂️

@sevenc-nanashi
Copy link
Member Author

iconはパスの方が取り回ししやすそうだなと感じました。

確かにそうなんですけど、UIでアイコンを出す時にパス関連の色々がなくせたりするのでbase64もbase64でいいんですよね。

image
"base64:aajsfhaksjhfkfha"みたいに指定することによりbase64もサポートするようにする…?

base info.jsonと役割が完全に被ってるので、base info.jsonをmanifest.jsonに置き換えて、機能も継承するのはどうでしょう

これは良さそうですね、変えます。

@Hiroshiba
Copy link
Member

Hiroshiba commented Jun 26, 2022

たしかにbase64にしとく良さもあると思います!
難しい判断なんですが、複雑度を下げるためにもgenerate manifestの工程をなくしときたい感じです。
工程が少ない方が文化が広がりやすいし、スマートにしておきたいなぁと。。

electronからファイル読み込むの面倒という感じでしたら、EngineManifestクラス内でiconをロードしてAPI経由でバイナリを返すという手もありそうです。
ただたぶんこちらの方が、openapiとエディタ側の実装の関係でより大変かも。。

@Hiroshiba Hiroshiba closed this Jun 26, 2022
@Hiroshiba
Copy link
Member

Hiroshiba commented Jun 26, 2022

すみません!間違えました。。

@Hiroshiba Hiroshiba reopened this Jun 26, 2022
@sevenc-nanashi
Copy link
Member Author

sevenc-nanashi commented Jun 26, 2022

試しにパスで色々やろうとしたんですけど、どうやらElectronの関連でfile://が使えないらしく苦戦しています。
image

electronからファイル読み込むの面倒という感じでしたら、EngineManifestクラス内でiconをロードしてAPI経由でバイナリを返すという手もありそうです。

これのほうがよさそう。

@sevenc-nanashi
Copy link
Member Author

エディタのElectron側でbase64にしちゃう、って手もありますね。(Electron側でアイコンをbase64化し、エディタのVueに送る)

@Hiroshiba
Copy link
Member

よくよく考えると、アイコンはエンジン未起動(バグって起動しないとか)でも表示したいので、API経由だとまずいかもという気がしました・・・!

base64にしちゃう

行けると思います!
VOICEVOXエディタはちょっと複雑になっていて、Electron・Vuex・Vueで分かれてるのですが、Vuexで読み込んでVueで参照の形がきれいなのかなと思いました。

その場合はこちらのファイル読み込みが使えるかもです、もしよかったら・・・!
https://github.com/VOICEVOX/voicevox/blob/aeda4aa004982ccaacefdb3aa91b4f31a3e7e232/src/store/project.ts#L102

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.

ちょっと議論が必要そうなので、一旦generate_manifest.py周りだけ別PRかissueにして頂けると・・・!

manifest_assets/manifest.json 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.

LGTM!!

ぜひ、どんどん実装していって頂けると!!!

@Hiroshiba Hiroshiba merged commit 2270fee into VOICEVOX:master Jun 26, 2022
sabonerune pushed a commit to sabonerune/voicevox_engine that referenced this pull request Dec 16, 2022
* Add: manifest.jsonを追加

* Fix: 空白を削除

* Add: workflowに生成処理を追加

* Add: アイコンを設定

* Fix: URLsafe Base64に

* Fix: 最後の=を削除

* Revert "Fix: URLsafe Base64に"

This reverts commit 855ee09.

* Revert "Fix: 最後の=を削除"

This reverts commit 7408712.

* Change: base_info.jsonをmanifest.jsonで置き換え

* Change: iconをパスに

* Delete: generate_manifest.pyを削除

* Fix: jsonの幅を修正
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants