feat(engine): enhance CityGML reader and update example workflows #647
Conversation
WalkthroughThe changes in this pull request involve modifications to the 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: 1
🧹 Outside diff range and nitpick comments (1)
engine/runtime/examples/plateau/testdata/workflow/data-convert/03-frn-veg/workflow.yml (1)
1-148: Consider workflow robustness improvementsWhile the workflow structure is sound, consider these architectural improvements:
- Add validation steps between major transformations
- Consider implementing retry logic for file operations
- Add monitoring/logging nodes for better observability
- Consider splitting the workflow into smaller, reusable subgraphs for better maintainability
Would you like me to provide specific examples for any of these improvements?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
engine/runtime/action-processor/src/feature/reader/citygml.rs(4 hunks)engine/runtime/examples/plateau/example_feature_transformer.rs(1 hunks)engine/runtime/examples/plateau/testdata/workflow/data-convert/03-frn-veg/workflow.yml(1 hunks)engine/runtime/examples/plateau/testdata/workflow/data-convert/04-luse-lsld/workflow.yml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- engine/runtime/examples/plateau/example_feature_transformer.rs
- engine/runtime/examples/plateau/testdata/workflow/data-convert/04-luse-lsld/workflow.yml
🔇 Additional comments (8)
engine/runtime/examples/plateau/testdata/workflow/data-convert/03-frn-veg/workflow.yml (5)
113-148: LGTM! Edge definitions are well-structured
The edge definitions properly connect the nodes in a logical sequence, with clear port specifications and unique identifiers.
107-111: Verify zoom level constraints for 3D tiles
The zoom levels (15-18) might need adjustment based on the data density and performance requirements. Also, ensure the output path exists.
✅ Verification successful
Zoom level configuration is consistent with other 3D tile configurations
Based on the search results, the zoom level configuration (minZoom: 15, maxZoom: 18) in the file is consistent with all other Cesium3DTilesWriter configurations in the codebase:
- Building models (01-bldg): minZoom: 15, maxZoom: 18
- Transportation models (02-tran): minZoom: 15, maxZoom: 18
- Flood models (05-fld): minZoom: 15, maxZoom: 18
The zoom levels appear to be standardized for 3D tile outputs across different feature types, suggesting this is an intentional design choice for optimal performance and visualization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other zoom level configurations for comparison
rg -g '*.{yml,yaml}' 'minZoom|maxZoom' -C 2
Length of output: 6216
18-19: Verify existence of included graph files
The workflow depends on external graph definitions. Ensure these files exist and are properly maintained.
✅ Verification successful
Graph files are valid and properly maintained
Both referenced graph files exist and contain valid graph definitions with proper node configurations, edge connections, and unique IDs. The files are located at:
./engine/runtime/examples/plateau/testdata/graphs/attribute_reader.yml./engine/runtime/examples/plateau/testdata/graphs/folder_and_file_path_reader.yml
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the referenced graph files exist
fd -g '*{attribute_reader,folder_and_file_path_reader}.yml' -x echo "Found graph at: {}"
Length of output: 364
Script:
#!/bin/bash
# Check the content of the found files to verify they are valid graph definitions
echo "=== Content of attribute_reader.yml ==="
cat ./engine/runtime/examples/plateau/testdata/graphs/attribute_reader.yml
echo -e "\n=== Content of folder_and_file_path_reader.yml ==="
cat ./engine/runtime/examples/plateau/testdata/graphs/folder_and_file_path_reader.yml
Length of output: 5572
6-16: Consider providing default values for input parameters
The workflow parameters (cityGmlPath, cityCode, etc.) are defined without default values. This could lead to runtime errors if not all parameters are provided when executing the workflow.
10-10: Verify schema.txt path resolution
The schema inclusion uses a relative path (../../../config/schema.txt). Ensure this path is correctly resolved in all deployment scenarios.
✅ Verification successful
Schema path resolution is correctly configured
The schema file exists at the expected location engine/runtime/examples/plateau/testdata/config/schema.txt, which matches the relative path ../../../config/schema.txt from the workflow file's location at engine/runtime/examples/plateau/testdata/workflow/data-convert/03-frn-veg/workflow.yml.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the schema file exists and is accessible
fd -g 'schema.txt' -x echo "Found schema at: {}"
Length of output: 126
engine/runtime/action-processor/src/feature/reader/citygml.rs (3)
69-72: LGTM: Efficient parameter passing and attribute preservation
The changes improve efficiency by passing context by reference and ensure attribute preservation by passing base attributes from the feature.
81-84: LGTM: Function signature properly updated
The signature changes appropriately reflect the new parameter requirements and optimize performance through reference passing.
Line range hint 158-176: Consider attribute conflict resolution and performance optimization
The current implementation has several potential issues:
- Base attributes might override CityGML-specific attributes due to the extension order
- Cloning the entire base_attributes map could be inefficient for large attribute sets
- There's no strategy for handling attribute name conflicts
Consider this alternative implementation:
- let mut attributes = HashMap::<Attribute, AttributeValue>::from([
+ let mut attributes = base_attributes.clone();
+ // Ensure CityGML-specific attributes take precedence
+ attributes.extend([
(Attribute::new("cityGmlAttributes"), attributes.into()),
(
Attribute::new("gmlName"),
name.map(|s| AttributeValue::String(s.to_string()))
.unwrap_or(AttributeValue::Null),
),
(
Attribute::new("gmlId"),
gml_id
.map(|s| AttributeValue::String(s.to_string()))
.unwrap_or(AttributeValue::Null),
),
(
Attribute::new("gmlRootId"),
AttributeValue::String(format!("root_{}", to_hash(base_url.as_str()))),
),
]);
- attributes.extend(base_attributes.clone());This change:
- Ensures CityGML attributes take precedence over base attributes
- Performs only one clone operation
- Makes the precedence rules more explicit
Let's verify the attribute naming patterns to identify potential conflicts:
engine/runtime/examples/plateau/testdata/workflow/data-convert/03-frn-veg/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