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

Improve: Java APIを色々改善 #673

Merged
merged 16 commits into from
Nov 7, 2023
Merged

Improve: Java APIを色々改善 #673

merged 16 commits into from
Nov 7, 2023

Conversation

sevenc-nanashi
Copy link
Member

@sevenc-nanashi sevenc-nanashi commented Nov 3, 2023 •
edited
Loading

内容

  • Synthesizerに#getMetasと#isGpuModeを足します。
  • getSupportedDevices、getVersionを追加します。
  • サンプルを追加します。

関連 Issue

その他

サンプルはKotlinで書きました（今はAndroid版しか提供してないので）

色々と面倒なのでJava APIをKotlinで書きなおしても良いかも（KotlinのライブラリはJavaから呼べるはずなので）

Hiroshiba
Hiroshiba reviewed Nov 3, 2023
View reviewed changes
crates/voicevox_core_java_api/lib/src/main/java/jp/hiroshiba/voicevoxcore/Synthesizer.java Outdated
* @return メタ情報。
*/
@Nonnull
public VoiceModel.SpeakerMeta[] getMetas() {
Copy link
Member

@Hiroshiba Hiroshiba Nov 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

気になったので質問です！

これがJavaAPIのメソッドになる感じなんでしたっけ。
Rustは.metas、pythonもmetasなのですが、JavaはgetMetasになってるかも？ 👀
（言語仕様に詳しくないので、その方が望ましいのかがわからず。。）

Copy link
Member

Choose a reason for hiding this comment

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

JavaではgetMetasという名前は、「metasというメンバ変数が内部にあり、それに対するgetter」という意味であるのが一般的かと思います。

この命名をするのであれば、VoiceModelのようにSpeakerMeta[] metasを持って随時更新し、それに対するgetterという形の定義にした方がよいと思います。
(ちなみに私の感覚だとpublic finalというgetterを介さないメンバ変数の露出は、Javaの慣習に反するのではと思っています。)

Copy link
Member

Choose a reason for hiding this comment

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

たしかにgetMetasはmetasプロパティの単なるgetterじゃないので直感的じゃないかもと思いました。
audio_query辺りと一緒だから命名規則もこれに合わせておくと直感的かも。

ということで変更お願いできると･･･！！ @sevenc-nanashi

Copy link
Member Author

Choose a reason for hiding this comment

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

#metas()にしました（という解釈で合ってるのかな）

Hiroshiba
Hiroshiba reviewed Nov 3, 2023
View reviewed changes
example/java/app/src/main/kotlin/jp/hiroshiba/voicevoxcoreexample/App.kt Outdated
Comment on lines 45 to 46
val vvm = VoiceModel(vvmPath)
synthesizer.loadVoiceModel(vvm)
Copy link
Member

@Hiroshiba Hiroshiba Nov 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

ここだけPython版と変数名違いそうですね！
modelでもvvmでもどっちでも良さそうですが、揃えておくと後からメンテナンス簡単かなと！

Copy link
Member

Choose a reason for hiding this comment

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

このPRとは関係が薄いですが、VVPPがvvppだったりするなら、VoiceModelもいっそのことVvmにするのはありなんじゃないかと思います。コアの実装的にもONNXファイルと紛らわしいですし。

Copy link
Member

Choose a reason for hiding this comment

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

たしかにこのプルリクエストと関係が薄いかもです。
有用な議論だと思うので、あとから参照しやすいように移動しますか！
#545 (comment)

Hiroshiba
Hiroshiba reviewed Nov 3, 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.

色々と面倒なのでJava APIをKotlinで書きなおしても良いかも

ちょっと調べてみた感じ、Cの構造体に該当するものの定義が簡単だったり、null安全だったりするんですね。
でもまあkotlinならではの大変さとかもあると思う（ビルド周りとかで）ので、一長一短ということから個人的には賛成でも反対でもない（進めもしないが止めもしない）くらいの気持ちです！

Javaの有識者に聞いてもいいかも。

Hiroshiba
Hiroshiba reviewed Nov 3, 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.

コード読みました！example良い感じですね！！

Java exampleのREADMEもお願いしても良いでしょうか 🙇
もしよければその中に、自動生成された？っぽいgradlew.batなどを生成する方法もお願いできれば･･･！
（メンテナンスの参考になるので）

@qryxip
Copy link
Member

qryxip commented Nov 3, 2023 •
edited
Loading

色々と面倒なのでJava APIをKotlinで書きなおしても良いかも

やろうと思えばできる(極端な話、Scalaの機能マシマシで実装されたライブラリをKotlinで呼ぶといったこともおそらくできる)とは思いますが、ビルドの話を抜きでも若干反対寄りです。

結局Javaとしてのシグネチャを考えつつ実装することになると思いますし、それなら現状のJava 8にLombokとかを突っ込んでましにする、という方向を模索する方が苦しくならないのではと思っています。

@Hiroshiba Hiroshiba mentioned this pull request Nov 3, 2023
Open
69 tasks
@sevenc-nanashi
Copy link
Member Author

なるほど。
Kotlinのサンプル爆破してJavaにします。

@qryxip
Copy link
Member

qryxip commented Nov 3, 2023

あ、サンプルがKotlinなのは良いと思います。
(というよりJavaとKotlinとScalaの三つを揃えるとかでもよさそう)

@sevenc-nanashi
Copy link
Member Author

もしよければその中に、自動生成された？っぽいgradlew.batなどを生成する方法もお願いできれば･･･！
（メンテナンスの参考になるので）

gradlew.batはgradle init（poetry new的な）で生成されるので書くとこがないんですよね…（単体を生成する方法を知らないのもある）

@Hiroshiba
Copy link
Member

gradlew.batはgradle init（poetry new的な）で生成されるので書くとこがないんですよね…（単体を生成する方法を知らないのもある）

なるほどです！
gradle initが知りたかった対象でした。けどまあREADMEに記載しなくても、僕たちの間で把握できてりゃ良いかな。

Hiroshiba
Hiroshiba approved these changes Nov 5, 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！！！

@sevenc-nanashi
Copy link
Member Author

レビューを反映しました。

qryxip
qryxip approved these changes Nov 7, 2023
View reviewed changes
Copy link
Member

@qryxip qryxip 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 4c4b767 into VOICEVOX:main Nov 7, 2023
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qryxip qryxip qryxip approved these changes

@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.
3 participants
@sevenc-nanashi @qryxip @Hiroshiba