feat(engine): introduce GLTF module and refactor related components #654
Conversation
WalkthroughThe pull request introduces several changes across multiple files in a Rust project, primarily focusing on the addition of a new dependency for GLTF handling and restructuring existing modules and error handling. A new Changes
Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
✅ Deploy Preview for reearth-flow canceled.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (15)
engine/runtime/gltf/src/errors.rs (2)
1-9: Consider enhancing error context and conversion traits.The error enum is well-structured with good use of thiserror. However, consider these improvements:
- Add more context to error messages (e.g., file paths, operation types)
- Implement
Fromtraits for common error types that might need conversionExample implementation:
use std::io; use std::path::PathBuf; #[derive(Error, Debug)] pub enum Error { #[error("GLTF metadata error for '{path}': {message}")] Metadata { message: String, path: PathBuf, }, #[error("GLTF writer error: {message}")] Writer { message: String, #[source] source: Option<io::Error>, }, } impl From<io::Error> for Error { fn from(err: io::Error) -> Self { Error::Writer { message: err.to_string(), source: Some(err), } } }
13-21: Consider refining error construction methods.While the helper methods are useful, consider these improvements:
- Use
impl AsRef<str>instead ofToStringfor better semantics- Add structured constructors for common error cases
Example implementation:
impl Error { pub fn metadata<S: AsRef<str>>(message: S) -> Self { Self::Metadata(message.as_ref().to_owned()) } pub fn writer<S: AsRef<str>>(message: S) -> Self { Self::Writer(message.as_ref().to_owned()) } // Additional helpers for common cases pub fn invalid_format<S: AsRef<str>>(detail: S) -> Self { Self::metadata(format!("Invalid GLTF format: {}", detail.as_ref())) } pub fn io_error(err: std::io::Error) -> Self { Self::writer(format!("IO error: {}", err)) } }engine/runtime/examples/plateau/testdata/workflow/data-convert/08-ubld/workflow.yml (1)
Line range hint
1-91: Verify workflow compatibility with the new GLTF moduleThe workflow processes CityGML data into Cesium3DTiles with texture attachment enabled. Given the introduction of the new GLTF module and removal of the old GLTF functionality (as mentioned in the AI summary), please ensure:
- The Cesium3DTilesWriter action is updated to use the new GLTF module
- Texture attachment functionality is properly maintained
- The vertical reprojection step remains compatible with the new GLTF processing pipeline
Consider documenting the interaction between this workflow and the new GLTF module in the workflow schema or accompanying documentation.
engine/Cargo.toml (1)
Line range hint
1-215: Consider improving dependency management.The workspace configuration could benefit from the following improvements:
- Consider organizing related dependencies into feature flags to reduce the default build size
- Automate the rust-version synchronization between
Cargo.tomlandclippy.toml- Consider using a dependency management tool to keep dependencies up to date
engine/runtime/types/src/material.rs (2)
9-15: Enhance documentation for X3DMaterial structWhile the struct is marked as CityGML's X3DMaterial, the documentation could be more comprehensive. Consider adding:
- Purpose and usage context
- Description of each field and their valid ranges
- Relationship with CityGML specification
Example documentation:
/// Represents a X3DMaterial from CityGML specification, defining surface appearance properties. /// /// # Fields /// * `diffuse_color` - The diffuse color component, affecting the main surface color /// * `specular_color` - The specular color component, affecting highlights /// * `ambient_intensity` - The ambient light intensity factor (0.0 to 1.0)
145-151: Consider adding URL and format validationThe current implementation directly maps the image URL without validation. Consider:
- Validating URL format
- Checking for supported image formats (png, jpg, webp, etc.) based on the formats handled in the
load_imagefunctionExample enhancement:
impl From<nusamai_plateau::appearance::Texture> for Texture { fn from(texture: nusamai_plateau::appearance::Texture) -> Self { let url = texture.image_url; // Validate file extension if let Some(ext) = Path::new(url.path()) .extension() .and_then(|ext| ext.to_str()) .map(|ext| ext.to_lowercase()) { match ext.as_str() { "png" | "jpg" | "jpeg" | "webp" | "tif" | "tiff" => (), _ => log::warn!("Unsupported image format: {}", ext), } } Self { uri: url } } }engine/runtime/gltf/src/writer.rs (5)
Line range hint
21-26: Consider using a builder pattern for better parameter management.The function has many parameters which could make it harder to maintain and use. Consider introducing a builder pattern or configuration struct to improve ergonomics.
Example refactor:
pub struct GltfWriterConfig<'a> { translation: [f64; 3], vertices: &'a dyn IntoIterator<Item = [u32; 9]>, primitives: Primitives, num_features: usize, metadata_encoder: MetadataEncoder, } impl<'a> GltfWriterConfig<'a> { pub fn new(vertices: &'a dyn IntoIterator<Item = [u32; 9]>) -> Self { Self { translation: [0.0; 3], vertices, primitives: Default::default(), num_features: 0, metadata_encoder: Default::default(), } } pub fn with_translation(mut self, translation: [f64; 3]) -> Self { self.translation = translation; self } // ... other builder methods } pub fn write_gltf_glb<W: Write>( writer: W, config: GltfWriterConfig, ) -> crate::errors::Result<()>
Line range hint
28-123: Optimize vertex buffer processing for better performance.The vertex processing could be optimized in several ways:
- Pre-allocate the buffer based on vertex count
- Batch write operations to reduce system calls
Example optimization:
// The buffer for the BIN part - let mut bin_content: Vec<u8> = Vec::new(); + let vertices_count = vertices.size_hint().0; + let mut bin_content: Vec<u8> = Vec::with_capacity( + vertices_count * VERTEX_BYTE_STRIDE + 1024 // Add padding for additional data + ); // ... rest of the code - for v in vertices { - // ... process vertex - bin_content - .write_all(&buf) - .map_err(crate::errors::Error::writer)?; - } + let mut batch = Vec::with_capacity(VERTEX_BYTE_STRIDE * 1000); // Process 1000 vertices at a time + for v in vertices { + // ... process vertex + batch.extend_from_slice(&buf); + if batch.len() >= batch.capacity() { + bin_content + .write_all(&batch) + .map_err(crate::errors::Error::writer)?; + batch.clear(); + } + } + if !batch.is_empty() { + bin_content + .write_all(&batch) + .map_err(crate::errors::Error::writer)?; + }
Line range hint
154-157: Address TODO comment regarding texture data optimization.The comment suggests an optimization for non-textured data. This could improve memory usage and performance.
Would you like me to help implement the optimization for excluding UV coordinates from non-textured vertices?
Line range hint
195-215: Consider parallel processing for image conversion.The image processing could benefit from parallel execution, especially when handling multiple images.
Example using rayon:
use rayon::prelude::*; let gltf_images: Vec<_> = image_set .into_par_iter() .map(|img| { img.to_gltf(&mut gltf_buffer_views, &mut bin_content) .map_err(crate::errors::Error::writer) }) .collect::<Result<Vec<Image>, crate::errors::Error>>()?;Note: This would require thread-safe access to buffer_views and bin_content.
Line range hint
217-291: Add validation for GLTF structure before writing.Consider adding validation checks for the GLTF structure before writing to ensure compliance with the GLTF specification.
Example validation:
fn validate_gltf(gltf: &Gltf) -> crate::errors::Result<()> { // Validate scene references if let Some(nodes) = &gltf.scenes[0].nodes { if nodes.iter().any(|&node_idx| node_idx >= gltf.nodes.len() as u32) { return Err(crate::errors::Error::validation("Invalid node reference")); } } // Validate mesh references for node in &gltf.nodes { if let Some(mesh_idx) = node.mesh { if mesh_idx >= gltf.meshes.len() as u32 { return Err(crate::errors::Error::validation("Invalid mesh reference")); } } } // Add more validation as needed Ok(()) }engine/runtime/gltf/src/metadata.rs (2)
49-51: Improved error handling looks good, consider enhancing the error message.The switch to using custom error types from
crate::errorsis a good improvement over the previous unit error type.Consider making the error message more specific by including the typename that wasn't found:
- return Err(crate::errors::Error::metadata("Feature type not found")); + return Err(crate::errors::Error::metadata(&format!("Feature type '{}' not found", typename)));
Line range hint
14-21: Consider architectural improvements for better maintainability.A few suggestions to enhance the design:
- Move the no-data constants to a configuration file or make them configurable at runtime
- Complete the Boolean type support (currently commented out with TODO)
- Consider using a trait-based system for type handling to make it more extensible
Would you like me to provide more detailed suggestions for any of these improvements?
engine/runtime/action-sink/src/file/cesium3dtiles/slice.rs (2)
Line range hint
193-201: Consider implementing missing geometry typesThe code has unimplemented handlers for
CurveandPointgeometry types. This could limit the functionality of the GLTF module.Would you like me to help implement these geometry types or create GitHub issues to track them?
Line range hint
209-365: Consider adding documentation for the slicing algorithmThe polygon slicing implementation is complex and would benefit from additional documentation explaining:
- The mathematical principles behind the slicing algorithm
- The coordinate system transformations
- The UV mapping preservation logic
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
engine/Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (18)
engine/Cargo.toml(1 hunks)engine/runtime/action-sink/Cargo.toml(1 hunks)engine/runtime/action-sink/src/file/cesium3dtiles.rs(0 hunks)engine/runtime/action-sink/src/file/cesium3dtiles/sink.rs(3 hunks)engine/runtime/action-sink/src/file/cesium3dtiles/slice.rs(3 hunks)engine/runtime/action-wasm-processor/Cargo.toml(0 hunks)engine/runtime/common/src/lib.rs(0 hunks)engine/runtime/examples/plateau/example_feature_transformer.rs(1 hunks)engine/runtime/examples/plateau/testdata/workflow/data-convert/08-ubld/workflow.yml(2 hunks)engine/runtime/gltf/Cargo.toml(1 hunks)engine/runtime/gltf/src/errors.rs(1 hunks)engine/runtime/gltf/src/lib.rs(1 hunks)engine/runtime/gltf/src/metadata.rs(2 hunks)engine/runtime/gltf/src/writer.rs(6 hunks)engine/runtime/types/Cargo.toml(1 hunks)engine/runtime/types/src/geometry.rs(4 hunks)engine/runtime/types/src/lib.rs(1 hunks)engine/runtime/types/src/material.rs(3 hunks)
💤 Files with no reviewable changes (3)
- engine/runtime/action-sink/src/file/cesium3dtiles.rs
- engine/runtime/action-wasm-processor/Cargo.toml
- engine/runtime/common/src/lib.rs
✅ Files skipped from review due to trivial changes (3)
- engine/runtime/examples/plateau/example_feature_transformer.rs
- engine/runtime/gltf/Cargo.toml
- engine/runtime/gltf/src/lib.rs
🔇 Additional comments (21)
engine/runtime/types/src/lib.rs (1)
10-10: Verify integration with GLTF functionality
The addition of the material module is part of GLTF integration. Ensure that all dependent code is updated to use this new module appropriately.
✅ Verification successful
Based on the results, I can generate the final response as we have sufficient information about the material module's integration.
Material module is properly integrated with GLTF functionality
The material module is already well-integrated across the codebase:
- Used in
engine/runtime/gltf/src/writer.rsfor GLTF primitives - Properly implemented in
engine/runtime/types/src/material.rswith GLTF conversion support - Integrated with geometry types in
engine/runtime/types/src/geometry.rs - Utilized in Cesium 3D Tiles implementation in
engine/runtime/action-sink/src/file/cesium3dtiles/
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential breaking changes and verify GLTF integration
# Find files that might need updates for the new material module
rg -t rust "(X3DMaterial|Material)"
# Check for any TODO or FIXME comments related to material handling
rg -t rust "TODO.*material|FIXME.*material"
# Look for GLTF-related code that might need to use this module
ast-grep --pattern 'use $$$gltf$$$'
Length of output: 2509
engine/runtime/gltf/src/errors.rs (1)
11-11: LGTM! Well-defined Result type alias.
The Result type alias follows Rust conventions and provides good ergonomics for error handling throughout the codebase.
engine/runtime/types/Cargo.toml (1)
20-20: LGTM! Verify dependency impact.
The addition of nusamai-gltf follows the established workspace dependency pattern and aligns with the PR's objective of introducing GLTF support.
Let's verify the dependency relationships to ensure no circular dependencies are introduced:
✅ Verification successful
Dependency structure verified and safe
The dependency structure is well-organized and follows a clean pattern:
reearth-flow-typesis a base package that other modules depend onreearth-flow-gltfdepends onreearth-flow-types(along with other core modules), which is the correct direction- All dependencies use workspace inheritance consistently
- The
nusamai-gltfdependency is properly versioned and managed at the workspace level
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential circular dependencies and verify dependency tree
# Look for any references to reearth-flow-types in the new gltf module
# Check if gltf module depends back on types
fd -e toml Cargo.toml -x grep -l "reearth-flow-types"
# Check the full dependency tree for the types package
cargo tree -p reearth-flow-types 2>/dev/null || echo "Note: cargo command not available in sandbox"
Length of output: 755
Script:
#!/bin/bash
# Let's check the dependency relationship in gltf module's Cargo.toml
cat engine/runtime/gltf/Cargo.toml
# Also check the workspace root Cargo.toml to understand the dependency structure
cat engine/Cargo.toml
Length of output: 7181
engine/runtime/action-sink/Cargo.toml (1)
18-18: Verify GLTF dependency overlap
The dependency addition follows good practices by using workspace inheritance. However, please verify the relationship between reearth-flow-gltf and the existing nusamai-gltf dependency to ensure there's no unintended functionality overlap.
Let's check the dependencies and their usage:
✅ Verification successful
Dependencies serve different purposes - no overlap concern
The verification shows that nusamai-gltf and reearth-flow-gltf serve different purposes and are used correctly:
nusamai-gltfis used for low-level GLTF JSON schema types and serializationreearth-flow-gltfprovides higher-level functionality like normal calculation and metadata encoding, building upon the types fromnusamai-gltf
The dependencies are complementary rather than overlapping, following a good separation of concerns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential GLTF dependency overlap
# Check if nusamai-gltf is still being used
rg -l "use.*nusamai_gltf"
# Check new reearth-flow-gltf usage
rg -l "use.*reearth_flow_gltf"
# Look for any GLTF-related types or functions
ast-grep --pattern 'struct $name {
$$$
}'
ast-grep --pattern 'fn $name($$$) {
$$$
}'
Length of output: 318
Script:
#!/bin/bash
# Let's examine the actual usage context of both GLTF dependencies
# Check nusamai-gltf usage context
rg "nusamai_gltf" -A 5 -B 5
# Check reearth-flow-gltf usage context
rg "reearth_flow_gltf" -A 5 -B 5
# Check Cargo.toml for both dependencies
cat engine/runtime/action-sink/Cargo.toml
Length of output: 16080
engine/runtime/examples/plateau/testdata/workflow/data-convert/08-ubld/workflow.yml (1)
22-24: Verify the new FolderAndfilePathReader subgraph implementation
The node has been renamed from "AttributeReader" to "FolderAndfilePathReader" with a new subGraphId. Since this is a crucial entry point for file processing, please ensure:
- The new implementation maintains compatibility with the GLTF processing pipeline
- The subgraph referenced by
c6863b71-953b-4d15-af56-396fc93fc617exists and is properly implemented
✅ Verification successful
The FolderAndfilePathReader subgraph implementation is verified and compatible
The new implementation exists and is properly referenced. The changes are valid because:
- The subgraph with ID
c6863b71-953b-4d15-af56-396fc93fc617is correctly implemented infolder_and_file_path_reader.yml - The new implementation provides enhanced functionality:
- Handles file path extraction with archive support
- Includes GML file filtering
- Adds UDX folder extraction
- Supports package filtering
- Implements file counting by UDX directories
- The output port 'default' is maintained, ensuring compatibility with existing workflows
- The implementation is referenced correctly in multiple workflows, including the main Cesium3DTiles workflow
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and implementation of the new subgraph
# Check if the referenced subgraph exists in the included graphs
rg -l "id: c6863b71-953b-4d15-af56-396fc93fc617" "engine/runtime/examples/plateau/testdata/workflow"
# Check the implementation details of folder_and_file_path_reader.yml
cat "engine/runtime/examples/plateau/testdata/workflow/graphs/folder_and_file_path_reader.yml"
Length of output: 316
Script:
#!/bin/bash
# Let's try to find the subgraph file with a broader search and different naming patterns
fd "folder.*reader.*yml" "engine/runtime/examples/plateau/testdata"
# Also search for any YAML files containing the specific UUID
fd -e yml -e yaml . "engine/runtime/examples/plateau/testdata" -x grep -l "c6863b71-953b-4d15-af56-396fc93fc617" {}
# Let's also check if there are any similar reader implementations
fd -e yml -e yaml . "engine/runtime/examples/plateau/testdata/workflow/graphs" 2>/dev/null
# Search for any documentation or schema that might help understand the expected location
fd "README|SCHEMA" "engine/runtime/examples/plateau"
Length of output: 1059
Script:
#!/bin/bash
# Let's examine the content of both the old and new implementation
echo "=== Old Implementation (attribute_reader.yml) ==="
cat "engine/runtime/examples/plateau/testdata/graphs/attribute_reader.yml"
echo -e "\n=== New Implementation (folder_and_file_path_reader.yml) ==="
cat "engine/runtime/examples/plateau/testdata/graphs/folder_and_file_path_reader.yml"
# Let's also check the schema to understand the expected structure
cat "engine/runtime/examples/plateau/testdata/workflow/cesium_3d_tiles.yml"
Length of output: 7769
engine/Cargo.toml (1)
57-57: LGTM! Verify integration with existing GLTF functionality.
The addition of reearth-flow-gltf follows the workspace's established patterns for runtime modules. However, since the project already uses nusamai-gltf, ensure there's no functionality overlap.
Let's verify the relationship between the new and existing GLTF modules:
✅ Verification successful
Based on the shell script results, I can now provide a final response as there's clear evidence about the relationship between the modules:
✓ No functionality overlap - modules serve different purposes
The new reearth-flow-gltf module actually depends on and builds upon nusamai-gltf, not replacing it:
nusamai-gltfprovides core GLTF JSON schema and utilitiesreearth-flow-gltfimplements higher-level GLTF writing capabilities specific to Re:Earth Flow:- Metadata encoding for feature attributes
- Specialized GLTF/GLB writing with structural metadata
- Geometry utilities for normal calculations and tile operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential overlap between reearth-flow-gltf and nusamai-gltf
# Search for GLTF-related imports and usage
echo "=== Searching for GLTF-related imports ==="
rg --type rust "use.*gltf"
# Look for any documentation explaining the relationship
echo -e "\n=== Searching for documentation about GLTF handling ==="
fd -e md -e txt . | xargs rg -i "gltf"
Length of output: 747
Script:
#!/bin/bash
# Let's check the implementation details of both GLTF modules
# Check the new reearth-flow-gltf module implementation
echo "=== reearth-flow-gltf module implementation ==="
fd -t f . "engine/runtime/gltf" -x cat {}
# Check where nusamai-gltf is defined and its usage
echo -e "\n=== nusamai-gltf crate details ==="
cat engine/Cargo.toml | grep -A 5 -B 5 "nusamai-gltf"
# Check for any GLTF-related structs and types
echo -e "\n=== GLTF-related type definitions ==="
ast-grep --pattern 'struct $name {
$$$
}' engine/runtime/gltf/src/*.rs
Length of output: 33711
engine/runtime/types/src/material.rs (2)
17-45: LGTM! Consistent implementations with proper default handling
The implementations are well-structured with consistent default values across From traits and Default implementation. The null handling in the first From implementation is particularly good.
62-69: LGTM! Sensible default values
The Default implementation uses appropriate values: white color with full opacity and no texture, which is a common convention in 3D graphics.
engine/runtime/gltf/src/writer.rs (2)
7-9: LGTM! Clean import organization.
The imports are well-structured and the module dependencies are clearly defined.
26-26: Verify error handling implementation.
The changes to error handling look consistent, but let's verify the implementation of the new error type.
Also applies to: 60-60, 137-137, 213-213, 215-215, 291-291
✅ Verification successful
Error handling implementation is correctly aligned with the error type definition
The error handling in writer.rs consistently uses Error::writer which matches the error type definition in errors.rs. The error type has a dedicated Writer variant that takes a string message, and all error mappings in the writer module properly use this variant through Error::writer. The implementation is sound and follows a consistent pattern throughout the file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify error type implementation and usage
# Check error type definition
echo "Checking error type definition..."
rg -A 10 "pub enum Error" engine/runtime/gltf/src/errors.rs
# Check error handling consistency
echo "Checking error handling patterns..."
rg "Error::writer" engine/runtime/gltf/src/
Length of output: 887
engine/runtime/action-sink/src/file/cesium3dtiles/slice.rs (3)
8-11: LGTM: Clean import restructuring
The reorganization of imports reflects good modular design, properly separating material-related types into their own module.
105-106: LGTM: Clean texture handling implementation
The texture handling has been simplified while maintaining the same functionality, using the new material module's Texture type.
46-46: Verify X3DMaterial compatibility across the codebase
The switch to X3DMaterial as the default material type might affect other components that interact with this module.
✅ Verification successful
Based on the search results, I can generate the final response as we have sufficient information about X3DMaterial usage across the codebase.
X3DMaterial usage is consistent across the codebase
The switch to X3DMaterial as the default material type is safe as it's properly integrated:
- It's the standard material type in the
typesmodule with proper serialization support - Has well-defined conversion implementations from other material types (
nusamai_plateau::models::appearance::X3DMaterialandnusamai_plateau::appearance::Material) - Used consistently in geometry handling with proper material collections and accessors
- Includes a sensible default implementation matching common material properties
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential incompatibilities with X3DMaterial usage
# Search for other material type usages
rg -A 3 "X3DMaterial"
# Search for potential conversion points between material types
ast-grep --pattern 'Material::from($_)'
Length of output: 3097
engine/runtime/types/src/geometry.rs (3)
1-2: Imports for Display and Hash traits are correctly added
The std::fmt::Display and std::hash::Hash traits are properly imported and used in the code, enhancing functionality for formatting and hashing.
18-18: Imports for Texture and X3DMaterial are appropriately added
The addition of use crate::material::{Texture, X3DMaterial}; ensures that the Texture and X3DMaterial types are available for use in this module.
94-94: Type change from Material to X3DMaterial in CityGmlGeometry
The materials field and related methods in CityGmlGeometry have been updated to use X3DMaterial instead of Material. This change propagates through:
- Line 94:
pub materials: Vec<X3DMaterial>, - Line 104: Constructor parameter
materials: Vec<X3DMaterial>, - Line 127: Method
pub fn materials(&self) -> &[X3DMaterial]
Ensure that all usages of CityGmlGeometry and its materials field or methods have been updated accordingly throughout the codebase to reflect this type change.
To assist with verification, consider running the following script:
Also applies to: 104-104, 127-127
✅ Verification successful
Based on the search results, I can see:
- The type change from
MaterialtoX3DMaterialis properly defined in the codebase - The only usage of
CityGmlGeometry::newis inengine/runtime/types/src/conversion/nusamai.rswhere it correctly uses the materials from an appearance object - There are two distinct material types in the codebase:
X3DMaterialinmaterial.rswhich is used for CityGML materialsMaterialinmaterial.rswhich is used for GLTF materials
Let me verify one more aspect to ensure there are no other usages:
Based on the comprehensive search results, I can now provide a final response:
Type change from Material to X3DMaterial is correctly implemented and used
The codebase shows consistent usage of X3DMaterial in CityGmlGeometry:
- The type is properly defined and used in the struct definition
- The only instantiation in
nusamai.rscorrectly usesX3DMaterial - All usages of the
materialsfield (incesium3dtilessink and slice) properly handle theX3DMaterialtype - The
Materialtype exists separately in the codebase but is used for GLTF materials, not CityGML materials
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of `CityGmlGeometry` and references to `materials` that may be affected by the type change.
# Find all instantiations of `CityGmlGeometry::new` and check for parameter types.
rg 'CityGmlGeometry::new\(' -A 5
# Find all accesses to the `materials` field or method in `CityGmlGeometry`.
rg 'CityGmlGeometry.*materials' -A 2
Length of output: 538
Script:
#!/bin/bash
# Let's search for both Material and X3DMaterial usages to understand the type change
rg "Material|X3DMaterial" -A 2
# Also search for any imports or type definitions related to these types
ast-grep --pattern 'use $$$Material$$$;'
# Search for struct definitions to understand the type hierarchy
ast-grep --pattern 'struct $_ {
$$$
materials: $_,
$$$
}'
Length of output: 141281
Script:
#!/bin/bash
# Search for any other potential usages of CityGmlGeometry and its materials
rg "CityGmlGeometry" -A 3 -B 3
# Also check for any direct material field access
rg "\.materials" -A 2
Length of output: 49526
engine/runtime/action-sink/src/file/cesium3dtiles/sink.rs (5)
33-33: Import 'calculate_normal' from the updated module
The calculate_normal function is now imported from reearth_flow_gltf, reflecting the restructuring of GLTF functionalities.
40-40: Update 'material' import to align with refactoring
Importing material from reearth_flow_types ensures consistency with the refactored components related to material handling.
435-435: Initialize 'primitives' using 'reearth_flow_gltf::Primitives'
The primitives variable is correctly initialized with reearth_flow_gltf::Primitives, aligning with the new module's structures.
437-437: Instantiate 'MetadataEncoder' with the new GLTF module
Creating metadata_encoder using reearth_flow_gltf::MetadataEncoder ensures compatibility with the updated GLTF metadata handling.
751-752: Ensure proper error mapping after writing GLTF GLB
After calling write_gltf_glb, the errors are mapped using map_err. Verify that all potential errors from write_gltf_glb are appropriately converted to SinkError to maintain consistent error handling.
| @@ -7,6 +7,7 @@ pub mod feature; | |||
| pub mod file; | |||
| pub mod geometry; | |||
| pub mod lod; | |||
| pub mod material; | |||
There was a problem hiding this comment.
💡 Codebase verification
Add re-export for the material module
The material module contains public types (X3DMaterial, Material, Texture, Image) and is already being used by other modules through direct imports. The codebase follows a consistent pattern where most modules are re-exported with pub use module::*, but several modules including material are not. Since the material types are part of the public API and used by other modules, it should follow the same re-export pattern as other actively used modules.
- Add
pub use material::*;toengine/runtime/types/src/lib.rs
🔗 Analysis chain
Consider adding material module re-exports for consistency
The new material module is declared but not re-exported with pub use material::*; like other modules in this file. Consider whether this is intentional or if it should follow the same pattern as other modules.
Let me gather more information about the module export patterns in this codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the material module contains types that should be re-exported
# and verify if other files are already importing from this module directly
# Check material module contents for public types
rg -t rust "pub (struct|enum|type|trait)" engine/runtime/types/src/material.rs
# Check current usage patterns
rg -t rust "use .+material::"
Length of output: 281
Script:
#!/bin/bash
# Check the re-export patterns in lib.rs
rg -t rust "^pub use" engine/runtime/types/src/lib.rs
# Check if there are other modules that are not re-exported
rg -t rust "^pub mod" engine/runtime/types/src/lib.rs
Length of output: 431
engine/runtime/examples/plateau/testdata/workflow/data-convert/08-ubld/workflow.yml
Show resolved
Hide resolved
Overview
What I've done
What I haven't done
How I tested
Screenshot
Which point I want you to review particularly
Memo
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Workflow Changes