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

Feature/gml2gltf #174

Merged
merged 8 commits into from
Jan 16, 2024
Merged

Feature/gml2gltf #174

merged 8 commits into from
Jan 16, 2024

Conversation

sato-toshifumi
Copy link
Contributor

musamai/nusamai-gltf/examples/geometry_to_gltf.rs を元に glb を吐き出す nusamai/nusamai/examples/gml2gltf.rs を作成しました。
glbのJSON チャンクが 4byte-aligned ではないと BIN チャンクの読込みをしくじる(読込み側で alignment は考慮しない)ということの様です。JSON チャンクはテキストなので 0x20 を足して4バイト境界に揃える仕様みたいです(0x0 でも問題はないと思いますが)。

付随して Gltf 構造体を new で作成すると初期値の設定ができなさそうなので、new() を隠して生成用の関数 construct() を作りました。asset の version と generator くらいは初期値を入れたかったので。

Copy link

codecov bot commented Jan 12, 2024

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Files Patch % Lines
nusamai-gltf/src/models/gltf.rs 0.00% 10 Missing ⚠️
Additional details and impacted files
Components Coverage Δ
GUI ∅ <ø> (∅)
Backend 75.74% <ø> (-4.16%) ⬇️
Libraries 86.19% <0.00%> (-1.21%) ⬇️

📢 Thoughts on this report? Let us know!

Comment on lines 93 to 95
fn new() -> Self {
Default::default()
}
Copy link
Member

Choose a reason for hiding this comment

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

あ、Rustでは、デフォルト値は基本的に Default trait で定めることがイディオムとなっていまして、実は Gltf::new() をするだけで、Asset の version = "2.0" も含めて設定されるように、既になっています(generatorは設定していませんが)。

(つまり西尾さんが書いた examples のコードは少し無駄な処理をしています)。

従前の Gltf::new で呼んでいる Default::default() が芋蔓式に Asset の impl Default なども呼びまして、デフォルト値がセットされます。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

仕掛けがあるのは気付いていましたが、うまく値がセットされていなかった様なので改修した次第です。

Copy link
Member

Choose a reason for hiding this comment

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

ありがとうございます。確認してみます。

Copy link
Member

@ciscorn ciscorn Jan 12, 2024

Choose a reason for hiding this comment

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

@sato-toshifumi
Asset の Default の実装を直しました 🙇 6d45717

(他のstructについてもデフォルト値の扱いに問題があろうと思いますので、今後直していければと思います)

Copy link
Member

@ciscorn ciscorn left a comment

Choose a reason for hiding this comment

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

ありがとうございます。
LGTM!

@nokonoko1203 nokonoko1203 merged commit 67fbed4 into main Jan 16, 2024
2 checks passed
@nokonoko1203 nokonoko1203 deleted the feature/gml2gltf branch January 16, 2024 01:30
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.

3 participants