feat: add OrientableSurface struct and parsing logic for enhanced geometry handling #34
Conversation
WalkthroughThe pull request introduces several enhancements across multiple files, focusing on the handling of geometric data and underground building attributes. Key changes include the addition of a new struct Changes
Possibly related PRs
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (12)
nusamai-citygml/src/geometry.rs (2)
96-101: Add documentation comments to explain the OrientableSurface struct.While the struct is well-designed, it would benefit from documentation comments explaining:
- The purpose and use case of OrientableSurface
- The significance of the reverse field
- Any constraints or invariants that must be maintained
Add documentation like this:
#[derive(Debug, Clone)] #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] +/// Represents a surface that can be oriented in 3D space. +/// +/// # Fields +/// * `surface_id` - Reference to the base surface being oriented +/// * `reverse` - When true, indicates that the surface orientation should be reversed pub struct OrientableSurface { pub surface_id: LocalId, pub reverse: bool, }
121-122: Maintain documentation consistency across structs.The
orientable_surfacesfield inGeometryCollectorshould have the same level of documentation as its counterpart inGeometryStorefor consistency.Add documentation like this:
/// Lists of surface for composite surface pub composite_surfaces: Vec<LocalId>, + /// Orientable surfaces for each surface pub orientable_surfaces: Vec<OrientableSurface>,nusamai-plateau/src/models/iur/uro/underground_building.rs (1)
Line range hint
59-89: Consider adding documentation for the new attributesWhile the field names and types are clear, adding documentation comments would help users understand:
- The purpose and expected values for each attribute
- The relationship between fields with dual CityGML paths
- The meaning of the Property-suffixed types and their relationships
Example documentation format:
/// Represents the quality attributes of building data. /// /// # CityGML Paths /// - `uro:buildingDataQualityAttribute/uro:BuildingDataQualityAttribute` (new format) /// - `uro:bldgDataQualityAttribute/uro:DataQualityAttribute` (legacy format) #[citygml(path = b"uro:buildingDataQualityAttribute/uro:BuildingDataQualityAttribute")] #[citygml(path = b"uro:bldgDataQualityAttribute/uro:DataQualityAttribute")] pub bldg_data_quality_attribute: Option<uro::DataQualityAttribute>,nusamai-citygml/src/parser.rs (9)
753-753: Improve error message clarity inparse_triangulated_propThe error message
"Unexpected element <{}> by parse triangulated prop"can be enhanced for better readability. Consider rephrasing it to:-"Unexpected element <{}> by parse triangulated prop", +"Unexpected element <{}> while parsing triangulated property",
817-817: Enhance error message inparse_triangle_patch_arrayThe error message
"Unexpected element <{}> by parsing triangle patch array"is slightly unclear. Rewriting it improves clarity:-"Unexpected element <{}> by parsing triangle patch array", +"Unexpected element <{}> while parsing triangle patch array",
846-850: Clarify error message inparse_multi_surfaceThe error message
"Unexpected element. Because only surface member"can be made more precise:-"Unexpected element. Because only surface member".into(), +"Unexpected element: expected 'surfaceMember' element".into(),
889-889: Refine error message inparse_composite_surfaceTo enhance understanding, modify the error message to:
-"Unexpected element <{}> by parsing composite surface", +"Unexpected element <{}> while parsing composite surface",
945-965: Refactor duplicated code for parsingOrientableSurfaceThe logic for parsing the
OrientableSurfaceand handling theorientationattribute is duplicated in multiple places (lines 642-664 and 945-965). Refactoring this into a helper function enhances code maintainability and reduces duplication.Apply this diff to extract the common logic:
+fn parse_orientation_attribute(&mut self, start: &BytesStart) -> Result<String, ParseError> { + let mut orientation = "+".to_string(); + for attr in start.attributes().flatten() { + let (_, localname) = self.reader.resolve_attribute(attr.key); + if localname.as_ref() == b"orientation" { + let value = String::from_utf8_lossy(attr.value.as_ref()).to_string(); + orientation = value; + break; + } + } + if orientation != "+" && orientation != "-" { + return Err(ParseError::InvalidValue(format!("Invalid orientation value: {}", orientation))); + } + Ok(orientation) +}Then replace the duplicated code with:
-let mut orientation = "+".to_string(); -for attr in start.attributes().flatten() { - let (_, localname) = self.reader.resolve_attribute(attr.key); - if localname.as_ref() == b"orientation" { - let value = String::from_utf8_lossy(attr.value.as_ref()).to_string(); - orientation = value; - break; - } -} +let orientation = self.parse_orientation_attribute(&start)?;
973-973: Improve error message inparse_surfaceTo make the error message clearer, consider:
-"Unexpected element <{}> by parsing surface", +"Unexpected element <{}> while parsing surface",
1025-1028: Clarify error message inparse_orientable_surfaceThe error message
"Unexpected element. Because only base surface"can be made more precise:-"Unexpected element. Because only base surface".into(), +"Unexpected element: expected only 'baseSurface' element".into(),
1232-1232: Correct singular form in error messageThe message
"Unexpected elements <{}> inside <app:textureCoordinates>"should use the singular form to refer to a single element:-"Unexpected elements <{}> inside <app:textureCoordinates>", +"Unexpected element <{}> inside <app:textureCoordinates>",
1258-1258: Adjust error message inparse_tex_coord_listFor clarity and correctness, modify the error message to:
-"Unexpected elements <{}> inside <app:TexCoordList>", +"Unexpected element <{}> inside <app:TexCoordList>",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
nusamai-citygml/src/geometry.rs(4 hunks)nusamai-citygml/src/parser.rs(11 hunks)nusamai-plateau/src/models/iur/uro/underground_building.rs(1 hunks)
🔇 Additional comments (3)
nusamai-citygml/src/geometry.rs (1)
184-184: 🛠️ Refactor suggestion
Verify surface_id references during geometry conversion.
The into_geometries method should validate that all surface_id references in orientable_surfaces point to valid surfaces in surface_spans to maintain data integrity.
Let's verify the surface references:
Consider adding validation like this:
// Add before constructing GeometryStore
for orientable in &self.orientable_surfaces {
if !self.surface_spans.iter().any(|span| span.id == orientable.surface_id) {
return Err(format!("Invalid surface reference: {:?}", orientable.surface_id));
}
}nusamai-plateau/src/models/iur/uro/underground_building.rs (1)
Line range hint 93-143: Verify surface mapping compatibility with OrientableSurface
Given that this PR introduces OrientableSurface handling, we should verify if UNDERGROUND_BUILDING_SURFACE_MAPPINGS needs to be updated to handle this new surface type.
nusamai-citygml/src/parser.rs (1)
21-21: Import of OrientableSurface is appropriate
The addition of OrientableSurface to the imports aligns with the new functionality introduced and is correctly placed.
Summary by CodeRabbit
New Features
OrientableSurfacein geometry handling, enhancing complex geometric representations.UndergroundBuildingstructure, improving data capture for building quality, disaster risk, and facilities.Bug Fixes