[alteralto]

Summary

A follow-up to #839, from the review note on bounding the SVG output. Each picture in an SVG layout export is limited to the page's pixel budget on its own, but the document keeps all of them, so a page with many large or stacked map frames could grow without bound.

layout_svg now adds up the on-page area of the elements it draws as pictures. When they would take more than twice the page's pixel budget (MAX_PIXELS), all of them are drawn at the resolution that fits, the way an oversized PNG page is drawn at a lower resolution rather than refused. Twice, because legends, scale bars and insets usually sit on a map frame: an ordinary layout keeps the resolution it has now, even on an A0 page already at the limit.

Testing

• surrounds_on_a_frame_keep_the_picture_resolution: a legend on a frame at 600 dpi, and two frames filling an A0 page at its pixel limit, keep 600 dpi
• stacked_frames_share_the_picture_budget: eight full-page frames on A0 get a lower resolution that uses (and stays within) twice the page's budget; frames moved off the page take nothing from it
• cargo fmt --all --check, cargo clippy --workspace --all-targets -D warnings (with collapsible_match allowed: my clippy 1.95 flags it in crates/ui-egui/src/opendataui.rs on main), cargo test -p rustgis-render (188 passed), cargo xtask layers
• cargo test --workspace on Windows: 2192 passed, 74 failed, all in four rustgis-ui-egui UI test targets (define_projection, measurement_units, menu_checkboxes, smoke); the same 74 fail on main on this machine, and the Linux CI run here is green

Related issue

Follow-up to #839 (review comment on crates/render/src/svg.rs, "Bound the total SVG output size")

Summary by CodeRabbit

• Rendering Improvements
  • SVG documents containing large or multiple pictures now render at a resolution adjusted to the visible content area, helping prevent rendering limits from being exceeded.
  • Picture resolution continues to follow the requested DPI when the content fits within the supported rendering budget.

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: 4f815fd6-fd19-49ad-9526-1f145038b690

Commits

Reviewing files that changed from the base of the PR and between c7bf8e1 and 63a93f8.

Files selected for processing: 2

• crates/render/src/layout.rs
• crates/render/src/svg.rs

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

Walkthrough

SVG export now estimates the visible area of non-vector elements and adjusts the shared DPI used for pictures when the estimated pixel area exceeds a budget of twice MAX_PIXELS. Existing platform-specific MAX_PIXELS values remain unchanged.

Changes: SVG Picture DPI Budgeting

Layer / File(s)	Summary
Calculate and apply picture DPI crates/render/src/layout.rs, crates/render/src/svg.rs	MAX_PIXELS is now crate-visible. layout_svg calculates and passes a shared picture DPI based on visible non-vector area. Tests cover retained DPI for small layouts, reduced DPI for multiple full-page frames, and off-page frames.

Priority: Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Possibly related issues

• opengeos/RustGIS#682 — The issue describes page-sized map layouts, and this change adjusts picture resolution for SVG exports of those layouts.

Possibly related PRs

• opengeos/RustGIS#839 — This PR introduced the layout_svg and render_element path that this change updates.

Suggested reviewers: giswqs

Merge Risk: Minimal · up to 63a93

No new merge-blocking issue was established for this change. The previously published minimum-DPI limitation remains for the owner to consider.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title clearly and concisely describes the main change: bounding the pixels embedded in SVG layout exports.
Description check		The description includes a clear summary, detailed testing results, and a related issue reference. It provides sufficient context for the change and its intended behavior.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

_{A rabbit checks the page at dawn
Picture pixels gather, then scale down
Small maps keep their chosen light
Large frames fit within the bound
Off-page scenes leave DPI alone
The SVG hops onward, crisp and sound
One shared measure guides the way}

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

[github-actions]

Low-confidence: page_geometry clamps ppp to [0.25, 8.0] (about 24 dpi), so the budget isn't a hard bound. With enough stacked frames, fit asks for a lower dpi than that and the clamp wins. The embedded pixels then exceed 2 * MAX_PIXELS again, and the SVG still grows with the frame count. A cap on the picture count, or a doc note that the bound holds only down to 24 dpi, would make "without bound" accurate. The tests only go to 8 frames, so a test with a very large n would show it.

[github-actions]

Code review

The change is small and correct. picture_dpi shares one pixel budget (2 × MAX_PIXELS) across all picture elements in the SVG export, so many large map frames can't grow the file without bound. I did not run the code or the tests. I read the diff and page_geometry (crates/render/src/layout.rs:61), and the unit conversion checks out by hand. k is pixels per mm, so k * fit / PT_PER_MM * 96 gives the dpi that fits, and the new tests use the same relation.

Bugs

• The budget isn't a hard bound at extreme frame counts (crates/render/src/svg.rs:152, confidence: low). page_geometry clamps ppp to [0.25, 8.0], about 24 dpi. With enough stacked frames, the computed dpi falls below that clamp, the clamp wins, and the embedded pixels exceed 2 × MAX_PIXELS again. The new doc comment says "without bound", which is slightly too strong. The tests stop at 8 frames. I left an inline comment.

Security

• Nothing found. The change only lowers the resolution of embedded images.

Performance

• Nothing beyond the point above. An empty area gives inf, so fit < 1.0 is false and the requested dpi is kept. A NaN dpi is already handled by page_geometry.

Quality

• MAX_PIXELS is now pub(crate), which is a reasonable way to share it. The new pub(crate) const line in layout.rs is one long line, so check it passes cargo fmt.
• The tests cover the main cases: surrounds on a frame keep their dpi, stacked frames stay within the budget and use it, and off-page frames take nothing from it. They use unwrap(), which is fine in test code.

AGENTS.md

• No violations seen: no panics or unwrap() outside tests, no wasm-breaking APIs, no new user-visible strings and no new dependencies.

Change size

Compared: the whole PR against main (merge base c7bf8e1 to PR head 315377f).

Metric	Value
Files changed	2 (added 0, modified 2, removed 0, renamed 0)
Size before	123.3 KB
Size after	126.2 KB
Total size change	+2.8 KB

File	Before	After	Change
crates/render/src/svg.rs	15.3 KB	18.1 KB	+2.8 KB
crates/render/src/layout.rs	108.1 KB	108.1 KB	+11 B

[github-actions]

📖 Docs preview

Item	Value
Preview	https://preview.rustgis.app/pr-912/
Web app	https://preview.rustgis.app/pr-912/app/
Commit	63a93f8

GitHub Pages takes a minute or two to publish a new commit.

[github-actions]

Code review

I found no bugs, security issues or guideline violations in this PR. I checked picture_dpi against page_geometry and the surrounding code by reading it. I did not build the PR or run its tests.

What I checked

• Unit conversion (svg.rs): page_geometry(..).2 is k, in pixels per millimetre, and k = PT_PER_MM * dpi / 96. So k * fit / PT_PER_MM * 96 correctly inverts it, and the area × k² pixel estimate matches how render_element sizes its pixmaps. The page_geometry cap on k (page MAX_PIXELS) is already included, so the new dpi can only go lower, never higher.
• Edge cases:
  • With no picture area, fit is inf, so the function returns dpi unchanged.
  • Elements off the page or with a negative size contribute 0 through on_page.
  • A NaN dpi falls back to 96 in page_geometry.
  • The 0.25 clamp on ppp applies only at extreme overload, about 24 dpi.
• Behaviour: ppp, which sets line widths, still uses the requested dpi. That is correct, because only the embedded pictures change resolution.
• Never-crash rules: there is no unwrap, expect or indexing in non-test code. The test-only unwrap is allowed.
• Wasm and layering: the change adds no dependencies, so wasm and layering are unaffected. MAX_PIXELS becomes pub(crate), which is a minimal exposure.
• Tests: they cover the unchanged case (surrounds on a frame, two frames at the limit), the shared budget (8 stacked frames use the budget within 1%) and off-page frames. That is good coverage.

Quality (minor, low confidence)

• svg.rs: the local let picture_dpi = picture_dpi(layout, dpi); shadows the function name. This is legal and works, but a different binding name such as pic_dpi would read more clearly.
• layout.rs: the pub(crate) const MAX_PIXELS line is now longer than before. Run cargo fmt or cargo xtask ci to confirm rustfmt accepts it, given the repo's wide line width.
• svg.rs: the 2× budget is a magic number that appears in both the doc comment and the code. A named constant would keep them in sync.

Change size

Compared: the whole PR against main (merge base c7bf8e1 to PR head 63a93f8)

Metric	Value
Files changed	2 (0 added, 2 modified, 0 removed, 0 renamed)
Size before	123.3 KB
Size after	126.2 KB
Total size change	+2.8 KB

File	Before	After	Change
crates/render/src/svg.rs	15.3 KB	18.1 KB	+2.8 KB
crates/render/src/layout.rs	108.1 KB	108.1 KB	+11 B

[github-actions]

Code review

I found no bugs, security issues or performance problems in this PR. I read the full diff and checked it against page_geometry and render_element in crates/render/src/layout.rs. I did not build or run the code or the tests.

Bugs

• No defects found (high confidence).
  • dpi conversion: page_geometry returns k in pixels per mm, and k = PT_PER_MM * dpi / 96. So k * fit / PT_PER_MM * 96.0 converts the scaled k back to a dpi correctly.
  • Pixel budget: pixels scale with k², so sqrt(2 * MAX_PIXELS / (area * k²)) is the right shrink factor.
  • Edge cases: an area of 0 gives fit = inf, so the original dpi is kept. Off-page elements are clamped by on_page.
  • Coordinates: render_element returns coordinates in mm, so changing the picture dpi doesn't shift the placement in the SVG.
• Minor, low confidence: page_geometry clamps the effective dpi to at least 24 (ppp ≥ 0.25). A layout with an extreme number of full-page frames can still exceed the 2× budget. A pathological case like that is bounded only by that floor, not by the budget. This is an unlikely edge and not worth blocking on.

Security

• Nothing found. No new input parsing, I/O or unsafe.

Performance

• The change does what it's meant to. It bounds the total embedded pixel count, and the area sum is O(elements).
• The 2× factor is a reasonable heuristic, since surrounds usually overlap a frame. The PR describes it as a deliberate trade-off.

Quality

• crates/render/src/layout.rs:9: the MAX_PIXELS line is now longer after adding pub(crate). Check that cargo fmt accepts it. The PR says fmt passes, so this is probably fine.
• The tests cover both the no-change case and the budget-sharing case, including off-page elements. The unwrap() in the test helper is in test code, so it is allowed.

AGENTS.md

• No violations. There are no unwrap, panic or unsafe calls in non-test code. The change stays inside rustgis-render and touches no wasm-unfriendly APIs, and MAX_PIXELS already accounts for wasm. No UI strings were added.

Change size

Compared: the whole PR against main (merge base c7bf8e1 to PR head 63a93f8).

Metric	Value
Files changed	2 (added 0, modified 2, removed 0, renamed 0)
Size before	123.3 KB
Size after	126.2 KB
Total size change	+2.8 KB

File	Before	After	Change
crates/render/src/svg.rs	15.3 KB	18.1 KB	+2.8 KB
crates/render/src/layout.rs	108.1 KB	108.1 KB	+11 B