feat(engine): add GltfWriter module and integrate into action sink #656
Conversation
WalkthroughThe pull request introduces several changes across multiple files, primarily focusing on the addition of a new dependency, 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: 7
🧹 Outside diff range and nitpick comments (24)
engine/runtime/gltf/src/lib.rs (2)
6-14: Add documentation and consider field visibilityThe struct looks well-organized, but could benefit from documentation explaining its purpose and the units/coordinate system used for each field (e.g., degrees for lat/lng, meters for height?). Also, consider whether all fields need to be public or if some operations could be encapsulated through methods.
#[derive(Debug, Clone)] +/// Represents the geographical bounds of a 3D volume using longitude, latitude, and height. pub struct BoundingVolume { + /// Minimum longitude in degrees pub min_lng: f64, + /// Maximum longitude in degrees pub max_lng: f64, + /// Minimum latitude in degrees pub min_lat: f64, + /// Maximum latitude in degrees pub max_lat: f64, + /// Minimum height in meters pub min_height: f64, + /// Maximum height in meters pub max_height: f64, }
27-38: Add documentation for Default implementationThe Default implementation is correct, using appropriate initial values that allow for proper expansion through the update method. Consider adding documentation explaining why these specific initial values were chosen.
+/// Provides a default instance with extremal values that can be updated to encompass +/// actual geographical bounds. Initial values are chosen to ensure the first update +/// will set the actual bounds correctly. impl Default for BoundingVolume { fn default() -> Self { Self { min_lng: f64::MAX, max_lng: f64::MIN, min_lat: f64::MAX, max_lat: f64::MIN, min_height: f64::MAX, max_height: f64::MIN, } } }engine/runtime/action-sink/src/errors.rs (3)
21-24: Consider using "GLTF" instead of "Gltf" in error variants and messages.The acronym "GLTF" is commonly written in uppercase. Consider updating the variant names and error messages for consistency with standard terminology.
- #[error("Gltf Writer Factory error: {0}")] - GltfWriterFactory(String), - #[error("Gltf Writer error: {0}")] - GltfWriter(String), + #[error("GLTF Writer Factory error: {0}")] + GLTFWriterFactory(String), + #[error("GLTF Writer error: {0}")] + GLTFWriter(String),
35-38: Implementation looks good, consider renaming for consistency.The helper method implementation follows the established pattern correctly. Consider renaming to
gltf_writertoglft_writerfor consistency with the suggested variant names.- pub fn gltf_writer<T: ToString>(message: T) -> Self { + pub fn glft_writer<T: ToString>(message: T) -> Self { Self::GltfWriter(message.to_string()) }
Line range hint
3-24: Consider adding documentation comments.To improve maintainability and help other developers understand the error cases better, consider adding documentation comments for the
SinkErrorenum and its variants. This would be particularly helpful for understanding when each error variant is used.Example:
/// Represents errors that can occur during sink operations #[derive(Error, Debug)] pub enum SinkError { /// Errors that occur during sink factory construction #[error("Build factory error: {0}")] BuildFactory(String), // ... other variants ... /// Errors that occur during GLTF writer factory operations #[error("GLTF Writer Factory error: {0}")] GLTFWriterFactory(String), /// Errors that occur during GLTF writing operations #[error("GLTF Writer error: {0}")] GLTFWriter(String), }engine/runtime/examples/plateau/testdata/workflow/data-convert/07-brid-tun-cons/workflow.yml (1)
110-112: Consider documenting the output path structure changeThe output path structure has been simplified from a LOD-specific pattern to a generic one. Please ensure this change is documented and that downstream processes are updated accordingly.
Would you like me to help create documentation for the new output path structure?
engine/Cargo.toml (1)
106-106: Consider consolidating linear algebra libraries.While the addition of
glamis appropriate for GLTF processing given its performance characteristics and GLTF ecosystem integration, the project now has multiple linear algebra libraries (glam,nalgebra,nalgebra-glm). This could lead to:
- Increased binary size
- Potential conversion overhead between different types
- Maintenance complexity
Consider:
- Documenting when to use each library to prevent confusion
- Long-term plan to consolidate to a single library if possible
- Adding type conversion utilities if interop between libraries is needed
engine/runtime/gltf/src/writer.rs (3)
Line range hint
147-149: Address TODO comment about texture coordinate optimization.The current implementation always includes texture coordinates in the vertex buffer. For meshes without textures, this wastes memory. Consider implementing the suggested optimization to exclude u,v coordinates when not needed.
Line range hint
39-39: Consider improving vertex stride calculation clarity.The vertex stride calculation uses magic numbers (
4 * 9). Consider introducing named constants to improve readability:+ const FLOAT32_SIZE: usize = 4; + const VERTEX_COMPONENTS: usize = 9; // position(3) + normal(3) + uv(2) + feature_id(1) - const VERTEX_BYTE_STRIDE: usize = 4 * 9; // 4-bytes (f32) x 9 + const VERTEX_BYTE_STRIDE: usize = FLOAT32_SIZE * VERTEX_COMPONENTS;
Line range hint
34-102: Consider memory optimization opportunities.Several opportunities for memory optimization:
- Pre-allocate vectors based on known vertex count
- Reuse temporary buffers where possible
Example optimization for the vertices section:
- let mut bin_content: Vec<u8> = Vec::new(); + let vertex_count = vertices.size_hint().0; + let mut bin_content: Vec<u8> = Vec::with_capacity(vertex_count * VERTEX_BYTE_STRIDE);engine/schema/actions.json (1)
1891-1914: Consider enhancing the parameter schema documentation.While the parameter schema is technically correct, it would be helpful to:
- Add descriptions for the
outputandattachTextureproperties- Document the expected format/path requirements for the
outputproperty- Explain the behavior when
attachTextureis true/false/nullApply this diff to enhance the schema documentation:
"properties": { "attachTexture": { + "description": "Whether to attach textures to the GLTF output. If null or false, textures will not be attached.", "type": [ "boolean", "null" ] }, "output": { + "description": "The output path where the GLTF file will be written. Should end with .gltf or .glb extension.", "$ref": "#/definitions/Expr" } },engine/runtime/action-sink/src/file/gltf.rs (6)
71-84: Handle missingwithparameter more gracefullyIn the
buildmethod, if thewithparameter is missing, the code returns an error. Consider providing a default value or a more user-friendly error message to guide users on how to provide the necessary parameters.
184-188: Clarify error message for unsupported geometry typesWhen a feature's geometry is not a CityGML geometry, the error message is generic. Enhance the error message to include the actual geometry type, aiding in debugging and user guidance.
309-321: Optimize transformation matrix calculationThe computation of
transform_matrixinvolves complex trigonometric functions and matrix operations. Consider simplifying the calculations or adding comments to explain the mathematical reasoning, improving code readability.
321-322: Remove unused inverse calculationThe inverse of
transform_matrixis computed but not utilized:let _ = transform_matrix.inverse();Consider removing this line to eliminate unnecessary computation.
Apply this diff:
-let _ = transform_matrix.inverse();
343-348: Enhance error handling when creating directoriesWhen creating the atlas directory, the error handling is minimal. Check for specific errors like permission issues or existing directories, and provide clear messages to help diagnose problems.
567-583: Improve vertex deduplication methodUsing floating-point bits for vertex deduplication may lead to precision issues. Consider using a spatial hash with a tolerance or quantizing coordinates to ensure reliable deduplication.
engine/runtime/action-sink/src/file/cesium3dtiles/sink.rs (7)
746-746: Ensure proper error handling forwrite_gltf_glbWhile the call to
reearth_flow_gltf::write_gltf_glbusesmap_errto handle errors, consider adding context to the error messages for easier debugging. This can be achieved by including additional information in the error conversion.Proposed change:
reearth_flow_gltf::write_gltf_glb( writer, Some(translation), vertices, primitives, features.len(), metadata_encoder, ) -.map_err(crate::errors::SinkError::cesium3dtiles_writer)?; +.map_err(|e| crate::errors::SinkError::Cesium3DTilesWriter(format!("write_gltf_glb failed: {}", e)))?;
Line range hint
798-798: Handle potential errors frompacked.exportThe call to
packed.exportdoes not handle potential errors, which could lead to silent failures. It's important to handle any errors returned by this function to ensure robustness.Apply this diff to add error handling:
packed.export( exporter, &atlas_path, &texture_cache, config.width, config.height, ); +// Handle potential error from packed.export +.map_err(crate::errors::SinkError::Cesium3DTilesWriter)?;
Line range hint
890-891: Avoid panics by handling Mutex lock errorsUsing
.unwrap()onMutex::lock()can cause a panic if the mutex is poisoned. To increase robustness, handle thelockresult properly.Proposed change:
let contents_lock = contents.lock().map_err(|e| { crate::errors::SinkError::Cesium3DTilesWriter(format!("Failed to acquire contents lock: {}", e)) })?; -contents_lock.push(content); +contents_lock.push(content);
Line range hint
878-880: Avoid variable shadowing ofoutput_pathThe variable
output_pathis being shadowed, which can lead to confusion and potential bugs. Consider renaming the inner variable to improve code clarity.Suggested change:
let storage = ctx .storage_resolver .resolve(output_path) .map_err(crate::errors::SinkError::Cesium3DTilesWriter)?; -let output_path = output_path.path().join(Path::new(&content_path)); +let content_output_path = output_path.path().join(Path::new(&content_path)); storage - .put_sync(Path::new(&output_path), bytes::Bytes::from(buffer)) + .put_sync(Path::new(&content_output_path), bytes::Bytes::from(buffer)) .map_err(crate::errors::SinkError::Cesium3DTilesWriter)?;
Line range hint
741-742: Consider checking for emptyfeaturesbefore proceedingCurrently,
features.len()is used without verifying if it's zero, which might lead to issues if no features are processed. Ensure that the code handles the case wherefeaturescould be empty.Suggested addition:
if features.is_empty() { // Handle empty features case return Ok(()); }
Line range hint
662-668: Avoid potential race conditions with shared mutable stateThe
texture_size_cacheandtexture_cacheare used across multiple threads without explicit synchronization. Ensure that these caches are thread-safe or properly synchronized to prevent race conditions.Consider using thread-safe data structures or adding synchronization mechanisms.
Line range hint
721-723: Avoid unwrapping results fromfindoperationUsing
unwrapon the result offindcan cause a panic if the value is not found. Replaceunwrapwith proper error handling.Apply this diff to handle the potential
Nonecase:let (u, v) = updated_vertices .iter() .find(|(x_, y_, z_, _, _)| { (*x_ - x).abs() < 1e-6 && (*y_ - y).abs() < 1e-6 && (*z_ - z).abs() < 1e-6 - }) - .map(|(_, _, _, u, v)| (*u, *v)) - .unwrap(); + }) + .ok_or_else(|| crate::errors::SinkError::Cesium3DTilesWriter("Vertex not found during UV mapping"))? + .map(|(_, _, _, u, v)| (*u, *v));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
engine/Cargo.lockis excluded by!**/*.lock,!**/*.lockengine/docs/mdbook/src/action.mdis excluded by!**/*.md
📒 Files selected for processing (11)
engine/Cargo.toml(1 hunks)engine/runtime/action-sink/Cargo.toml(1 hunks)engine/runtime/action-sink/src/errors.rs(2 hunks)engine/runtime/action-sink/src/file.rs(1 hunks)engine/runtime/action-sink/src/file/cesium3dtiles/sink.rs(1 hunks)engine/runtime/action-sink/src/file/gltf.rs(1 hunks)engine/runtime/action-sink/src/mapping.rs(2 hunks)engine/runtime/examples/plateau/testdata/workflow/data-convert/07-brid-tun-cons/workflow.yml(1 hunks)engine/runtime/gltf/src/lib.rs(1 hunks)engine/runtime/gltf/src/writer.rs(2 hunks)engine/schema/actions.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- engine/runtime/action-sink/src/file.rs
🔇 Additional comments (10)
engine/runtime/action-sink/src/mapping.rs (2)
10-10: LGTM! Import follows established patterns
The import is correctly placed within the file module group and follows the consistent naming pattern.
23-23: LGTM! Verify GltfWriterSinkFactory implementation
The factory registration follows the established pattern and properly integrates the new GLTF writer functionality.
Let's verify the factory implementation:
✅ Verification successful
GltfWriterSinkFactory implementation verified and complete
The implementation follows the expected pattern with all required traits and methods properly implemented:
- Implements
SinkFactorytrait with complete required methods:name()returns "GltfWriter"description()provides clear purposeparameter_schema()defines GltfWriterParam schemacategories()properly categorized as "File"get_input_ports()returns default portprepare()andbuild()methods handle initialization and construction
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify GltfWriterSinkFactory implementation matches the expected pattern
# Check if the factory implements required traits and methods
ast-grep --pattern 'impl SinkFactory for GltfWriterSinkFactory {
$$$
fn name(&self) -> &str {
$$$
}
$$$
}'
# Verify the default implementation
ast-grep --pattern 'impl Default for GltfWriterSinkFactory {
$$$
fn default() -> Self {
$$$
}
$$$
}'
Length of output: 5202
engine/runtime/gltf/src/lib.rs (1)
16-25: 🛠️ Refactor suggestion
Consider longitude wraparound and add method documentation
The update logic is correct for most cases, but might not handle longitude wraparound correctly (e.g., when a bounding box crosses the 180/-180 degree meridian). Also, please add documentation explaining the method's purpose and behavior.
impl BoundingVolume {
+ /// Updates the current bounding volume to include the extent of another bounding volume.
+ ///
+ /// # Arguments
+ /// * `other` - Another bounding volume to include in the current bounds
pub fn update(&mut self, other: &Self) {
- self.min_lng = self.min_lng.min(other.min_lng);
- self.max_lng = self.max_lng.max(other.max_lng);
+ // Handle longitude wraparound at 180/-180 degrees
+ if self.max_lng < self.min_lng || other.max_lng < other.min_lng {
+ // Box crosses the 180/-180 meridian
+ self.min_lng = self.min_lng.min(other.min_lng);
+ self.max_lng = self.max_lng.max(other.max_lng);
+ } else {
+ self.min_lng = self.min_lng.min(other.min_lng);
+ self.max_lng = self.max_lng.max(other.max_lng);
+ }
self.min_lat = self.min_lat.min(other.min_lat);
self.max_lat = self.max_lat.max(other.max_lat);
self.min_height = self.min_height.min(other.min_height);
self.max_height = self.max_height.max(other.max_height);
}
}Let's verify if there are any existing tests that cover the longitude wraparound case:
engine/runtime/action-sink/Cargo.toml (1)
46-46: LGTM! Verify glam version compatibility.
The addition of glam as a workspace dependency is appropriate for GLTF processing requirements.
Let's verify the glam version compatibility with GLTF processing:
✅ Verification successful
LGTM! The glam dependency is correctly used for GLTF processing
The verification confirms that:
- The workspace glam v0.29.2 is used appropriately in the GLTF processing code
- The glam types (Vec3, Mat4) are used in the action-sink/gltf.rs for 3D transformations
- The dependency is necessary for GLTF geometry processing and coordinate transformations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check glam version compatibility with other GLTF-related dependencies
# Check workspace glam version and other GLTF-related dependencies
rg -A 1 "glam.*=.*\".*\""
rg -A 1 "gltf.*=.*\".*\""
# Check if there are any examples or documentation showing glam usage with GLTF
fd -e rs -e md | xargs rg -l "(?i)(glam.*gltf|gltf.*glam)"
Length of output: 601
Script:
#!/bin/bash
# Let's check the actual GLTF-related code to understand the glam usage context
# First, find GLTF-related Rust files
fd -e rs | xargs rg -l -i "gltf"
# Then check for glam type usage in those files
fd -e rs | xargs rg -l "glam::(Vec|Mat|Quat)"
# Also check for any GLTF writer implementation
ast-grep --pattern 'struct GltfWriter {
$$$
}'
Length of output: 496
Script:
#!/bin/bash
# Let's check the specific GLTF writer implementation and its dependencies
cat engine/runtime/gltf/src/writer.rs
# Also check the action-sink GLTF implementation
cat engine/runtime/action-sink/src/file/gltf.rs
# And check the Cargo.toml of the GLTF module
cat engine/runtime/gltf/Cargo.toml
Length of output: 38703
engine/runtime/examples/plateau/testdata/workflow/data-convert/07-brid-tun-cons/workflow.yml (1)
110-110: Verify the attachTexture parameter behavior
The new attachTexture: true parameter has been added. Please ensure that:
- The texture files are available in the expected location
- The performance impact of texture attachment is acceptable for your use case
engine/runtime/gltf/src/writer.rs (2)
266-266: LGTM! Safe handling of optional translation.
The use of unwrap_or_default() is a safe and idiomatic way to handle the optional translation, defaulting to [0.0, 0.0, 0.0] when no translation is provided.
21-21: LGTM! Verify callers are updated for the new optional translation parameter.
The change to make translation optional improves flexibility. However, we should verify that all callers have been updated to handle the new signature.
✅ Verification successful
All callers have been updated correctly to handle the optional translation parameter
The verification shows that both callers of write_gltf_glb are properly handling the new optional translation parameter:
action-sink/src/file/gltf.rspassesNonewhen no translation is neededaction-sink/src/file/cesium3dtiles/sink.rspassesSome(translation)when translation is required
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all callers of write_gltf_glb to verify they've been updated
rg -A 3 "write_gltf_glb\(" --type rust
Length of output: 718
engine/schema/actions.json (2)
1887-1923: LGTM! The GltfWriter action is well-defined and consistent with the system's design.
The action definition follows the established patterns:
- Proper categorization under "File"
- Clear description
- Consistent port configuration
- Well-structured parameter schema with appropriate property definitions
1887-1923: The GltfWriter implementation is consistent with other file writers.
The action follows the same patterns as other file writers in the codebase (Cesium3DTilesWriter, FileWriter, GeoJsonWriter, MVTWriter):
- Consistent parameter schema structure
- Similar property types and definitions
- Proper categorization and port configuration
engine/runtime/action-sink/src/file/cesium3dtiles/sink.rs (1)
Line range hint 940-941: Verify the correctness of geometric_error value
The geometric_error is set to 1e+100, which is an extremely large value. Verify that this value is appropriate and does not negatively impact the tileset rendering or performance.
Run the following script to search for typical geometric_error values used elsewhere:
✅ Verification successful
Let me search for more context about geometric error usage in the codebase.
Based on the search results, I can now provide a conclusive response about the geometric error value:
The geometric error value of 1e+100 is correct and intentionally set for the root tileset
The codebase shows a well-defined geometric error calculation system where:
- The root tileset uses 1e+100 as confirmed by test cases (
test_geometric_errorfunction shows this value for zoom level 1) - Child tiles use progressively smaller geometric errors based on their zoom level and y-coordinate
- There's a dedicated
geometric_errorfunction in the tiling scheme that calculates appropriate values for different zoom levels
The large value for the root is a standard practice in tilesets to ensure proper LOD selection, while child tiles use more precise values (as seen in the test cases ranging from ~500k to ~65k depending on the zoom level).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usages of `geometric_error` in the codebase to compare values.
# Search for `geometric_error` assignments
rg 'geometric_error\s*:\s*[\d.e+-]+'
Length of output: 129
Script:
#!/bin/bash
# Search for any mentions of geometric error in comments or documentation
rg -i "geometric.?error" --type-add 'docs:*.{md,txt,rst}' -t docs -t rust -C 3
# Also search for test cases that might validate geometric error
rg -i "geometric.?error.*test"
Length of output: 9554
engine/runtime/examples/plateau/testdata/workflow/data-convert/07-brid-tun-cons/workflow.yml
Outdated
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
GltfWriterprocessor for exporting features to GLTF format.GltfWriterSinkFactoryfor creating sinks that write features in GLTF format.Cesium3DTilesWriterand enhanced processing capabilities.BoundingVolumestruct.Bug Fixes
Documentation