spz 2.0へのアプデプルリクのレビュースレ
にー兄さんCesiumGSからspz-loaderへ、spz 2.0対応のプルリクが飛んできたのでレビューする
とてもありがたい
にー兄さん今回のプルリクの主な焦点は、2.0から追加された座標系変換周りである
SPZではこんな感じに座標系のenumが提供され、それを指定して変換が可能になった
enum class CoordinateSystem {
UNSPECIFIED = 0,
LDB = 1, // Left Down Back
RDB = 2, // Right Down Back
LUB = 3, // Left Up Back
RUB = 4, // Right Up Back, Three.js coordinate system
LDF = 5, // Left Down Front
RDF = 6, // Right Down Front, PLY coordinate system
LUF = 7, // Left Up Front, GLB coordinate system
RUF = 8, // Right Up Front, Unity coordinate system
};
にー兄さんemscriptenにおけるenumの扱いはこう
にー兄さんenum OldStyle {
OLD_STYLE_ONE,
OLD_STYLE_TWO
};
enum class NewStyle {
ONE,
TWO
};
EMSCRIPTEN_BINDINGS(my_enum_example) {
enum_<OldStyle>("OldStyle")
.value("ONE", OLD_STYLE_ONE)
.value("TWO", OLD_STYLE_TWO)
;
enum_<NewStyle>("NewStyle")
.value("ONE", NewStyle::ONE)
.value("TWO", NewStyle::TWO)
;
}
Module.OldStyle.ONE;
Module.NewStyle.TWO;
ChatGPTとかべうちしていたら、名案が浮かんだかもしれない
にー兄さんまずはCoordinateSystemのキーのUnionを作る
これがこのAPIの引数で受け取るオプションにする
例えばspzUnpackOptionsと仮に（いったん。おかしいけど）方をAとすると、こんなことができる
const wasmModule = await MainModuleFactory();
const aaa = wasmModule.CoordinateSystem[spzUnpackOptions]
ここでaaaは{value:number}っぽい値になる
にー兄さんレビューの結果、4つのファイルに提案がある感じになった
にー兄さんspzApi.tsは、unpackSpzOptionもILoadSpzOptionsに入れない？というのと
ユーザ的にはstringのUnnionのほうが嬉しいのでは？という提案（メンテ性も上がる）
にー兄さんこれでビルドなどはできたので、あとはコメントしよう
にー兄さん今回のレビューの観点は下記
- CoordinateSystemオブジェクトを宣言するのではなく、CoordinateSystemUnionを定義してそれを使ってwasmModuleからCoordinateSystemオブジェクトに対応する値を取得しよう
- もしCoordinateSystemに変更があった場合にも追従しやすい
- コードの可読性
- ユーザフレンドリー
- UnpackOptionをユーザ触るAPIに持たせるのではなく、ILoadSpzOptionsにunpackOption相当のオプションを持たせよう
- 関数のシグネチャの変更が最小限で済む
- babylon/playcanvasに対しても同様にUnpackOptionsを既存のオプションに含めよう
- lint/formatを適用しよう
にー兄さんん？となったコメントが来たんだけど、これは自分の伝えミスだなというのがわかった
The issue here is that wasmModule is loaded async at runtime which means we can't create the values at compile time. As far as I know, that would mean having a runtime shim to build the values from the type after the module is loaded. Any external modules would then have to collect the values at runtime through some interface.
However, I have added a new CoordinateSystemEnum type external modules can import. With this and the CoordinateSystem values external modules can specify coordinate systems cleanly in a strongly typed manner. Plus it keeps all wasm internals hidden. I did this after removing the UnpackOptions I needed a clean way to add a CoordinateSystem field to the ICreateGSFromSpzOptions interface for Babylon.
https://github.com/drumath2237/spz-loader/pull/71#discussion_r2237574463
というのも、どうやら自分が提示したコード例では、loadSpz関数の中ではwasmModuleを使っているので、それを使ってCoordinateSystemを取得すればいいよねという話だったんだけど
loadSpzの中で使うという意図がうまく伝わっていなかったんだと思う
にー兄さんHi @drumath2237! Thank you for this great library and taking a look at my colleague's PR. Once your concerns are resolved, do you have an idea when you might be able to get this merged and released to npm?
We are looking to do a release this week and need to include an update to SPZ within it. We have a "plan b" but would like to stick with your npm package if possible.
https://github.com/drumath2237/spz-loader/pull/71#issuecomment-3133610627
なるほどね、少し無理をするか…
にー兄さんあれ、前は失敗していたactionsでのdockerを使ったビルドが成功するようになっている
はて？ これはdockerのバージョン指定をさぼったことによるものか…？（いい方向に倒れたけど
上手くいかなかった時のメモはこれ
にー兄さん何とか対応が終わり、マージ
やったね
にー兄さんそして問題なく0.3.0をリリース完了