glTFへの属性情報付与 #268
glTFへの属性情報付与 #268
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 25
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
demo/cesium/examples/ext_mesh_features/tileset_feature_id_attribute.jsonis excluded by:!**/*.jsondemo/cesium/examples/ext_structural_metadata/tileset.jsonis excluded by:!**/*.json
Files selected for processing (18)
- demo/cesium/examples/ext_mesh_features/test.gltf (1 hunks)
- demo/cesium/examples/ext_structural_metadata/test.gltf (1 hunks)
- demo/cesium/gltf_ext_structural_metadata.html (2 hunks)
- nusamai-gltf/nusamai-gltf-json/src/models/accessor.rs (6 hunks)
- nusamai-gltf/nusamai-gltf-json/src/models/buffer.rs (1 hunks)
- nusamai-gltf/nusamai-gltf-json/src/models/extensions/gltf/ext_structural_metadata.rs (7 hunks)
- nusamai-gltf/nusamai-gltf-json/src/models/extensions/gltf/mod.rs (1 hunks)
- nusamai-gltf/nusamai-gltf-json/src/models/extensions/mesh/ext_mesh_features.rs (3 hunks)
- nusamai-gltf/nusamai-gltf-json/src/models/extensions/mesh/ext_structural_metadata.rs (1 hunks)
- nusamai-gltf/nusamai-gltf-json/src/models/extensions/mesh/khr_materials_variants.rs (1 hunks)
- nusamai-gltf/nusamai-gltf-json/src/models/extensions/mesh/mod.rs (1 hunks)
- nusamai-gltf/nusamai-gltf-json/src/models/mesh.rs (3 hunks)
- nusamai/src/main.rs (3 hunks)
- nusamai/src/sink/gltf/attributes.rs (1 hunks)
- nusamai/src/sink/gltf/gltf_writer.rs (1 hunks)
- nusamai/src/sink/gltf/mod.rs (15 hunks)
- nusamai/src/sink/gltf/positions.rs (1 hunks)
- nusamai/src/sink/mod.rs (1 hunks)
Additional comments: 47
nusamai/src/sink/gltf/positions.rs (1)
- 1-6: この変更は問題ありません。
nusamai-gltf/nusamai-gltf-json/src/models/extensions/mesh/khr_materials_variants.rs (1)
- 6-6: この変更は問題ありません。
nusamai-gltf/nusamai-gltf-json/src/models/extensions/gltf/mod.rs (1)
- 15-15: この変更は問題ありませんが、
othersフィールドの外部からの操作には注意が必要です。nusamai-gltf/nusamai-gltf-json/src/models/extensions/mesh/mod.rs (1)
- 10-10: この変更は問題ありません。
nusamai/src/sink/mod.rs (1)
- 7-7: この変更は問題ありません。
nusamai-gltf/nusamai-gltf-json/src/models/extensions/mesh/ext_structural_metadata.rs (1)
- 10-10: この変更は問題ありません。
demo/cesium/examples/ext_mesh_features/test.gltf (1)
- 1-87: この変更は問題ありません。
demo/cesium/gltf_ext_structural_metadata.html (2)
- 41-47: この変更は問題ありません。
- 66-77: この変更は問題ありません。
nusamai-gltf/nusamai-gltf-json/src/models/buffer.rs (1)
- 49-49: この変更は問題ありません。
nusamai-gltf/nusamai-gltf-json/src/models/extensions/mesh/ext_mesh_features.rs (3)
- 10-10: この変更は問題ありません。
- 27-27: この変更は問題ありません。
- 64-64: この変更は問題ありません。
nusamai-gltf/nusamai-gltf-json/src/models/mesh.rs (3)
- 22-22: この変更は問題ありません。
- 55-55: この変更は問題ありません。
- 79-79: この変更は問題ありません。
nusamai/src/main.rs (3)
- 9-9: この変更は問題ありません。
- 65-66: この変更は問題ありません。
- 82-83: この変更は問題ありません。
nusamai-gltf/nusamai-gltf-json/src/models/accessor.rs (6)
- 30-30: この変更は問題ありません。
- 46-46: この変更は問題ありません。
- 59-59: この変更は問題ありません。
- 73-73: この変更は問題ありません。
- 87-87: この変更は問題ありません。
- 142-142: この変更は問題ありません。
nusamai-gltf/nusamai-gltf-json/src/models/extensions/gltf/ext_structural_metadata.rs (7)
- 72-72: Class構造体にCloneトレイトを追加しました。この変更は適切です。
- 97-97: ClassPropertyType列挙型にCloneトレイトを追加しました。この変更は適切です。
- 114-114: ClassPropertyComponentType列挙型にCloneトレイトを追加しました。この変更は適切です。
- 130-130: ClassProperty構造体にCloneトレイトを追加しました。この変更は適切です。
- 276-276: PropertyTable構造体にCloneトレイトを追加しました。この変更は適切です。
- 393-393: OffsetType列挙型にCloneトレイトを追加しました。この変更は適切です。
- 404-404: PropertyTableProperty構造体にCloneトレイトを追加しました。この変更は適切です。
nusamai/src/sink/gltf/mod.rs (7)
- 2-4: 新しいサブモジュール(
attributes,gltf_writer,positions)が導入されています。これらのモジュールが正しく実装されているか、関連する機能が適切に分割されているかを確認してください。- 38-42:
TriangulatedEntity構造体は、三角形化されたエンティティの位置と属性を保持します。この構造体の設計が、使用されるコンテキストに適しているか検討してください。- 44-47:
Buffers構造体は、頂点とインデックスのバッファを保持します。IndexSetを使用して頂点の重複を避ける設計は効率的ですが、パフォーマンスへの影響を検討してください。- 243-280: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [232-313]
glTFファイルへの書き込み処理が実装されています。この処理の効率性と、生成されるglTFファイルの正確性を検証してください。また、
bounding_volumeの計算方法についても、正確性を確認してください。
- 342-378: クラス名ごとにエンティティの
feature_idを再割り当てする処理があります。この処理の必要性と実装の効率性を検討してください。- 383-391: glTFとバイナリコンテンツの構築、および拡張機能の追加に関する処理があります。この部分の実装が、仕様に準拠しているか確認してください。
- 393-393: 3DTilesの書き込み処理が実装されています。この処理が正しく機能し、期待される出力を生成しているか検証してください。
demo/cesium/examples/ext_structural_metadata/test.gltf (8)
- 2-5:
EXT_mesh_featuresおよびEXT_structural_metadata拡張機能が使用されています。これらの拡張機能が正しく使用されているか、関連する仕様に準拠しているかを確認してください。- 6-34: アクセサの定義が追加されています。これらのアクセサが適切に定義されており、関連するバッファビューと整合性が取れているか確認してください。
- 36-39:
assetオブジェクトにgeneratorとversionが設定されています。これがglTF 2.0の仕様に準拠しているか確認してください。- 40-402: バッファビューの定義が複数追加されています。各バッファビューが正しく定義されており、適切なバイトオフセットとバイト長を持っているか検証してください。
- 404-407:
buffersオブジェクトが定義されています。全体のバイト長が正確に計算されているか確認してください。- 409-431:
meshesオブジェクトにプリミティブが定義されています。POSITIONと_FEATURE_ID_0属性が正しく設定されているか、またEXT_mesh_features拡張機能が適切に使用されているか検証してください。- 433-441:
nodesオブジェクトが定義されています。メッシュの参照と変換が正しく設定されているか確認してください。- 450-767:
EXT_structural_metadata拡張機能の詳細な定義があります。この拡張機能の使用が仕様に準拠しており、属性情報が正確に表現されているか検証してください。
| component_type: None, | ||
| ..Default::default() | ||
| }, | ||
| TypeRef::Unknown => todo!(), |
There was a problem hiding this comment.
TypeRef::Unknownのケースでtodo!()を使用していますが、これはpanicを引き起こします。プロダクションコードでpanicを引き起こす可能性があるコードは避けるべきです。適切なエラーハンドリングを実装してください。
| // todo: feature 以外の型も実装する | ||
| TypeDef::Data(_) => unimplemented!(), | ||
| TypeDef::Property(_) => unimplemented!(), | ||
| } |
There was a problem hiding this comment.
TypeDef::DataとTypeDef::Propertyのケースでunimplemented!()を使用しています。これもpanicを引き起こすため、適切なエラーハンドリングまたは実装を提供する必要があります。
| // todo: feature 以外の型も実装する | ||
| TypeDef::Data(_) => unimplemented!(), | ||
| TypeDef::Property(_) => unimplemented!(), | ||
| } |
There was a problem hiding this comment.
TypeDef::DataとTypeDef::Propertyのケースでunimplemented!()を使用しています。これもpanicを引き起こすため、適切なエラーハンドリングまたは実装を提供する必要があります。
| ); | ||
| buffer_view_length += 1; | ||
| } | ||
| _ => unimplemented!(), |
There was a problem hiding this comment.
未実装のケースに対してunimplemented!()を使用しています。panicを引き起こす可能性があるため、適切なエラーハンドリングまたは実装を提供する必要があります。
| let mut vertices_count = 0; | ||
| for vertex in vertices.clone() { | ||
| LittleEndian::write_u32_into(&vertex.position, &mut buf); | ||
| bin_content.write_all(&buf).unwrap(); |
There was a problem hiding this comment.
unwrap()を使用することは、エラーが発生した場合にパニックを引き起こす可能性があります。エラーハンドリングを改善するために、?演算子またはmatch文を使用してください。
| writer.write_all(b"glTF").unwrap(); | ||
| // magic | ||
| writer.write_all(&2u32.to_le_bytes()).unwrap(); | ||
| // version: 2 | ||
| writer | ||
| .write_all(&(total_size as u32).to_le_bytes()) | ||
| .unwrap(); | ||
| // total size | ||
|
|
||
| writer | ||
| .write_all(&(json_content.len() as u32).to_le_bytes()) | ||
| .unwrap(); | ||
| // json content | ||
| writer.write_all(b"JSON").unwrap(); | ||
| // chunk type | ||
| writer.write_all(&json_content).unwrap(); | ||
| // json content | ||
|
|
||
| writer | ||
| .write_all(&(bin_content.len() as u32).to_le_bytes()) | ||
| .unwrap(); | ||
| // json content | ||
| writer.write_all(b"BIN\0").unwrap(); | ||
| // chunk type | ||
| writer.write_all(&bin_content).unwrap(); | ||
| // bin content | ||
|
|
||
| writer.flush().unwrap(); |
There was a problem hiding this comment.
write_all()メソッドの使用において、unwrap()を使用することは、エラーが発生した場合にパニックを引き起こす可能性があります。エラーハンドリングを改善するために、?演算子またはmatch文を使用してください。
| ..Default::default() | ||
| }; | ||
|
|
||
| let mut tileset_file = File::create(tileset_path).unwrap(); |
There was a problem hiding this comment.
File::create()の呼び出しでunwrap()を使用することは、ファイル作成時にエラーが発生した場合にパニックを引き起こす可能性があります。エラーハンドリングを改善するために、?演算子またはmatch文を使用してください。
|
|
||
| let mut tileset_file = File::create(tileset_path).unwrap(); | ||
| let tileset_writer = BufWriter::with_capacity(1024 * 1024, &mut tileset_file); | ||
| serde_json::to_writer_pretty(tileset_writer, &tileset).unwrap(); |
There was a problem hiding this comment.
serde_json::to_writer_pretty()の呼び出しでunwrap()を使用することは、シリアライズ時にエラーが発生した場合にパニックを引き起こす可能性があります。エラーハンドリングを改善するために、?演算子またはmatch文を使用してください。
nusamai/src/sink/gltf/mod.rs
Outdated
| // todo: Exception handling | ||
| let Value::Object(obj) = &entity.root else { | ||
| unimplemented!() | ||
| }; | ||
| // todo: Support for other stereotypes |
There was a problem hiding this comment.
例外処理のためにunimplemented!()が使用されていますが、これは実際の運用環境では適切ではありません。より具体的なエラーハンドリングを実装してください。
- unimplemented!()
+ return Err(PipelineError::UnsupportedGeometryType);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // todo: Exception handling | |
| let Value::Object(obj) = &entity.root else { | |
| unimplemented!() | |
| }; | |
| // todo: Support for other stereotypes | |
| // todo: Exception handling | |
| let Value::Object(obj) = &entity.root else { | |
| return Err(PipelineError::UnsupportedGeometryType) | |
| }; | |
| // todo: Support for other stereotypes |
nusamai/src/sink/gltf/mod.rs
Outdated
| max_height: f64::MIN, | ||
| }; | ||
|
|
||
| // todo: transformerからschemaを受け取る必要がある | ||
|
|
||
| let entity = parcel.entity; | ||
| let geom_store = entity.geometry_store.read().unwrap(); | ||
|
|
||
| // todo: Exception handling | ||
| let Value::Object(obj) = &entity.root else { | ||
| unimplemented!() | ||
| }; | ||
| // todo: Support for other stereotypes | ||
| let ObjectStereotype::Feature { id: _, geometries } = &obj.stereotype | ||
| else { | ||
| unimplemented!() | ||
| }; | ||
|
|
||
| let typename = obj.typename.clone(); | ||
|
|
||
| // Divide polygons into triangles | ||
| let mut earcutter = Earcut::new(); | ||
| let mut buf3d: Vec<f64> = Vec::new(); |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [137-219]
多角形を三角形に分割するロジックが実装されています。この処理のパフォーマンスと正確性を確認し、必要に応じて最適化を検討してください。また、GeometryType::CurveやGeometryType::Pointに対するサポートが未実装である点に注意してください。
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- nusamai/src/sink/gltf/mod.rs (15 hunks)
- nusamai/src/sink/mod.rs (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- nusamai/src/sink/gltf/mod.rs
- nusamai/src/sink/mod.rs
大きいPRですみません！
これでも出来ていないことが多いんですが、流石に大きすぎる気がするので、一旦ここで切らせてください…！
やったこと
出来ていないこと
追記
その後の調査により、以下のことが分かったため、「属性情報がうまく表示されない」については別な解決策（テーブルごとにtileset.json・gltfを作成するなど）が必要になるかもしれない
確認方法
cd demo/cesium/ python -m http.serverhttp://localhost:8000/gltf_ext_structural_metadata.htmlを確認Summary by CodeRabbit
新機能
EXT_mesh_features拡張機能をサポートするための新しいファイルtest.gltfを追加しました。バグ修正
viewerの初期化を更新し、tileset.modelMatrixとviewer.zoomToの設定をカメラコントローラーのプロパティの設定と地形の深さテストの有効化に置き換えました。リファクタ
Cloneトレイトを追加し、これらのタイプのインスタンスを複製できるようにしました。Gltf構造体内のothersフィールドの可視性をpubに変更し、モジュール外からアクセス可能にしました。nusamai/src/sink/gltf/mod.rs内のモジュールを再構成し、新しいサブモジュールを導入し、コードを再編成しました。スタイル
GltfPocからGltfへ）。