mm2txt and mm2ply now have a --export-fields flag#24
Conversation
📝 WalkthroughWalkthroughAdds a new --export-fields CLI option to mm2ply and mm2txt, implements field-selection parsing/validation, propagates selected fields through per-layer export, updates writer routines to emit dynamic headers and type-aware field values, and expands documentation and examples. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant App as mm2ply/mm2txt
participant Layer as LayerProcessor
participant Writer as saveToPly/saveToTxt
participant FS as Filesystem
CLI->>App: parse args (including --export-fields)
App->>App: parse & normalize selectedFields
App->>Layer: iterate map layers -> validate selectedFields
Layer-->>App: validation result (ok / missing)
alt validation OK
App->>Writer: call writer(layerData, filename, binary, selectedFields)
Writer->>FS: open file
Writer->>Writer: generate dynamic header based on selectedFields
Writer->>Writer: for each point -> write per-field values (type-aware)
Writer->>FS: close file
else validation failed
App->>CLI: emit warnings and throw/abort for invalid fields
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes 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
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: 0
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)
336-376: Compilation error on MRPT 3.0.0+: orphanelseclause.When
MRPT_VERSION >= 0x030000, the preprocessor removes lines 337-364, leaving theelseon line 365 without a matchingif. This will cause a syntax error.🐛 Proposed fix
The
elseblock should be inside the#ifpreprocessor directive, or the logic needs restructuring:#if MRPT_VERSION < 0x030000 // <3.0.0 else { #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wdeprecated-declarations" if (auto* xyzirt = dynamic_cast<const mrpt::maps::CPointsMapXYZIRT*>(pts); xyzirt) { // ... xyzirt handling ... xyzirt->saveXYZIRT_to_text_file(filName); } else if (auto* xyzi = dynamic_cast<const mrpt::maps::CPointsMapXYZI*>(pts); xyzi) { // ... xyzi handling ... xyzi->saveXYZI_to_text_file(filName); } -#pragma GCC diagnostic pop -#endif else { if (!selectedFields.empty()) { std::cerr << "Warning: --export-fields not supported for this point cloud type, " "exporting all fields." << std::endl; } pts->save3D_to_text_file(filName); } +#pragma GCC diagnostic pop } +#endif } }
🧹 Nitpick comments (6)
apps/mm2ply/README.md (1)
77-79: Add language identifier to code block for consistency.The output section code block is missing a language identifier. For consistency with other blocks in this file, wrap the example in a language-specific tag (e.g.,
```textor```bash).Proposed fix
## Output The tool creates separate PLY files for each point cloud layer in the metric map. Files are named using the pattern: -``` +```text <prefix>_<layer_name>.ply -``` +```textapps/mm2txt/main.cpp (2)
116-118: Simplify string formatting.The
%.*sformat with explicit length is unnecessarily complex forstd::string. Using%swithc_str()is cleaner and more idiomatic.♻️ Suggested simplification
- mrpt::system::os::fprintf( - f, "%.*s%s", static_cast<int>(fieldName.length()), fieldName.data(), - (i + 1 < fieldsToExport.size()) ? " " : ""); + mrpt::system::os::fprintf( + f, "%s%s", fieldName.c_str(), + (i + 1 < fieldsToExport.size()) ? " " : "");
177-181: Consider logging a warning for the unreachable fallback.If this code path is reached, it indicates a validation bug. Silently printing "0" could mask data integrity issues. Consider adding a warning or assertion.
♻️ Suggested improvement
else { - // Field not found - this should have been caught earlier - mrpt::system::os::fprintf(f, "0"); + // Field not found - this should have been caught earlier + std::cerr << "Warning: Field '" << fieldName << "' not found during export, using 0\n"; + mrpt::system::os::fprintf(f, "0"); }apps/mm2ply/main.cpp (3)
126-163: Default type fallback may produce incorrect PLY headers.The default
return "float"on line 162 is reached for unknown fields. While validation should prevent this, consider throwing an exception for consistency with the accessor building logic (line 260).♻️ Suggested improvement
#endif - return "float"; // default + throw std::runtime_error("Unknown field type for: " + fieldName); };
283-315: Missingdefaultcase in switch may trigger compiler warnings.The switch statement doesn't have a
defaultcase. While the accessor building logic should prevent unknown types, adefaultcase improves robustness and may silence compiler warnings about unhandled enum values.♻️ Suggested improvement
case FieldAccessor::UINT8: { const auto& buf = *static_cast<const mrpt::aligned_std_vector<uint8_t>*>(acc.bufPtr); write_val(buf[i]); break; } #endif + default: + throw std::runtime_error("Unexpected field type for: " + acc.name); }
381-470: Consider extracting field validation into a shared helper.The field validation logic is duplicated between main() and the accessor building in saveToPly(). Extracting this into a shared helper function would reduce duplication and ensure consistent behavior.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/mm2ply/README.mdapps/mm2ply/main.cppapps/mm2txt/README.mdapps/mm2txt/main.cppdocs/source/app_mm2ply.rstdocs/source/app_mm2txt.rst
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
apps/mm2ply/README.md
94-94: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
103-103: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build_docker (ubuntu:jammy, humble, false)
- GitHub Check: build_docker (ubuntu:noble, jazzy, false)
- GitHub Check: build_docker (ubuntu:noble, rolling, true)
- GitHub Check: build_docker (ubuntu:noble, jazzy, true)
- GitHub Check: build_docker (ubuntu:jammy, humble, true)
- GitHub Check: build_docker (ubuntu:noble, rolling, true, true)
- GitHub Check: build_docker (ubuntu:noble, rolling, true, true)
🔇 Additional comments (13)
apps/mm2ply/README.md (1)
85-94: Document color field mapping behavior in Field Selection section.The documentation mentions automatic color field mapping in the Field Selection section but doesn't clearly explain what happens when users request unmapped names. Clarify whether users should request
color_rorredwhen using--export-fields, and what the tool does if they request the old name.The code needs to clarify: Does the export validation check against internal field names (
color_r) or output names (red)? Should users specify the input or output field name?apps/mm2txt/README.md (1)
110-120: Documentation of field selection behavior and limitations is clear and comprehensive.The Field Selection section effectively explains the feature's behavior, including the important limitation that
--export-fieldsis only supported for CGenericPointsMap with explicit warnings for legacy types. This sets clear expectations for users.Verify that the implementation correctly:
- Validates field existence and reports available fields when a requested field is not found
- Preserves field order exactly as specified by the user
- Issues appropriate warnings for legacy point cloud types when
--export-fieldsis provideddocs/source/app_mm2txt.rst (3)
116-126: Field Selection section provides clear guidance on option usage and validation.The documentation effectively communicates the feature's behavior: comma-separated lists with optional spacing, exact order preservation, field existence validation, and the important caveat about legacy point cloud type support. This will help users understand both capabilities and limitations.
144-146: Clear documentation of per-type feature support.The explicit marking of
--export-fieldssupport in the Supported Point Cloud Types section (full support for CGenericPointsMap; not supported for legacy types) sets clear expectations and prevents user confusion.
1-146: Implementation verified and matches documentation.The C++ implementations in
apps/mm2txt/main.cppandapps/mm2ply/main.cppcorrectly implement all documented behaviors for the--export-fieldsfeature:
--export-fieldsargument parsing is properly implemented in both tools- Field validation logic correctly throws errors when specified fields don't exist
- Field selection and ordering logic preserves the user-specified order via the
selectedFieldsvector- Function signatures (
saveToPly()andsaveToTxt()) accept theselectedFieldsparameter as expected- Warnings for unsupported types (CPointsMapXYZI, CPointsMapXYZIRT) are present in mm2txt as documented
- Default behavior exports all fields when
--export-fieldsis not specifiedThe documentation across all three files is thorough, consistent, and accurately reflected in the implementation.
apps/mm2txt/main.cpp (3)
50-55: LGTM! Clean CLI argument definition.The new
--export-fieldsargument is well-documented and follows the existing pattern for TCLAP arguments in this file.
284-331: Good validation with helpful error messages.The field validation correctly checks all field types, provides informative error messages listing available fields, and properly handles MRPT version differences.
227-259: LGTM! Robust field parsing.The tokenization handles various delimiters well, filters empty tokens, validates that at least one field was specified, and provides clear user feedback.
apps/mm2ply/main.cpp (2)
35-40: LGTM! Consistent CLI argument definition.The
--export-fieldsargument matches the definition in mm2txt, providing a consistent user experience across both tools.
176-264: Well-designed field accessor pattern.The
FieldAccessorstruct with type enumeration and buffer pointer provides a clean, extensible approach for handling multiple field types. The accessor building logic is comprehensive and handles version-specific fields appropriately.docs/source/app_mm2ply.rst (3)
16-16: LGTM! Good feature documentation.The new "Selectively exports specific fields in a custom order" feature is clearly documented in the features list.
58-80: Excellent examples covering various use cases.The examples progress well from basic field selection to combining with binary output and custom prefixes. The variety of field combinations demonstrates the flexibility of the feature.
96-107: Clear documentation of field selection behavior.The Field Selection section accurately describes the comma-separated format, space handling, ordering guarantees, and error behavior, all matching the implementation.
jlblancoc
force-pushed
the
feat/mm2txt-select-fields
branch
from
20cccd6 to
aad9adf
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @apps/mm2ply/main.cpp:
- Around line 281-316: The switch on acc.type in the loop over accessors
(FieldAccessor::FLOAT/DOUBLE/UINT16/UINT8) lacks a default branch; add a default
case to the switch on acc.type that handles unexpected enum values (e.g., log an
error or throw/skip) to silence compiler warnings and guard against future enum
extensions—update the switch in the loop that iterates accessors (using acc.type
and acc.bufPtr and calling write_val(buf[i])) to include a default which reports
the unhandled FieldAccessor value and safely continues or aborts as appropriate.
In @apps/mm2txt/main.cpp:
- Around line 261-268: The warning string inside the printSelectedFieldsWarning
lambda incorrectly references only CPointsMapXYZIRT while the lambda is used for
CPointsMapXYZI and the generic save3D_to_text_file fallback as well; update the
message in printSelectedFieldsWarning to a generic phrase (e.g.,
"--export-fields not supported for this point map type, exporting all fields")
so it correctly applies whenever selectedFields is non-empty and the lambda is
invoked for CPointsMapXYZIRT, CPointsMapXYZI, or the generic save3D_to_text_file
path.
- Around line 177-181: Replace the silent fallback that writes "0" (the
mrpt::system::os::fprintf(f, "0"); in the "Field not found" else branch) with an
explicit failure: throw a std::runtime_error or std::logic_error describing the
missing field and that validation should have caught it (include the field
identifier/variable if available, e.g., fieldName or similar), and/or emit a
clear error via the project's logging facility before aborting; remember to add
the necessary <stdexcept> include if you throw. This makes the logic error
visible instead of masking it.
In @apps/mm2txt/README.md:
- Around line 77-79: The fenced code blocks showing output examples (the ones
containing "<input_filename>_<layer_name>.txt", the block starting with "x y z
intensity ring time" and the block starting with "x y z intensity") are missing
language specifiers; update each opening triple-backtick to include "text"
(e.g., ```text) so the examples are treated as plain text in the README and
satisfy the static analysis tool.
🧹 Nitpick comments (3)
apps/mm2txt/main.cpp (2)
113-118: Consider simplifying the header field printing.The current format string with
static_cast<int>(fieldName.length())is overly complex for printing a simple string. This can be simplified:♻️ Suggested simplification
for (size_t i = 0; i < fieldsToExport.size(); i++) { const auto& fieldName = fieldsToExport[i]; - mrpt::system::os::fprintf( - f, "%.*s%s", static_cast<int>(fieldName.length()), fieldName.data(), - (i + 1 < fieldsToExport.size()) ? " " : ""); + mrpt::system::os::fprintf( + f, "%s%s", fieldName.c_str(), + (i + 1 < fieldsToExport.size()) ? " " : ""); }
155-175: Double map lookup can be avoided.Using
.count()followed by.at()performs two lookups. Consider using.find()once and reusing the iterator, or accepting the current approach for readability since the number of fields is typically small.♻️ Example using find()
- else if (floatFields.count(fieldName)) - { - mrpt::system::os::fprintf(f, "%f", floatFields.at(fieldName).at(i)); - } + else if (auto it = floatFields.find(fieldName); it != floatFields.end()) + { + mrpt::system::os::fprintf(f, "%f", it->second.at(i)); + }apps/mm2ply/main.cpp (1)
381-470: Field validation is duplicated between main() and saveToPly().The validation in
main()(lines 381-470) largely duplicates the field discovery logic insaveToPly()(lines 192-264). Consider extracting a shared helper function that validates field existence, though this is acceptable for now given the separate compilation contexts.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/mm2ply/README.mdapps/mm2ply/main.cppapps/mm2txt/README.mdapps/mm2txt/main.cppdocs/source/app_mm2ply.rstdocs/source/app_mm2txt.rst
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/mm2ply/README.md
- docs/source/app_mm2txt.rst
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
apps/mm2txt/README.md
77-77: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
94-94: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
103-103: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build_docker (ubuntu:noble, rolling, true, true)
- GitHub Check: build_docker (ubuntu:jammy, humble, false)
- GitHub Check: build_docker (ubuntu:noble, rolling, true, true)
- GitHub Check: build_docker (ubuntu:jammy, humble, true)
- GitHub Check: build_docker (ubuntu:noble, jazzy, true)
- GitHub Check: build_docker (ubuntu:noble, jazzy, false)
- GitHub Check: build_docker (ubuntu:noble, rolling, true)
🔇 Additional comments (9)
apps/mm2txt/main.cpp (2)
50-55: LGTM - CLI option well-defined.The
--export-fieldsargument is properly configured with clear help text describing the comma-separated format and ordering semantics.
293-340: Field validation logic is thorough and well-implemented.The validation correctly checks x/y/z coordinates plus all field type maps, provides helpful error messages listing available fields, and throws a clear exception when a field is missing.
apps/mm2txt/README.md (1)
1-137: Documentation is comprehensive and well-organized.The README covers all key aspects: features, usage, arguments, examples, output format, field selection behavior, and supported types. The examples demonstrate various use cases effectively.
apps/mm2ply/main.cpp (3)
35-40: LGTM - CLI option consistent with mm2txt.The
--export-fieldsargument definition matches mm2txt, ensuring consistent CLI experience across tools.
176-188: FieldAccessor design is clean and extensible.The struct with enum type discriminator and void pointer is a practical approach for handling heterogeneous field types without complex template machinery.
258-261: Exception thrown for unknown field provides clear feedback.This correctly handles the case where a field passes validation but somehow isn't found during accessor building - good defensive programming.
docs/source/app_mm2ply.rst (3)
16-20: Documentation accurately reflects the new feature.The features list and usage examples correctly document the
--export-fieldsoption, including the comma-separated syntax with optional spaces.
96-107: Field selection rules are clearly documented.The documentation clearly explains the behavior: comma-separated format, space tolerance, ordering preservation, validation with helpful error messages, and color field mapping. This matches the implementation.
108-120: Good detail on MRPT version requirements.The documentation appropriately notes which field types require MRPT ≥ 2.15.3, helping users understand compatibility constraints.
| ``` | ||
| <input_filename>_<layer_name>.txt | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to fenced code blocks.
The static analysis tool flagged missing language specifiers. For output examples, use text or plaintext:
🔧 Suggested fix for line 77
-```
+```text
<input_filename>_<layer_name>.txt🔧 Suggested fix for line 94
-```
+```text
x y z intensity ring time
1.234 5.678 0.123 45.6 0 12345.678🔧 Suggested fix for line 103
-```
+```text
x y z intensity
1.234 5.678 0.123 45.6Also applies to: 94-99, 103-108
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
77-77: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In @apps/mm2txt/README.md around lines 77 - 79, The fenced code blocks showing
output examples (the ones containing "<input_filename>_<layer_name>.txt", the
block starting with "x y z intensity ring time" and the block starting with "x y
z intensity") are missing language specifiers; update each opening
triple-backtick to include "text" (e.g., ```text) so the examples are treated as
plain text in the README and satisfy the static analysis tool.
jlblancoc
force-pushed
the
feat/mm2txt-select-fields
branch
from
aad9adf to
5382ddb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/mm2txt/main.cpp (1)
177-181: Consider whether this fallback is necessary.The fallback case that prints "0" for unknown fields should theoretically never be reached due to the validation at lines 293-340. You might consider either:
- Throwing an exception here instead of silently printing "0" (defensive fail-fast)
- Adding an assertion that this case is unreachable
- Keeping as-is for maximum defensive programming
The current approach is safe but may mask bugs if validation logic has gaps.
apps/mm2txt/README.md (3)
77-77: Add language specifier to fenced code block.For proper syntax highlighting and rendering, add a language identifier (e.g.,
text) to the fenced code block:-``` +```text <input_filename>_<layer_name>.txt ```Based on static analysis hints.
94-99: Add language specifier to fenced code block.For better rendering, add a language identifier (e.g.,
text) to the fenced code block:-``` +```text x y z intensity ring time 1.234 5.678 0.123 45.6 0 12345.678 2.345 6.789 0.234 67.8 1 12345.679 ... ```Based on static analysis hints.
103-108: Add language specifier to fenced code block.For better rendering, add a language identifier (e.g.,
text) to the fenced code block:-``` +```text x y z intensity 1.234 5.678 0.123 45.6 2.345 6.789 0.234 67.8 ... ```Based on static analysis hints.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/mm2ply/README.mdapps/mm2ply/main.cppapps/mm2txt/README.mdapps/mm2txt/main.cppdocs/source/app_mm2ply.rstdocs/source/app_mm2txt.rst
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
apps/mm2txt/README.md
77-77: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
94-94: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
103-103: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build_docker (ubuntu:noble, rolling, true, true)
- GitHub Check: build_docker (ubuntu:jammy, humble, false)
- GitHub Check: build_docker (ubuntu:noble, rolling, true, true)
- GitHub Check: build_docker (ubuntu:noble, jazzy, true)
- GitHub Check: build_docker (ubuntu:jammy, humble, true)
- GitHub Check: build_docker (ubuntu:noble, jazzy, false)
- GitHub Check: build_docker (ubuntu:noble, rolling, true)
🔇 Additional comments (6)
apps/mm2txt/main.cpp (2)
233-242: Token parsing handles edge cases well.The tokenization by
", \t"combined with empty token filtering correctly handles various input formats including extra spaces, trailing commas, and multiple delimiters. Good defensive programming.
293-340: Comprehensive field validation with excellent user feedback.The validation logic thoroughly checks all field types and provides helpful error messages listing available fields when a requested field is not found. This will significantly improve the user experience.
apps/mm2ply/main.cpp (3)
176-264: Excellent use of the FieldAccessor pattern.The FieldAccessor structure pre-computes field access patterns, making the data writing loop much more efficient for large point clouds. This is cleaner than repeated conditional checks per point and ensures type safety through the enum-based approach.
281-319: Clean type-safe data writing with proper error handling.The switch-based type handling with immediate exceptions for unknown types is a good fail-fast approach. This is actually superior to the defensive "print 0" fallback in mm2txt.
384-473: Comprehensive field validation with helpful error messages.The validation logic matches mm2txt's approach and provides excellent user feedback by listing available fields when validation fails.
apps/mm2ply/README.md (1)
12-12: Documentation for --export-fields feature is well-structured and comprehensive.The additions clearly explain the new functionality with:
- Feature introduction (line 12) that fits the existing feature list
- Usage syntax (line 25) showing the new flag in standard argument format
- Argument documentation (line 33) explaining format, behavior, and space-handling flexibility
- Progressive examples (lines 55–71) demonstrating basic usage, binary+fields, and minimalist 2D+intensity combinations
- Field Selection section (lines 85–94) explaining validation, ordering behavior, and color field mapping
- Expanded field inventory (lines 102–107) with type details and MRPT version requirements
The separation between the "Field Selection" section (which explains how to use the feature) and "Supported Point Fields" section (which lists available fields with implementation details like color field variants) appropriately balances ease of use with detailed reference information.
Also applies to: 25-25, 33-33, 55-94, 102-107
1 participant
Summary by CodeRabbit
New Features
Bug Fixes / UX
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.