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: metasにsupported_featuresだけ追加 #900

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Dec 21, 2024

内容

#713 の前に、metasにsupported_featuresだけ入れる。

以下のPRを参考にコードを書いた。

@Hiroshiba と以下の1名の許諾のもと、 #874 にのっとりMITライセンスとしてライセンスする。

関連 Issue

Refs: #874 (comment)

その他

Co-authored-by: Segu <51497552+Segu-g@users.noreply.github.com>
Co-authored-by: Hiroshiba <hihokaruta@gmail.com>
@qryxip qryxip marked this pull request as ready for review December 22, 2024 06:37
@qryxip qryxip requested a review from Hiroshiba December 22, 2024 06:38
@Hiroshiba
Copy link
Member

Hiroshiba commented Dec 22, 2024

モーフィング周りはかなり考えることがあって時間がかかるので、優先度的にレビューは後回し気味でも問題なさそうでしょうか…?🙇

現状のエンジン側の仕様を整理し、コアも設計から再確認していく形になりそうです。
このPRの内容はAPIに現れないと思うので、このPRだけならそこまで考えずで実装しちゃっても問題なさそうかも。

@qryxip
Copy link
Member Author

qryxip commented Dec 23, 2024

#713 は後回し(来年)でもよいです。

このPRだけならそこまで考えずで実装しちゃっても問題なさそうかも。

このPRで考えることがあるとすれば、permitted_synthesis_morphingがVVMに含まれるべきか、ということになるかと思います。

このPRの内容はAPIに現れないと思うので、

一応現れることは現れますね。supported_featuresは見えるけどpermitted_synthesis_morphingはプライベートという風にもできることはできそうではあります。

image

@Hiroshiba
Copy link
Member

Hiroshiba commented Dec 23, 2024

あ〜どこにどう出すか、なんで名前にするか等いったん考えたいですね…!
とりあえず0.16まで止めるか、プライベートにするかになりそうです。
でもちょっと設計からな気がするから、とめておくのが丸そうかもです🙇

@qryxip
Copy link
Member Author

qryxip commented Dec 29, 2024

プライベートにするか

SpeakerMetaからsupported_featuresという名のオブジェクトが生えている事だけ確約しつつ、その型をトップ型 (top type)にしておくのはどうでしょうか?

  • Rust: serde_json::Value
  • Python: object
  • Java: java.lang.Object
  • TS: unknown

ただしJSONからデシリアライズする時はpermitted_synthesis_morphingまでちゃんと見るようにします。

C APIから出すときとC API以外でJSONにシリアライズするときは、permitted_synthesis_morphingの中身を出す手もありますが検閲(#[serde::skip_serializing])してもよいと思います(supported_featuresという名のフィールドごと消す)。

こうすればプライベートっぽくはなるはず。そうすれば後はVVMのmetas.jsonかmanifest.jsonに入れるかだけ考えればよいと思います。

@Hiroshiba
Copy link
Member

Hiroshiba commented Jan 3, 2025

supported_featuresという名前にするのか、そもそもどこに情報を格納するのかなどが未定だけど、何も案内せずしれっと何も入ってないプロパティを足すということですよね。

そこまでしなくても、いろいろ決まった後に追加でも良いのかも、とちょっと思いました!
あえて今実装したい理由とかはありますか?👀

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