feat: update nusamai dependencies and add support for LineString3D in geometry processing #35
Conversation
…ing3D in geometry processing
WalkthroughThe pull request introduces several enhancements across multiple files. In Changes
Possibly related PRs
Poem
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 (
|
miseyu
changed the title
![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 (2)
nusamai-citygml/src/geometry.rs (1)
152-159: Consider validating input coordinates inadd_linestringThe
add_linestringmethod does not perform validation on the input coordinates. Consider adding checks to ensure that the input iterator provides valid 3D points and handle any potential errors or inconsistencies.If input validation is necessary, you can modify the method as follows:
pub fn add_linestring(&mut self, iter: impl IntoIterator<Item = [f64; 3]>) { for v in iter { + if !v.iter().all(|coord| coord.is_finite()) { + // Handle invalid coordinate + continue; // or return an error + } let vbits = [v[0].to_bits(), v[1].to_bits(), v[2].to_bits()]; let (index, _) = self.vertices.insert_full(vbits); self.multilinestring.add_point(index as u32); } self.multilinestring.end_line(); }nusamai-gltf/src/glb.rs (1)
Line range hint
57-73: Handle optional BIN chunk in GLB filesThe
from_readermethod assumes that the BIN chunk is always present, which may not be the case for all GLB files. According to the GLB 2.0 specification, the BIN chunk is optional. Modify the method to handle cases where the BIN chunk is absent to improve compatibility.Apply this diff to handle the optional BIN chunk:
// BIN chunk - let bin_content_size = reader.read_u32::<LittleEndian>()?; - reader.read_exact(&mut buf)?; - if &buf[0..4] != b"BIN\x00" { - return Err(std::io::Error::new( - std::io::ErrorKind::InvalidData, - "Second chunk must be 'BIN'".to_string(), - )); - } - let mut bin_content = vec![0; bin_content_size as usize]; - reader.read_exact(&mut bin_content)?; + if let Ok(bin_content_size) = reader.read_u32::<LittleEndian>() { + reader.read_exact(&mut buf)?; + if &buf[0..4] != b"BIN\x00" { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidData, + "Expected 'BIN' in chunk type".to_string(), + )); + } + let mut bin_content = vec![0; bin_content_size as usize]; + reader.read_exact(&mut bin_content)?; + return Ok(Self { + json: json_content.into(), + bin: Some(bin_content.into()), + }); + } else { + return Ok(Self { + json: json_content.into(), + bin: None, + }); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
nusamai-citygml/src/geometry.rs(1 hunks)nusamai-citygml/src/parser.rs(11 hunks)nusamai-gltf/src/glb.rs(1 hunks)
🔇 Additional comments (3)
nusamai-citygml/src/parser.rs (2)
729-730:
Ensure proper handling of line_begin when parsing geometries
In the parse_geometry_prop method, when parsing a LineString, line_begin is initialized before the match statement. However, if the code path does not process a LineString or MultiCurve, line_begin may not be correctly used. Make sure that line_begin and line_end are appropriately handled for each geometry type to prevent incorrect geometry references.
Run the following script to check the usage of line_begin and ensure it's properly initialized and used:
1107-1124:
Verify depth management in parse_linestring
The depth management in the parse_linestring method may not correctly handle all XML structures, potentially leading to incorrect parsing or premature termination. Ensure that the depth tracking accurately reflects the XML hierarchy and that the method can handle various nested elements properly.
Run the following script to test parse_linestring with different XML inputs:
If you need assistance in setting up tests or adjusting the implementation, please let me know.
nusamai-gltf/src/glb.rs (1)
20-20: Improve lifetime annotations by removing unnecessary elision
The change from impl<'a> Glb<'a> to impl Glb<'_> simplifies the lifetime annotations. Double-check that this change does not affect the lifetime requirements elsewhere in the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
nusamai-citygml/src/parser.rs (4)
390-390: Consider addressing the FIXME comment.The FIXME comment suggests incomplete implementation. Please document what needs to be fixed or remove the comment if it's no longer relevant.
Would you like me to help identify what needs to be fixed or open a GitHub issue to track this?
Line range hint
652-789: Consider refactoring geometry reference handling.The code for handling geometry references is similar between polygon and line geometries. Consider extracting this common logic into a helper method to reduce duplication.
Example refactor:
fn add_geometry_ref(&mut self, geomrefs: &mut GeometryRefs, geomtype: GeometryType, lod: u8, start: usize, end: usize, surface_id: Option<LocalId>, feature_id: Option<String>, feature_type: Option<String>) { if end - start > 0 { geomrefs.push(GeometryRef { ty: geomtype, lod, pos: start as u32, len: (end - start) as u32, id: surface_id, solid_ids: Vec::new(), feature_id, feature_type, }); } }
857-883: Improve error message clarity.The error message "Unexpected element. Because only surface member" could be more descriptive.
- return Err(ParseError::SchemaViolation( - "Unexpected element. Because only surface member".into(), - )) + return Err(ParseError::SchemaViolation(format!( + "Unexpected element in MultiCurve: expected <curveMember> but found <{}>", + String::from_utf8_lossy(start.name().as_ref()) + )))
1105-1165: Consider adding documentation for depth tracking.The depth tracking logic could benefit from documentation explaining the expected XML structure and what each depth level represents.
Add documentation like:
/// Parses a LineString element with the following structure: /// <LineString> // depth = 1 /// <posList> // depth = 2 /// <coordinates> // depth = 3 /// </posList> /// </LineString> fn parse_linestring(&mut self) -> Result<(), ParseError> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
nusamai-citygml/src/parser.rs(12 hunks)
🔇 Additional comments (3)
nusamai-citygml/src/parser.rs (3)
Line range hint 105-113: LGTM: Default implementation for ParseContext is well-structured.
The implementation provides appropriate defaults with a valid file URL and NoopResolver.
400-451: LGTM: Well-structured implementation of MultiCurve property parsing.
The implementation follows consistent patterns with other geometry parsing methods and includes proper error handling.
1291-1294: LGTM: Proper error handling for text unescaping.
The implementation now properly handles potential errors when unescaping text content.
Summary by CodeRabbit
add_linestringmethod for adding line strings to theGeometryCollector.MultiCurveandLineStringgeometries in theCityGmlReader.MultiCurveelements.from_readermethod.Glbstruct implementation.