Export tools now accept "--frame enu" to generate XYZ data in ENU frame#43
Merged
Export tools now accept "--frame enu" to generate XYZ data in ENU frame#43
Conversation
📝 WalkthroughWalkthroughThree point cloud export utilities (mm2las, mm2ply, mm2txt) gain a new --frame CLI option to export points in either the map frame (default) or ENU (East-North-Up) frame. On-the-fly coordinate transformation applies when ENU is selected, using georeferencing data from the input map. Export function signatures are updated to accept an optional transformation parameter, and documentation is revised accordingly. Changes
Sequence DiagramsequenceDiagram
participant User
participant Tool as Export Tool
participant Map as Input Map
participant Transform as Transform Engine
participant Output as Output File
User->>Tool: Execute with --frame enu
Tool->>Tool: Parse and validate --frame argument
Tool->>Map: Load map and retrieve georeferencing data
alt Georeferencing exists
Map-->>Tool: Return georeferencing data
Tool->>Transform: Construct T_enu_to_map from georeferencing
Transform-->>Tool: Transformation ready
loop For each point
Tool->>Transform: Apply T_enu_to_map to point coordinates
Transform-->>Tool: Return transformed (tx, ty, tz)
Tool->>Output: Write transformed coordinates
end
Output-->>User: Export complete in ENU frame
else Missing georeferencing
Map-->>Tool: No georeferencing data
Tool-->>User: Error: ENU frame requires georeferencing
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/mm2txt/main.cpp (1)
407-436:⚠️ Potential issue | 🟠 Major
--frame enuis silently ignored for legacy point cloud types.When the layer contains a legacy type (
CPointsMapXYZIRT,CPointsMapXYZI, or a plainCPointsMapin the else branch), the code falls through to native save methods (saveXYZIRT_to_text_file,saveXYZI_to_text_file,save3D_to_text_file) which don't accept a transformation parameter. If the user requests--frame enu, coordinates are exported in the map frame with no warning.At minimum, emit a warning when
T_enu_to_map.has_value()and the legacy path is taken. Ideally, error out since the user's request can't be honored.Suggested fix
else { + if (T_enu_to_map.has_value()) + { + std::cerr << "Warning: --frame enu is not supported for legacy point map type '" + << pts->GetRuntimeClass()->className + << "' in layer '" << name << "'. Exporting in map frame.\n"; + } `#pragma` GCC diagnostic push `#pragma` GCC diagnostic ignored "-Wdeprecated-declarations"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mm2txt/main.cpp` around lines 407 - 436, The legacy branch that calls CPointsMapXYZIRT::saveXYZIRT_to_text_file, CPointsMapXYZI::saveXYZI_to_text_file or pts->save3D_to_text_file ignores a requested ENU transform; update the else branch so that if T_enu_to_map.has_value() is true you emit a clear warning (or better: return an error/exit non‑zero) before calling the legacy save methods, e.g. detect T_enu_to_map.has_value() at the start of the legacy-handling block and then either call processLogger.warn(...) or print an error and exit, referencing the legacy classes CPointsMapXYZIRT, CPointsMapXYZI and the pts->save3D_to_text_file call so the user knows the ENU frame request cannot be honored.
🧹 Nitpick comments (1)
apps/mm2ply/main.cpp (1)
343-350: Transformed ENU coordinates are truncated tofloatfor PLY output.The transformed coordinates are computed as
doublebut cast tofloaton Line 347 before writing. The PLY header declares x/y/z asfloat(consistent with the non-ENU path), so this is format-correct. However, if ENU coordinates are large (e.g., hundreds of km),floatprecision (~7 significant digits) may introduce cm-level quantization. This may be acceptable given the PLY format constraint, but worth noting for users of the--frame enufeature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mm2ply/main.cpp` around lines 343 - 350, The code casts ENU-transformed coordinates (tx/ty/tz) to float before writing (in the block checking T_enu_to_map and acc.name), which loses precision for large ENU values; to fix, keep tx/ty/tz as double when writing and update the PLY header generation to declare x/y/z as double when the ENU frame is used (so the PLY types match), and call write_val with a double format (e.g., "%.16e") without static_cast<float>, adjusting any downstream writers that rely on float-sized values; reference symbols: T_enu_to_map, tx, ty, tz, acc.name, write_val and the PLY header generation logic that emits x/y/z types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/mm2las/main.cpp`:
- Around line 601-611: The code applies T_enu_to_map->composePoint(...) to
points that are already in the metric map frame, which is backwards; replace the
call to composePoint(...) with inverseComposePoint(...) so the transform
converts map->ENU (i.e. call T_enu_to_map->inverseComposePoint(xs[i], ys[i],
zs[i], px, py, pz) or the equivalent overload used elsewhere), and make the same
replacement for the identical usages in the other tools (mm2txt and mm2ply).
Ensure you update every occurrence where composePoint is used on xs/ys/zs ->
px/py/pz guarded by T_enu_to_map.has_value().
In `@apps/mm2ply/main.cpp`:
- Around line 307-312: The transform is applied in the wrong direction: when
converting points from ENU to map you must call inverseComposePoint, not
composePoint. In the block using T_enu_to_map (the code that currently calls
T_enu_to_map->composePoint(xs[i], ys[i], zs[i], tx, ty, tz)), replace that call
with T_enu_to_map->inverseComposePoint(xs[i], ys[i], zs[i], tx, ty, tz) so the
ENU coordinates (xs, ys, zs) are correctly transformed into map coordinates (tx,
ty, tz).
In `@apps/mm2txt/main.cpp`:
- Around line 152-157: The ENU->map on-the-fly transform is using
T_enu_to_map->composePoint but likely needs the inverse direction (use
inverseComposePoint) as in the mm2las fix; change the call in the loop that
currently invokes T_enu_to_map->composePoint(xs.at(i), ys.at(i), zs.at(i), tx,
ty, tz) to call T_enu_to_map->inverseComposePoint(...) so the input ENU
coordinates (xs.at(i), ys.at(i), zs.at(i)) are correctly mapped to tx, ty, tz,
and keep the same tx/ty/tz output variables and logic.
---
Outside diff comments:
In `@apps/mm2txt/main.cpp`:
- Around line 407-436: The legacy branch that calls
CPointsMapXYZIRT::saveXYZIRT_to_text_file, CPointsMapXYZI::saveXYZI_to_text_file
or pts->save3D_to_text_file ignores a requested ENU transform; update the else
branch so that if T_enu_to_map.has_value() is true you emit a clear warning (or
better: return an error/exit non‑zero) before calling the legacy save methods,
e.g. detect T_enu_to_map.has_value() at the start of the legacy-handling block
and then either call processLogger.warn(...) or print an error and exit,
referencing the legacy classes CPointsMapXYZIRT, CPointsMapXYZI and the
pts->save3D_to_text_file call so the user knows the ENU frame request cannot be
honored.
---
Nitpick comments:
In `@apps/mm2ply/main.cpp`:
- Around line 343-350: The code casts ENU-transformed coordinates (tx/ty/tz) to
float before writing (in the block checking T_enu_to_map and acc.name), which
loses precision for large ENU values; to fix, keep tx/ty/tz as double when
writing and update the PLY header generation to declare x/y/z as double when the
ENU frame is used (so the PLY types match), and call write_val with a double
format (e.g., "%.16e") without static_cast<float>, adjusting any downstream
writers that rely on float-sized values; reference symbols: T_enu_to_map, tx,
ty, tz, acc.name, write_val and the PLY header generation logic that emits x/y/z
types.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #43 +/- ##
========================================
Coverage 77.49% 77.49%
========================================
Files 185 185
Lines 9883 9883
Branches 934 934
========================================
Hits 7659 7659
Misses 2224 2224 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
--framecommand-line option to mm2las, mm2ply, and mm2txt applications, allowing users to export point clouds in either map or ENU (East-North-Up) coordinate frames.Documentation
--frameoption specifications, usage examples, and coordinate frame transformation details.