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.

Sign up for GitHub

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

Jump to bottom

morphable_targetsAPIの実装 #588

Merged
merged 26 commits into from
Jan 23, 2023
Merged

morphable_targetsAPIの実装 #588

merged 26 commits into from
Jan 23, 2023

Conversation

Segu-g
Copy link
Member

@Segu-g Segu-g commented Jan 22, 2023

内容

関連 Issue

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

その他

@github-actions
Copy link

github-actions bot commented Jan 22, 2023 •
edited
Loading

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/metas/Metas.py 33 0 coverage-100%
voicevox_engine/metas/MetasStore.py 29 14 coverage-52%
voicevox_engine/metas/init.py 2 0 coverage-100%
voicevox_engine/model.py 145 9 coverage-94%
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 206 166 coverage-19%
voicevox_engine/synthesis_engine/make_synthesis_engines.py 57 49 coverage-14%
voicevox_engine/synthesis_engine/synthesis_engine.py 130 11 coverage-92%
voicevox_engine/synthesis_engine/synthesis_engine_base.py 67 9 coverage-87%
voicevox_engine/user_dict.py 129 6 coverage-95%
voicevox_engine/utility/init.py 3 0 coverage-100%
voicevox_engine/utility/connect_base64_waves.py 37 0 coverage-100%
voicevox_engine/utility/path_utility.py 26 6 coverage-77%
TOTAL 1289 277 coverage-79%

@Segu-g Segu-g marked this pull request as ready for review January 22, 2023 12:56
@Segu-g Segu-g requested a review from a team as a code owner January 22, 2023 12:56
@Segu-g Segu-g requested review from Hiroshiba and removed request for a team January 22, 2023 12:56
Hiroshiba
Hiroshiba reviewed Jan 22, 2023
View reviewed changes
run.py Outdated Show resolved Hide resolved
Hiroshiba
Hiroshiba reviewed Jan 22, 2023
View reviewed changes
Copy link
Member

@Hiroshiba Hiroshiba left a comment •
edited
Loading

Choose a reason for hiding this comment

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

ちょっと気づかずにごちゃごちゃコメントを書いてしまったのですが、

  • MetasStoreをdictかModel等の型どちらを持たせる構造にするか
    • dictは結構簡単にバグりそう
    • すぐ修正できるならModelのが良さそう
    • 難しそうならスピード優先でマージが良さそう

という感じです！ 🙇‍♂️
個人的にはスピード優先でマージし、「型があるなかで変換したほうが良さそう」みたいなFIXMEコメント付けるのが塩梅かなと思いました。

Hiroshiba
Hiroshiba reviewed Jan 22, 2023
View reviewed changes
run.py Outdated
Comment on lines 499 to 503
summary="base_speakersに指定した話者に対してエンジン内の話者がモーフィングが可能かどうか返す",
)
def is_morphable(
def morphable_targets(
base_speaker: int,
target_speaker: int,
core_version: Optional[str] = None,
Copy link
Member

Choose a reason for hiding this comment

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

あ、引数がbase_speaker（単数）のままですね！
単数でも複数でもどっちでも良いかなと思います。
けどやっぱり複数のが取り回ししやすいかも？

Copy link
Member

@Hiroshiba Hiroshiba Jan 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

あ、ここどっちに倒しましょう？ 👀
（コメントと実装が違うことが気になっています）

機構はできてるのであとはfor回せば複数人対応行けそうですし、まあ１人ずつでも良い気もしますし、って感じです！
５５：４５くらいの気持ちで複数人のが良いかな、という気持ちです。

Copy link
Member Author

Choose a reason for hiding this comment

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

とりあえず実装しました02ce3fc

Hiroshiba
Hiroshiba reviewed Jan 22, 2023
View reviewed changes
run.py Outdated Show resolved Hide resolved
Hiroshiba
Hiroshiba reviewed Jan 22, 2023
View reviewed changes
run.py Outdated Show resolved Hide resolved
Hiroshiba
Hiroshiba reviewed Jan 23, 2023
View reviewed changes
voicevox_engine/model.py Outdated Show resolved Hide resolved
Hiroshiba
Hiroshiba reviewed Jan 23, 2023
View reviewed changes
voicevox_engine/morphing.py

# FIXME: 他にsupported_featuresができたら共通化する
base_speaker_morphing_info: SpeakerSupportPermittedSynthesisMorphing = (
Copy link
Member

Choose a reason for hiding this comment

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

ここいい感じになったので型宣言いらなそうですね！
（suggestionできませんでした。。）

Hiroshiba
Hiroshiba approved these changes Jan 23, 2023
View reviewed changes
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！！

いろいろコメントしてますがだいたいコメントのadd suggestなのでLGTMです！
修正ありがとうございました！！！

@Hiroshiba
Copy link
Member

マージします！！@Segu-g さんありがとうございました！！！！！

@Hiroshiba Hiroshiba merged commit fc4a99c into VOICEVOX:master Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hiroshiba Hiroshiba Hiroshiba approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Segu-g @Hiroshiba