mm2las: export flag '--frame geodetic' for georeferenced clouds#49
Merged
mm2las: export flag '--frame geodetic' for georeferenced clouds#49
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a "geodetic" export mode to mm2las (EPSG:4979). CLI accepts "geodetic"; saveToLas gains an optional Georeferencing parameter; per‑point lat/lon/alt fields are used if present or ENU/map→geodetic conversion is performed via mrpt‑topography; LAS header/scale/VLRs updated and mrpt‑topography is linked. Changes
Sequence DiagramsequenceDiagram
participant CLI as User/CLI
participant Parser as Option Parser
participant Map as Input Map/Points
participant Topo as Topography Converter (mrpt-topography)
participant LAS as LAS Writer
CLI->>Parser: --frame geodetic + files
Parser->>Map: Load points & optional georeferencing
alt Per-point lat/lon/alt present
Map->>LAS: Emit per-point geodetic coords
else
Map->>Topo: Request ENU/map→geodetic conversion (uses georef)
Topo->>Map: Return lat/lon/alt per point
Map->>LAS: Emit converted geodetic coords
end
LAS->>LAS: Add EPSG:4979 WKT VLR and extra-dim VLRs
LAS->>CLI: Write LAS file with geodetic coords and VLRs
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 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)
📝 Coding Plan
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 Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
![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
🤖 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/CMakeLists.txt`:
- Line 4: The package is missing the corresponding ROS2 package dependency for
mrpt-topography; update package.xml to add a runtime/build dependency by
inserting a <depend>mrpt_libtopography</depend> entry (matching how mrpt_libmaps
and mrpt_libobs are declared) so colcon/ROS2 builds recognize the
mrpt-topography linkage referenced by find_package(mrpt-topography REQUIRED) in
the CMakeLists.
In `@apps/mm2las/main.cpp`:
- Around line 263-265: The variable lonBuf is incorrectly declared as a
pointer-to-pointer; change its type from const
mrpt::aligned_std_vector<double>** to const mrpt::aligned_std_vector<double>* so
it matches latBuf and altBuf and can be dereferenced safely (update the
declaration of lonBuf and any related initializations/uses where lonBuf is
dereferenced, e.g., places referenced near the current declarations and where
it's used later around the dereferences at lines that correspond to the code
using lonBuf).
- Around line 309-314: The precomputed transform T_map_to_enu_geodetic is
inverted incorrectly; when geodetic.has_value() and !hasPerPointGeodetic you
should assign the map→ENU transform directly from geodetic->T_enu_to_map.mean
(not its negation) so subsequent calls that expect
T_enu_to_map.composePoint(p_map) to produce ENU coordinates behave the same as
the ENU export path; update the initialization of T_map_to_enu_geodetic
accordingly in the block that references geodetic and hasPerPointGeodetic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b3644753-844f-4378-9255-fd288bee573f
📒 Files selected for processing (3)
apps/mm2las/CMakeLists.txtapps/mm2las/main.cppdocs/source/app_mm2las.rst
jlblancoc
force-pushed
the
mm2las/georef
branch
from
9f42b6d to
8d6e28f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/mm2las/main.cpp (2)
263-265:⚠️ Potential issue | 🔴 CriticalCritical type error:
lonBufdeclared with double pointer.
lonBufis declared asconst mrpt::aligned_std_vector<double>**but should beconst mrpt::aligned_std_vector<double>*to matchlatBufandaltBuf. This will cause a compilation error at line 290 wheregetPointsBufferRef_double_fieldreturns a single pointer.🐛 Proposed fix
[[maybe_unused]] const mrpt::aligned_std_vector<double>* latBuf = nullptr; - [[maybe_unused]] const mrpt::aligned_std_vector<double>** lonBuf = nullptr; + [[maybe_unused]] const mrpt::aligned_std_vector<double>* lonBuf = nullptr; [[maybe_unused]] const mrpt::aligned_std_vector<double>* altBuf = nullptr;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mm2las/main.cpp` around lines 263 - 265, The variable lonBuf is incorrectly declared as a double pointer; change its type to match latBuf and altBuf by declaring lonBuf as const mrpt::aligned_std_vector<double>* (not **), so it can hold the single-pointer returned by getPointsBufferRef_double_field; update the declaration near other buffers (latBuf, altBuf) and ensure subsequent uses with getPointsBufferRef_double_field compile without pointer-level mismatch.
309-314:⚠️ Potential issue | 🔴 CriticalTransformation direction inconsistency: geodetic path inverts the transform incorrectly.
Per the codebase convention,
T_enu_to_map.composePoint(p_map)yields ENU-frame coordinates. The ENU export path at line 728 correctly usesT_enu_to_mapdirectly. However, the geodetic path uses the negation (inverse) at line 313, which reverses the transformation direction.🐛 Proposed fix
std::optional<mrpt::poses::CPose3D> T_map_to_enu_geodetic; if (geodetic.has_value() && !hasPerPointGeodetic) { - T_map_to_enu_geodetic = -geodetic->T_enu_to_map.mean; + T_map_to_enu_geodetic = geodetic->T_enu_to_map.mean; }Based on learnings: "T_enu_to_map.composePoint(p_map) as mapping from map-frame to ENU-frame".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mm2las/main.cpp` around lines 309 - 314, The code incorrectly negates the geodetic transform when computing T_map_to_enu_geodetic, reversing the intended map->ENU direction; in the geodetic block (check geodetic and !hasPerPointGeodetic) set T_map_to_enu_geodetic to geodetic->T_enu_to_map.mean (use the transform as-is) instead of assigning its negation so that T_enu_to_map.composePoint(p_map) continues to produce ENU-frame coordinates.
🧹 Nitpick comments (1)
apps/mm2las/main.cpp (1)
686-708: Consider caching geodetic coordinates to avoid duplicate computation.The
pointToGeodeticlambda is called twice per point: once during bounds calculation (lines 710-737) and once during point data writing (lines 930-952). For large point clouds, this doubles the cost of the expensive geodetic conversion (involving ellipsoid math).Consider pre-computing all geodetic coordinates into temporary buffers before the bounds loop, then reusing them for both bounds and point writing. This trades memory for CPU time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mm2las/main.cpp` around lines 686 - 708, The pointToGeodetic lambda is being invoked twice per point causing redundant expensive conversions; instead, before the bounds calculation loop, allocate and fill cache vectors (e.g., std::vector<double> cached_lon, cached_lat, cached_alt or std::vector<std::array<double,3>> cached_geod) by iterating over indices and using the same logic as pointToGeodetic (respecting hasPerPointGeodetic by copying from lonBuf/latBuf/altBuf when true, otherwise performing the ENU->geocentric->geodetic conversion using T_map_to_enu_geodetic, ENUToGeocentric and geocentricToGeodetic), then replace calls to pointToGeodetic in the bounds calculation and the point-writing section with reads from these cached vectors (referencing xs/ys/zs only for the conversion step when filling the cache).
🤖 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 181-203: The WKT string kWKT_EPSG4979 is WKT2 and declares
latitude as ORDER[1], which will cause CRS-aware readers to swap X/Y; replace it
with a WKT1 (OGC 2001) representation that explicitly uses GEOGCS-style syntax
and declares longitude as the first axis (lon, lat, ellipsoidal height) to match
how coordinates are stored (X=lon, Y=lat, Z=height). Generate or copy a correct
WKT1 for EPSG:4979 (3D WGS84) — e.g. using proj/gdal utilities — and update the
kWKT_EPSG4979 string so axis order and keywords conform to WKT1 (ensure AXIS
entries put longitude first) to avoid misinterpretation by strict LAS readers.
---
Duplicate comments:
In `@apps/mm2las/main.cpp`:
- Around line 263-265: The variable lonBuf is incorrectly declared as a double
pointer; change its type to match latBuf and altBuf by declaring lonBuf as const
mrpt::aligned_std_vector<double>* (not **), so it can hold the single-pointer
returned by getPointsBufferRef_double_field; update the declaration near other
buffers (latBuf, altBuf) and ensure subsequent uses with
getPointsBufferRef_double_field compile without pointer-level mismatch.
- Around line 309-314: The code incorrectly negates the geodetic transform when
computing T_map_to_enu_geodetic, reversing the intended map->ENU direction; in
the geodetic block (check geodetic and !hasPerPointGeodetic) set
T_map_to_enu_geodetic to geodetic->T_enu_to_map.mean (use the transform as-is)
instead of assigning its negation so that T_enu_to_map.composePoint(p_map)
continues to produce ENU-frame coordinates.
---
Nitpick comments:
In `@apps/mm2las/main.cpp`:
- Around line 686-708: The pointToGeodetic lambda is being invoked twice per
point causing redundant expensive conversions; instead, before the bounds
calculation loop, allocate and fill cache vectors (e.g., std::vector<double>
cached_lon, cached_lat, cached_alt or std::vector<std::array<double,3>>
cached_geod) by iterating over indices and using the same logic as
pointToGeodetic (respecting hasPerPointGeodetic by copying from
lonBuf/latBuf/altBuf when true, otherwise performing the
ENU->geocentric->geodetic conversion using T_map_to_enu_geodetic,
ENUToGeocentric and geocentricToGeodetic), then replace calls to pointToGeodetic
in the bounds calculation and the point-writing section with reads from these
cached vectors (referencing xs/ys/zs only for the conversion step when filling
the cache).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d25eb613-0c90-45f4-b89b-bcac5e3a83a3
📒 Files selected for processing (3)
apps/mm2las/CMakeLists.txtapps/mm2las/main.cppdocs/source/app_mm2las.rst
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/mm2las/CMakeLists.txt
- docs/source/app_mm2las.rst
Comment on lines
+181
to
+203
| // WKT string for EPSG:4979 (WGS 84 geographic 3D) | ||
| // X=Longitude, Y=Latitude, Z=Ellipsoidal height | ||
| static const char* const kWKT_EPSG4979 = | ||
| "GEOGCRS[\"WGS 84\"," | ||
| "ENSEMBLE[\"World Geodetic System 1984 ensemble\"," | ||
| "MEMBER[\"World Geodetic System 1984 (Transit)\"]," | ||
| "MEMBER[\"World Geodetic System 1984 (G730)\"]," | ||
| "MEMBER[\"World Geodetic System 1984 (G873)\"]," | ||
| "MEMBER[\"World Geodetic System 1984 (G1150)\"]," | ||
| "MEMBER[\"World Geodetic System 1984 (G1674)\"]," | ||
| "MEMBER[\"World Geodetic System 1984 (G1762)\"]," | ||
| "MEMBER[\"World Geodetic System 1984 (G2139)\"]," | ||
| "ELLIPSOID[\"WGS 84\",6378137,298.257223563," | ||
| "LENGTHUNIT[\"metre\",1]]," | ||
| "ENSEMBLEACCURACY[2.0]]," | ||
| "PRIMEM[\"Greenwich\",0,ANGLEUNIT[\"degree\",0.0174532925199433]]," | ||
| "CS[ellipsoidal,3]," | ||
| "AXIS[\"geodetic latitude (Lat)\",north,ORDER[1],ANGLEUNIT[\"degree\",0.0174532925199433]]," | ||
| "AXIS[\"geodetic longitude (Lon)\",east,ORDER[2],ANGLEUNIT[\"degree\",0.0174532925199433]]," | ||
| "AXIS[\"ellipsoidal height (h)\",up,ORDER[3],LENGTHUNIT[\"metre\",1]]," | ||
| "USAGE[SCOPE[\"Geodesy.\"],AREA[\"World.\"]," | ||
| "BBOX[-90,-180,90,180]]," | ||
| "ID[\"EPSG\",4979]]"; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
EPSG:4979 axis order WKT LAS file convention
💡 Result:
- EPSG:4979 (WGS 84 3D) axis order in EPSG / WKT2: the CRS is defined with axes (latitude, longitude, ellipsoidal height) = (north, east, up). (epsg.io)
- But LAS point storage is always X/Y/Z: LAS point records store coordinates as X, Y, Z, and most tooling treats that as (longitude/easting, latitude/northing, height) (i.e., x = east/longitude, y = north/latitude). (gdal.org)
- LAS WKT convention constraint: LAS 1.4 WKT CRS metadata (in VLR/EVLR) is specified to use OGC 2001 “WKT1”, and (per USGS Lidar Base Spec guidance quoting LAS 1.4-R13) OGC 2015 / WKT2 is forbidden in LAS WKT records. (pubs.usgs.gov)
- Practical “LAS convention” takeaway for EPSG:4979: even though EPSG:4979’s formal axis order is lat, lon, h, in a LAS file you should expect/emit coordinate tuples in X,Y,Z = lon,lat,h order, and ensure your WKT/metadata doesn’t accidentally cause axis swapping in software that honors EPSG axis rules. (epsg.io)
Citations:
- 1: https://epsg.io/4979
- 2: https://gdal.org/en/stable/development/rfc/rfc20_srs_axes.html?utm_source=openai
- 3: https://pubs.usgs.gov/tm/11b4/pdf/tm11-B4.pdf
- 4: https://epsg.io/4979
🏁 Script executed:
# First, check the actual coordinate storage around lines 717-719 and 939-941
cat -n apps/mm2las/main.cpp | sed -n '710,730p'Repository: MOLAorg/mp2p_icp
Length of output: 771
🏁 Script executed:
cat -n apps/mm2las/main.cpp | sed -n '930,950p'Repository: MOLAorg/mp2p_icp
Length of output: 809
🏁 Script executed:
# Check context around the WKT definition and how it's used
cat -n apps/mm2las/main.cpp | sed -n '175,220p'Repository: MOLAorg/mp2p_icp
Length of output: 2488
🏁 Script executed:
# Check if there are any comments or documentation about WKT format choice
grep -n "WKT1\|WKT2\|OGC 2001\|OGC 2015\|LAS 1.4" apps/mm2las/main.cppRepository: MOLAorg/mp2p_icp
Length of output: 531
🏁 Script executed:
# Check if there are tests or other references to this WKT constant
grep -r "kWKT_EPSG4979" . --include="*.cpp" --include="*.h"Repository: MOLAorg/mp2p_icp
Length of output: 338
🏁 Script executed:
# Check nearby comments to understand intent of geodetic mode
sed -n '200,230p' apps/mm2las/main.cppRepository: MOLAorg/mp2p_icp
Length of output: 1273
WKT format violates LAS 1.4 spec and risks axis-swapping in readers.
The code correctly stores coordinates as X=longitude, Y=latitude, Z=height (LAS convention), but the WKT string uses WKT2 format (GEOGCRS, ENSEMBLE keywords). LAS 1.4 spec requires WKT1 (OGC 2001), not WKT2.
More critically: the WKT declares axis ORDER[1]=latitude, ORDER[2]=longitude (lines 198–199). Strict CRS-aware readers honoring these EPSG axis rules will swap X/Y, interpreting stored (lon,lat) as (lat,lon), breaking coordinates.
Convert the WKT string to WKT1 format with lon/lat axis order to match the actual coordinate storage, or accept that non-standard CRS readers may misinterpret the data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/mm2las/main.cpp` around lines 181 - 203, The WKT string kWKT_EPSG4979 is
WKT2 and declares latitude as ORDER[1], which will cause CRS-aware readers to
swap X/Y; replace it with a WKT1 (OGC 2001) representation that explicitly uses
GEOGCS-style syntax and declares longitude as the first axis (lon, lat,
ellipsoidal height) to match how coordinates are stored (X=lon, Y=lat,
Z=height). Generate or copy a correct WKT1 for EPSG:4979 (3D WGS84) — e.g. using
proj/gdal utilities — and update the kWKT_EPSG4979 string so axis order and
keywords conform to WKT1 (ensure AXIS entries put longitude first) to avoid
misinterpretation by strict LAS readers.
jlblancoc
force-pushed
the
mm2las/georef
branch
from
8d6e28f to
963b01a
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/mm2las/main.cpp (1)
181-203:⚠️ Potential issue | 🟠 MajorCRS axis metadata conflicts with written LAS coordinate order.
Line 198 and Line 199 declare axis order as latitude-first, but Line 932 and Line 933 write X=longitude and Y=latitude. CRS-aware readers can swap axes and misplace points.
🛠️ Proposed fix
- "AXIS[\"geodetic latitude (Lat)\",north,ORDER[1],ANGLEUNIT[\"degree\",0.0174532925199433]]," - "AXIS[\"geodetic longitude (Lon)\",east,ORDER[2],ANGLEUNIT[\"degree\",0.0174532925199433]]," + "AXIS[\"geodetic longitude (Lon)\",east,ORDER[1],ANGLEUNIT[\"degree\",0.0174532925199433]]," + "AXIS[\"geodetic latitude (Lat)\",north,ORDER[2],ANGLEUNIT[\"degree\",0.0174532925199433]],"For LAS 1.4 WKT CRS VLR (record_id 2112), what WKT dialect and axis-order convention should be used for EPSG:4979 when point tuples are stored as X=longitude, Y=latitude, Z=ellipsoidal height?Also applies to: 932-934
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mm2las/main.cpp` around lines 181 - 203, The WKT in kWKT_EPSG4979 declares axes latitude-first but the file stores points as X=longitude, Y=latitude; update the WKT to a WKT2 flavor that explicitly defines longitude as ORDER[1] and latitude as ORDER[2] (or use the WKT2_2019 convention) so axis metadata matches stored tuples; specifically edit the AXIS entries in kWKT_EPSG4979 to list "geodetic longitude (Lon)" as ORDER[1] and "geodetic latitude (Lat)" as ORDER[2], and ensure the WKT dialect used for the LAS VLR (record_id 2112) is WKT2 with explicit axis order to avoid CRS-aware readers swapping X/Y.
🧹 Nitpick comments (1)
apps/mm2las/main.cpp (1)
705-723: Refactor duplicated coordinate-branch logic into one helper.The map/ENU/geodetic coordinate-selection branch is repeated in two loops. Extracting one helper for
px,py,pzavoids future divergence bugs.Also applies to: 927-945
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mm2las/main.cpp` around lines 705 - 723, Refactor the duplicated coordinate-selection logic into a single helper that returns or fills px, py, pz given an index: create a function (e.g., getPointCoords(int i, double &px, double &py, double &pz) or std::tuple<double,double,double> getPointCoords(int i)) that contains the current branch using geodetic, pointToGeodetic, xs/ys/zs and T_enu_to_map->composePoint; replace both duplicated blocks (the one using geodetic and the one at the other loop) to call this helper so all callers use the same logic (referencing variables/identifiers geodetic, pointToGeodetic, xs, ys, zs, T_enu_to_map, composePoint).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/mm2las/main.cpp`:
- Around line 181-203: The WKT in kWKT_EPSG4979 declares axes latitude-first but
the file stores points as X=longitude, Y=latitude; update the WKT to a WKT2
flavor that explicitly defines longitude as ORDER[1] and latitude as ORDER[2]
(or use the WKT2_2019 convention) so axis metadata matches stored tuples;
specifically edit the AXIS entries in kWKT_EPSG4979 to list "geodetic longitude
(Lon)" as ORDER[1] and "geodetic latitude (Lat)" as ORDER[2], and ensure the WKT
dialect used for the LAS VLR (record_id 2112) is WKT2 with explicit axis order
to avoid CRS-aware readers swapping X/Y.
---
Nitpick comments:
In `@apps/mm2las/main.cpp`:
- Around line 705-723: Refactor the duplicated coordinate-selection logic into a
single helper that returns or fills px, py, pz given an index: create a function
(e.g., getPointCoords(int i, double &px, double &py, double &pz) or
std::tuple<double,double,double> getPointCoords(int i)) that contains the
current branch using geodetic, pointToGeodetic, xs/ys/zs and
T_enu_to_map->composePoint; replace both duplicated blocks (the one using
geodetic and the one at the other loop) to call this helper so all callers use
the same logic (referencing variables/identifiers geodetic, pointToGeodetic, xs,
ys, zs, T_enu_to_map, composePoint).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4cc381c8-4554-41f4-aefa-0f5d5a47f578
📒 Files selected for processing (3)
apps/mm2las/CMakeLists.txtapps/mm2las/main.cppdocs/source/app_mm2las.rst
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/mm2las/CMakeLists.txt
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #49 +/- ##
========================================
Coverage 78.59% 78.59%
========================================
Files 190 190
Lines 10569 10569
Branches 980 980
========================================
Hits 8307 8307
Misses 2262 2262 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/mm2las/main.cpp (1)
706-726: Deduplicate export-coordinate branching used in both loops.The same geodetic/map/ENU branching appears twice. A single helper (e.g.,
computeExportPoint(i)) will prevent bounds-vs-write divergence in future edits.Also applies to: 930-948
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mm2las/main.cpp` around lines 706 - 726, The coordinate-export branching for geodetic vs. cartesian/ENU is duplicated; extract it into a helper like computeExportPoint(size_t i) that returns or fills (px,py,pz) and use it from both loops to avoid future divergence. In computeExportPoint, call pointToGeodetic(i) when geodetic.has_value() to set px/py/pz, otherwise read xs[i], ys[i], zs[i] and, if T_enu_to_map.has_value(), call T_enu_to_map->composePoint(xs[i], ys[i], zs[i], px, py, pz). Replace the duplicated branches at both locations (the loop using pointToGeodetic and the later loop at lines ~930-948) with calls to computeExportPoint to centralize bounds and write logic.
🤖 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 295-297: The fast-path flag hasPerPointGeodetic is being set based
only on latBuf->empty(), which can lead to out-of-bounds reads when latBuf,
lonBuf, or altBuf are shorter than N; update the check that sets
hasPerPointGeodetic to ensure latBuf, lonBuf, and altBuf are non-null and each
has size() >= N (or the points count used for indexing), e.g. verify
latBuf->size() >= N && lonBuf->size() >= N && altBuf->size() >= N before
enabling the fast-path; if any buffer is too short, leave hasPerPointGeodetic
false and fall back to the safe per-point lookup.
- Around line 768-787: Add an explicit int32 quantization range check before
writing points: for each axis (use
header.x_scale_factor/header.y_scale_factor/header.z_scale_factor and
header.x_offset/header.y_offset/header.z_offset) compute quantized =
round((coord - offset) / scale) for the point set extrema (min/max coords) and
verify they fit within int32 bounds (INT32_MIN..INT32_MAX); if any axis
overflows, fail early (return error) or adjust/export with a coarser
scale/offset according to policy. Insert this guard immediately before the point
quantization/writing block (the code that currently uses header.*_scale_factor
and header.*_offset to turn coordinates into int32 values) so no silent
truncation occurs.
---
Nitpick comments:
In `@apps/mm2las/main.cpp`:
- Around line 706-726: The coordinate-export branching for geodetic vs.
cartesian/ENU is duplicated; extract it into a helper like
computeExportPoint(size_t i) that returns or fills (px,py,pz) and use it from
both loops to avoid future divergence. In computeExportPoint, call
pointToGeodetic(i) when geodetic.has_value() to set px/py/pz, otherwise read
xs[i], ys[i], zs[i] and, if T_enu_to_map.has_value(), call
T_enu_to_map->composePoint(xs[i], ys[i], zs[i], px, py, pz). Replace the
duplicated branches at both locations (the loop using pointToGeodetic and the
later loop at lines ~930-948) with calls to computeExportPoint to centralize
bounds and write logic.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/mm2las/main.cpp (1)
684-706: Avoid doing the geodetic conversion twice per point.When
!hasPerPointGeodetic,pointToGeodetic(i)runs once in the bounds pass and again in the write pass. On large clouds that doubles the expensive ENU→geocentric→geodetic path; caching the converted triplets for geodetic mode would avoid paying that cost twice.Also applies to: 708-718, 956-964
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mm2las/main.cpp` around lines 684 - 706, The pointToGeodetic lambda performs an expensive ENU→geocentric→geodetic conversion twice per point when hasPerPointGeodetic is false (once in the bounds pass and again in the write pass); modify the code to compute and cache geodetic coordinates for each point when hasPerPointGeodetic == false (e.g., allocate a vector<std::array<double,3>> geodeticCache sized to number of points and fill it using pointToGeodetic logic once using xs/ys/zs and geodetic->T_enu_to_map, then make pointToGeodetic (or call sites that use it) return values from geodeticCache instead of recomputing; keep existing behavior that reads directly from lonBuf/latBuf/altBuf when hasPerPointGeodetic is true.
🤖 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 212-217: The current saveToLas signature and preflight use
geodetic.has_value() both to indicate "export in geodetic mode" and to provide
ENU→geodetic context, which prevents the per-point latitude/longitude/altitude
fast path from being used when map-level mm.georeferencing is empty; change
saveToLas(const mrpt::maps::CPointsMap& pc, ..., const
std::optional<mp2p_icp::metric_map_t::Georeferencing>& geodetic = std::nullopt)
to accept an explicit export-frame flag/enum (e.g., requestedFrame or
exportInGeodetic bool/enum) separate from the optional Georeferencing context,
update callers to pass the desired frame, and alter the preflight checks so
Georeferencing is required only when requestedFrame indicates geodetic AND
per-point geodetic buffers are not present; update code paths in saveToLas, and
any related functions (references at lines ~272-311, 1161-1190, 1264-1266) to
perform on-the-fly conversion only when geodetic context is actually needed.
- Around line 819-825: The LASHeader_1_4 global_encoding bit for WKT is being
left set unconditionally while the WKT VLR (OGC WKT record 2112) is only written
when geodetic.has_value() is true; update the header write to set bit 4 (0x10)
only when geodetic.has_value() and clear it otherwise (i.e., modify
LASHeader_1_4.global_encoding around the VLR count/size logic where numVLRs and
vlrTotalBytes are updated) so the WKT flag matches whether you actually write
the WKT VLR record 2112.
---
Nitpick comments:
In `@apps/mm2las/main.cpp`:
- Around line 684-706: The pointToGeodetic lambda performs an expensive
ENU→geocentric→geodetic conversion twice per point when hasPerPointGeodetic is
false (once in the bounds pass and again in the write pass); modify the code to
compute and cache geodetic coordinates for each point when hasPerPointGeodetic
== false (e.g., allocate a vector<std::array<double,3>> geodeticCache sized to
number of points and fill it using pointToGeodetic logic once using xs/ys/zs and
geodetic->T_enu_to_map, then make pointToGeodetic (or call sites that use it)
return values from geodeticCache instead of recomputing; keep existing behavior
that reads directly from lonBuf/latBuf/altBuf when hasPerPointGeodetic is true.
d8bcfee to
ce0585b
Compare
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
New Features
Documentation