Conversation
📝 WalkthroughWalkthroughPoint cloud export workflows in mm2ply and mm2txt now validate requested export fields against actual fields in each layer, building filtered field lists before export. Missing fields trigger warnings and are skipped; errors only occur when the --ignore-missing-fields flag is absent. Documentation updated accordingly. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/mm2ply/main.cpp (3)
421-530:⚠️ Potential issue | 🔴 CriticalDefault export path is skipped when
--export-fieldsisn’t provided.
saveToPly()is only called insideif (!selectedFields.empty()), so the common case (no field selection) produces no output. Please add anelsebranch to preserve the default export behavior.💡 Proposed fix
for (const auto& [name, layer] : mm.layers) { auto* pts = mp2p_icp::MapToPointsMap(*layer); if (!pts) { continue; } // Validate selected fields exist in this point cloud if (!selectedFields.empty()) { ... saveToPly(*pts, out, arg_binary.getValue(), validFields); } + else + { + std::string out = prefix + "_" + name + ".ply"; + std::cout << "Exporting '" << name << "' to " << out << "..." << std::endl; + saveToPly(*pts, out, arg_binary.getValue()); + } }
439-529:⚠️ Potential issue | 🟠 MajorWhen all requested fields are missing, you end up exporting all fields.
If every requested field is missing and
--ignore-missing-fieldsis set,validFieldsstays empty andsaveToPly()interprets that as “export all fields.” That contradicts the user’s selection intent. Consider skipping the layer (or emitting an empty-header file) whenvalidFieldsis empty butselectedFieldswas explicit.💡 Suggested guard
for (const auto& field : selectedFields) { ... else { validFields.push_back(field); } } + if (validFields.empty()) + { + std::cerr << "Warning: None of the requested fields exist in layer '" << name + << "'. Skipping export for this layer." << std::endl; + continue; + } std::string out = prefix + "_" + name + ".ply"; std::cout << "Exporting '" << name << "' to " << out << "..." << std::endl; saveToPly(*pts, out, arg_binary.getValue(), validFields);
49-53:⚠️ Potential issue | 🟡 MinorCLI help text still claims missing fields are padded with zeros.
The
--ignore-missing-fieldsdescription is now stale; behavior is “warn and skip,” not “pad with zeros.”✏️ Proposed wording update
TCLAP::SwitchArg argIgnoreMissingFields( "", "ignore-missing-fields", "If defined, the lack of any of the --export-fields in the map will be considered a warning " - "instead of an error, and that column will be padded with zeros.", + "instead of an error; missing fields are skipped.", cmd);apps/mm2txt/main.cpp (2)
299-362:⚠️ Potential issue | 🟠 MajorAll-missing field selection falls back to exporting all fields.
If
selectedFieldsis explicit but none exist (and--ignore-missing-fieldsis set),validFieldsbecomes empty andsaveToTxt()exports all fields by design. That undermines the user’s selection intent. Consider skipping the layer (or outputting an empty-header file) whenvalidFieldsis empty but a selection was provided.💡 Suggested guard
if (!selectedFields.empty()) { ... for (const auto& field : selectedFields) { ... else { validFields.push_back(field); } } + if (validFields.empty()) + { + std::cerr << "Warning: None of the requested fields exist in layer '" << name + << "'. Skipping export for this layer." << std::endl; + continue; + } } else { validFields = selectedFields; } bool printHeader = true; saveToTxt(*genxyz, filName, printHeader, validFields);
57-61:⚠️ Potential issue | 🟡 MinorCLI help text still mentions “padded with zeros.”
The current behavior is to skip missing fields; the help text should match.
✏️ Proposed wording update
TCLAP::SwitchArg argIgnoreMissingFields( "", "ignore-missing-fields", "If defined, the lack of any of the --export-fields in the map will be considered a warning " - "instead of an error, and that column will be padded with zeros.", + "instead of an error; missing fields are skipped.", cmd);
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #36 +/- ##
========================================
Coverage 77.03% 77.03%
========================================
Files 179 179
Lines 9526 9526
Branches 917 917
========================================
Hits 7338 7338
Misses 2188 2188 🚀 New features to boost your workflow:
|
jlblancoc
force-pushed
the
feat/mm2txt-missing-fields-dont-emit
branch
from
d47ad05 to
8700cb7
Compare
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)
319-346:⚠️ Potential issue | 🟡 MinorAvoid “skipping” warning when an error is thrown.
When
--ignore-missing-fieldsis not set, the code throws, so “skipping this field” is misleading. Consider branching the message based on the flag.📝 Suggested message branching
- std::cerr << " - skipping this field." << std::endl; - if (!argIgnoreMissingFields.isSet()) - { - THROW_EXCEPTION_FMT( - "Field '%s' specified in --export-fields not found in layer '%s'", - field.c_str(), name.c_str()); - } + if (argIgnoreMissingFields.isSet()) + { + std::cerr << " - skipping this field." << std::endl; + } + else + { + std::cerr << " - aborting export." << std::endl; + THROW_EXCEPTION_FMT( + "Field '%s' specified in --export-fields not found in layer '%s'", + field.c_str(), name.c_str()); + }
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary by CodeRabbit
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.