fix: stream points instead of buffering the whole cloud in memory

[pnodet]

Problem

main() buffered the entire point cloud into a Vec<las::Point> while manually calling header.add_point(&point) on a header round-tripped through las::Builder, then wrote the buffered points in a second pass. LAS files are routinely multi-GB, so buffering the whole cloud in memory is a real OOM risk.

Why the manual header pass was dead work

Verified against the las 0.8.8 crate source:

• Writer::new calls header.clear() on the header you pass it, zeroing point counts, per-return counts, and bounds — so everything the manual add_point loop accumulated was discarded before the first point was written.
• Every writer.write(point) internally calls header.add_point itself.
• writer.close() seeks back and rewrites the finished header with the recomputed counts and bounds.

The Builder::from(header).into_header() round-trip and the Vec buffer existed solely to support that dead pass.

Change

Rewrite main() as a single streaming read -> write pass: clone the reader's header, hand it to Writer::from_path, and write each point as it is read. Memory usage is now constant regardless of file size, and the unused Builder import is removed (the Read/Write traits stay — they provide points()/write()).

Output is behaviorally equivalent: the writer produces the same recomputed header (counts, per-return counts, bounds) and the same point stream; invalid points are still silently skipped.

cargo build and cargo clippy pass with no warnings.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Improved LAS file processing to write output progressively instead of buffering all points first.
  • Better handling of malformed point records by skipping invalid entries and continuing the conversion.
  • Updated error messages for clearer feedback when reading, writing, or closing files fails.

[greptile-apps]

Your trial has ended. Reactivate Greptile to resume code reviews.

[coderabbitai]

Walkthrough

The LAS point-processing logic in src/main.rs is refactored from a buffered approach that rebuilt the header and collected all points into a vector, to a streaming approach that clones the reader header, creates a writer directly, and writes points one at a time as they are read.

Changes

Streaming LAS Processing

Layer / File(s)	Summary
Streaming writer flow src/main.rs	Imports drop las::Builder and add las::Read; main logic clones the reader header to create a writer, streams points via reader.points(), writes each Ok(point) with updated error context, skips Err results, and closes the writer with a context message, replacing the prior vector-accumulation approach.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Main
  participant Reader
  participant Writer

  Main->>Reader: clone header
  Main->>Writer: create writer from header
  loop for each point in reader.points()
    Reader-->>Main: Ok(point) or Err
    alt Ok(point)
      Main->>Writer: write point
    else Err
      Main->>Main: skip point
    end
  end
  Main->>Writer: close writer

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly captures the main change: switching from buffering LAS points to streaming them in memory.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/stream-points-remove-buffering

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

src/main.rs (1)

22-27: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Consider logging skipped invalid points.

Invalid points are silently dropped via Err(_) => continue, matching the prior behavior, but with no visibility into how many/which points were skipped. For multi-GB files this could mask a systemic read issue (e.g., truncated file, corrupt VLR) without any trace in the output.

♻️ Optional: surface a warning per skipped point

-            Err(_) => continue,
+            Err(e) => {
+                eprintln!("Warning: skipping invalid point: {e}");
+                continue;
+            }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/main.rs` around lines 22 - 27, The point-processing loop in main
currently drops reader.points() errors silently via Err(_) => continue, so add
visibility for skipped invalid points while keeping the same behavior. Update
the match in the reader.points() iteration to log a warning or otherwise record
each skipped point, referencing the point-processing logic in main and the
writer.write path so it’s easy to locate even if lines move. If you want to
avoid noisy output, at minimum count skipped points and emit a summary warning
after the loop.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/main.rs`:
- Around line 22-27: The point-processing loop in main currently drops
reader.points() errors silently via Err(_) => continue, so add visibility for
skipped invalid points while keeping the same behavior. Update the match in the
reader.points() iteration to log a warning or otherwise record each skipped
point, referencing the point-processing logic in main and the writer.write path
so it’s easy to locate even if lines move. If you want to avoid noisy output, at
minimum count skipped points and emit a summary warning after the loop.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b8d525b5-7458-4a3b-82d8-6b763f93e093

📥 Commits

Reviewing files that changed from the base of the PR and between e20b82b and 66be33b.

📒 Files selected for processing (1)

• src/main.rs

[pnodet]

Thermo-nuclear code quality review

Verdict: approves the bar. No major findings.

This is the rare PR where the diff itself is the code-judo move. The old main() carried four pieces of incidental complexity — a Builder::from(header).into_header() round-trip, a manual header.add_point accumulation pass, a full in-memory Vec<las::Point> buffer, and a second write loop — and all four were dead weight given the actual las 0.8.8 writer contract (Writer::new clears the header, write() re-accumulates counts/bounds, close() rewrites the header). The rewrite doesn't rearrange that complexity; it deletes it. The result is a single streaming pass that reads as the inevitable shape of this program: open reader, clone header, open writer, stream points, close. Constant memory, same output. Verified locally: cargo build and cargo clippy pass with no warnings.

Structural checklist against the bar:

• No file-size or decomposition concerns — src/main.rs is 32 lines and cohesive.
• No new branches, flags, or modes; branching strictly decreased.
• No wrapper/cast/optionality churn; the Builder indirection was removed, not replaced.
• Type facts treated as ground truth were verified against the crate: passing reader.header().clone() straight to the writer is output-equivalent to the old manual pass.

One deliberate non-finding: reader.points().flatten() would collapse the match into a one-line loop body, but PR #2 needs the Err arm back for dropped-point reporting, so the explicit match is the right resting shape given the sibling PRs. Not worth churning here.

Merge-order notes (overlap with sibling PRs — intentionally not pulled into this diff):

• Silent Err(_) => continue point-dropping is pre-existing behavior, addressed by PR feat: report kept and dropped point counts #2.
• The unused uuid dependency in Cargo.toml is PR chore: remove unused uuid dependency #3.
• The output default of "./" (a directory, which Writer::from_path will reject) and the String-instead-of-PathBuf arg types are PR fix: replace broken "./" output default with a derived output path #4. Whichever of feat: report kept and dropped point counts #2/fix: replace broken "./" output default with a derived output path #4 lands second will need a trivial rebase over this loop body.

Approved. This is what a fix PR should look like: smaller, simpler, and more correct than what it replaced.