Skip to content

Use ZStd compression by default (for MRPT>=2.15.7)#37

Merged
jlblancoc merged 5 commits intodevelopfrom
feat/use-zstd
Feb 3, 2026
Merged

Use ZStd compression by default (for MRPT>=2.15.7)#37
jlblancoc merged 5 commits intodevelopfrom
feat/use-zstd

Conversation

@jlblancoc
Copy link
Copy Markdown
Member

@jlblancoc jlblancoc commented Feb 2, 2026

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Added error validation for file save operations; failures are now reported with error messages instead of silently failing.
  • New Features

    • Added timing measurements for map loading and reading operations, displayed in output.
    • Added compression method configuration option for saving metric map files (supported in newer MRPT versions).
  • Chores

    • Updated codebase for compatibility with newer MRPT library versions.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Feb 2, 2026

Warning

Rate limit exceeded

@jlblancoc has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 36 minutes and 37 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📝 Walkthrough

Walkthrough

This PR introduces MRPT version-gated compression support across the codebase, enabling newer CCompressedInputStream/CCompressedOutputStream APIs (MRPT >= 0x020f07) while maintaining backward compatibility with legacy CFileGZInputStream/CFileGZOutputStream. Command-line applications gain error handling for save operations and timing instrumentation for map loading.

Changes

Cohort / File(s) Summary
Error handling in save operations
apps/icp-log-viewer/main.cpp, apps/mm-georef/main.cpp
Added return value checks for save_to_file() with error messages on failure.
Formatting and structural updates
apps/kitti2mm/main.cpp
Wrapped single-line if blocks with braces for consistency without altering control flow.
Timing instrumentation
apps/mm-info/main.cpp, apps/mm2txt/main.cpp
Added elapsed time measurement and reporting around map loading operations.
Version-gated compression in command-line tools
apps/rawlog-filter/main.cpp
Conditional stream selection using CCompressedOutputStream (MRPT >= 0x020f07) or CFileGZOutputStream, with version-based header includes.
Compression method CLI option
apps/sm2mm/main.cpp
Added compression-method CLI argument for MRPT >= 0x020f07; integrated timing instrumentation and conditional compression options during save.
LogRecord compression API updates
mp2p_icp/include/mp2p_icp/LogRecord.h, mp2p_icp/src/LogRecord.cpp
Removed explicit destructor; added overloaded save_to_file() with optional CompressionOptions parameter (MRPT >= 0x020f07); updated I/O streams with version-gated conditional compilation.
metric_map_t compression API updates
mp2p_icp_map/include/mp2p_icp/metricmap.h, mp2p_icp_map/src/metricmap.cpp
Extended save_to_file() and load_from_file() with version-gated compressed I/O streams and added CompressionOptions parameter overload (MRPT >= 0x020f07).
Version-gated I/O in map loading
mp2p_icp_map/src/load_xyz_file.cpp
Introduced conditional stream selection between CCompressedInputStream (MRPT >= 0x020f07) and CFileGZInputStream for gzip file handling.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • Feat/sm2mm-new-options #33: Modifies CLI structure in apps/sm2mm/main.cpp; the current PR adds the arg_compression_method field to this same CLI struct, indicating direct code-level dependency.

Poem

🐰 Across the codebase hops a change so grand,
Compression springs forth with MRPT's command,
Version-gated paths both new and old,
Files saved faster than a rabbit can scold!
Errors are caught, times are measured with care. ⏱️✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 26.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately describes the main change: introducing ZStd compression as the default for MRPT versions 2.15.7 and above, which is the primary objective across multiple files.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/use-zstd

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
mp2p_icp/src/LogRecord.cpp (1)

149-153: ⚠️ Potential issue | 🟡 Minor

Copy-paste error in error message.

The error message incorrectly references save_to_file when this is the load_from_file function.

Proposed fix
     catch (const std::exception& e)
     {
-        std::cerr << "[LogRecord::save_to_file] Error: " << e.what();
+        std::cerr << "[LogRecord::load_from_file] Error: " << e.what();
         return false;
     }
🤖 Fix all issues with AI agents
In `@apps/sm2mm/main.cpp`:
- Around line 74-85: The CLI help/default for arg_compression_method uses scoped
enum names like "CompressionType::Zstd" but
TEnumType<CompressionType>::name2value() expects unscoped names (e.g., "Zstd"),
causing parse failures; update the TCLAP::ValueArg<std::string>
arg_compression_method to use unscoped names in both the help/options list and
the default (e.g., "Zstd", "Gzip", "None") or alternatively normalize the input
before parsing by stripping any "CompressionType::" prefix when resolving via
TEnumType<CompressionType>::name2value(); adjust the help text to list the
unscoped enum identifiers to match the parser.
🧹 Nitpick comments (2)
mp2p_icp_map/src/metricmap.cpp (1)

630-642: Consider catching serialization exceptions to honor the bool contract.
save_to_file() now returns a bool that callers check; uncaught exceptions will still abort the caller. Align with LogRecord::save_to_file() by returning false on exceptions.

Suggested change (guard serialization and return false on errors)
 bool metric_map_t::save_to_file(
 `#if` MRPT_VERSION >= 0x020f07
     const std::string& fileName, const mrpt::io::CompressionOptions& co
 `#else`
     const std::string& fileName
 `#endif`
 ) const
 {
 `#if` MRPT_VERSION >= 0x020f07
     auto f = mrpt::io::CCompressedOutputStream(fileName, mrpt::io::OpenMode::TRUNCATE, co);
 `#else`
     auto f = mrpt::io::CFileGZOutputStream(fileName);
 `#endif`
     if (!f.is_open())
     {
         return false;
     }
 
-    auto arch = mrpt::serialization::archiveFrom(f);
-    arch << *this;
+    try
+    {
+        auto arch = mrpt::serialization::archiveFrom(f);
+        arch << *this;
+    }
+    catch (const std::exception& e)
+    {
+        std::cerr << "[metric_map_t::save_to_file] Error: " << e.what();
+        return false;
+    }
 
     return true;
 }
apps/sm2mm/main.cpp (1)

137-145: Avoid relying on transitive includes for mrpt::Clock.
Please verify that a direct header provides mrpt::Clock::nowDouble(); if not, include the appropriate MRPT header explicitly to keep this compile‑safe.

Comment thread apps/sm2mm/main.cpp
@jlblancoc jlblancoc enabled auto-merge February 3, 2026 00:11
@jlblancoc jlblancoc disabled auto-merge February 3, 2026 00:21
@jlblancoc jlblancoc merged commit 2bfd9a8 into develop Feb 3, 2026
10 of 13 checks passed
@jlblancoc jlblancoc deleted the feat/use-zstd branch February 3, 2026 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant