Repository navigation
feat(raster): Shaded Relief casts soft shadows (#1782, part 1) - #1796
Conversation
Shaded Relief gets "shadow" (0 to 1, Cast shadows in the Style pane): how dark the slopes the terrain hides from the sun are drawn. 0, the default, draws today's pixels and isn't saved. rustgis_map::shadow::light traces each cell toward the sun, reading heights between cells (bilinear, so diagonal suns don't over-shadow), and keeps the share of a sun disc 5 degrees either side of its altitude left above the steepest angle seen; the trace stops once nothing farther can change it. RasterLayer::cast_shadows works the grid out once per pixels, band, sun, Z factor and mask on a thread of its own and keeps it (up to 3 per layer and its copies): the map draws without it meanwhile and again when it lands; exports, the Layout page and the PMTiles export wait for it (the wait stops when the run is cancelled). The drawn hillshade is multiplied by 1 - shadow * (1 - light), so the ambient light stays. The relief is drawn without cast shadows, with a notice, over 500,000 cells in the browser, for a COG read in place with only its overview held, and when the grid can't be held. Tests: a plane, a wall and the soft edge against their known geometry at 10 azimuths; stop, flat, no data, mask; threads give the same grid; the grid kept per sun and for copies; read in place said; drawing at 0, 0.5 and 1; raster.symbology keeps, clamps and saves it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. Walkthrough
Merge Risk:
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Title check |  |
This assessment has not completed. The check will be retried on the next pre-merge check run. | |
| Description check | |
This assessment has not completed. The check will be retried on the next pre-merge check run. |
- 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.
A rabbit watched the sunlit hill,
While shaded pixels learned to chill.
A grid of light crossed root and stone,
Then found its way to maps full-grown.
“Draw again,” the rabbit said,
As shadows settled where they led.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Its cast shadows: an export waits for them; the map draws without them meanwhile, | ||
| // and again once they land. | ||
| let landed = || rustgis_tiles::cache::notify_at(web_time::Instant::now()); | ||
| let light = match rl.cast_shadows(full_res_rasters, landed) { |
There was a problem hiding this comment.
Bug (AGENTS: Never hold the window), medium-high confidence. The Layout view renders its page on the UI thread (ui-egui/src/layoutview.rs:658, render_pending → complete → render_cpu(map, view, true, …)), so full_res_rasters is true here. With full_res_rasters true, cast_shadows(true, …) blocks in work.result(true) until the whole grid is worked out. That is seconds on one thread for a few million cells at a low sun (the PR's own numbers: 5.8 s extra at 30° on one thread, about 10 s at 20°). The wait only stops on progress::check(), and the UI thread has no probe installed, so Cancel can't reach it. Before this change the layout wait was for network tiles. It now also waits on CPU work, with no way out.
Suggested direction: the layout page should draw without the grid and set page_wait/tile_out.pending the way the interactive map does, or the grid should be started from a background path before the page draws.
There was a problem hiding this comment.
Fixed in 0306284.
- The map's frames never wait for the grid any more.
- An export waits for the visible layers' grids in
completebefore it draws. That covers map exports,layout::renderandlayout::render_drawing, and the PMTiles export. layout::render_pending, which the Layout view calls on the interface's thread, draws its frames without the grids still to come (without_shadow_wait).layout::shadows_to_come()reports them, andlayoutview.rssetspage_waitfor them natively too, so the page is drawn again as they land (notify_atmoves the generationlandings()reads).- Test:
an_export_waits_for_cast_shadows.
| None => { | ||
| // A few kept, for copies of the layer lit otherwise (another map, an undo): | ||
| // past them, the oldest goes and its work stops. | ||
| if jobs.len() >= KEPT_LIGHTS { |
There was a problem hiding this comment.
Bug (doc/behaviour mismatch, medium confidence). The doc comment and PR body say "a change of sun stops the old work", but nothing stops a job when the sun changes. The old job is kept, and its LightJob::drop only runs on eviction here, after three newer keys exist. So a slider drag through many suns leaves up to three full grid computations running per layer (each on threads::max() threads). The newest sun then competes with stale work.
There is a second effect. Copies share this list (CastLight is an Arc), so another map's request for a fourth sun evicts the job an export is waiting on. The export's result(true) then returns Unavailable("…were stopped"), and the export refuses with a misleading reason.
Either stop the superseded job when a new key for the same pixels arrives, or correct the doc and PR body.
There was a problem hiding this comment.
Fixed in 0306284.
cast_shadowsnow takes the drawing layer's id as its owner. A new key from the same owner drops (and so stops) that owner's unfinished job, unless an export is waiting for it.- Finished grids are kept, so going back to a sun is free.
- Copies drawn by other owners are left alone, so two maps lit differently don't stop each other.
- Eviction past the 3 kept skips grids an export waits for: another map can no longer make an export fail with "were stopped".
- The doc and the PR body now say this.
- Test:
a_new_sun_supersedes_the_owners_last_work.
| if let Some((grid, strength)) = self.shadow | ||
| && let Some(lit) = grid.get(ri as usize * r.width as usize + ci as usize) | ||
| { | ||
| let left = 1.0 - strength * (1.0 - f64::from(*lit) / 255.0); |
There was a problem hiding this comment.
Bug (spec mismatch), medium confidence. The PR body says drawing keeps "the ambient light (ambient + direct × shadow)". The code does not do that. hillshade (crates/map/src/raster.rs:1776) has no ambient term, and this line multiplies the whole shade by 1 − strength × (1 − lit). At shadow 1 and light 0 the shade goes to 0, which the test cast_shadows_darken_the_direct_light_by_their_strength confirms (want(0)). So a fully shadowed slope is pure black, not ambient-lit.
Either add an ambient floor to the formula, or correct the PR body and the user guide (docs/user-guide/data.md) so they say shadows darken the whole relief.
There was a problem hiding this comment.
No code change for the formula: the ambient term is there, in the Colorizer rather than in hillshade.
RasterRenderer::ShadedReliefcolours withbase × (0.25 + 0.75 × h)(crates/map/src/raster.rs,paint). This PR scales onlyh, so the colour isbase × (0.25 + 0.75 × h × (1 − shadow × (1 − light))): ambient + direct × shadow.- In the test,
want(0)isColorizer::color(.., Some(0)), which is a quarter of the colour, not black. - 0306284 makes that explicit: the test now asserts that a fully shadowed grey pixel is
[64, 64, 64, 255], and the user guide says full shadow keeps the ambient light.
| }); | ||
| if spawned.is_err() | ||
| && let Some(f) = run.lock().ok().and_then(|mut f| f.take()) | ||
| { |
There was a problem hiding this comment.
AGENTS (Never hold the window), medium-low confidence. If the OS refuses the thread, f() runs on the caller. The caller is render_frame, which runs on the UI thread for the interactive map and for the Layout view. The whole trace then runs on the UI thread, and it also runs while self.cast.0 is locked, so any other copy of the layer blocks behind it.
AGENTS says a refused thread "costs speed, not the run". A refused thread should not cost the window. Consider leaving the job unstarted (Pending) and retrying on a later frame, or spawning from a background path. Holding the jobs lock during the trace also needs a look.
There was a problem hiding this comment.
Left as it is, with one part fixed in 0306284.
- A thread the system refuses is the AGENTS.md case of "a thread the OS refuses costs speed, not the run". It needs the OS to refuse a thread, which hardly happens.
- Retrying on later frames would leave the shadows out for as long as threads are refused, and an export would wait for nothing.
- The other part is fixed: the work now starts after the jobs lock is let go, so other copies of the layer don't block behind an inline trace.
| #[serde(default, skip_serializing_if = "is_no_shadow")] | ||
| shadow: f64, | ||
| }, | ||
| } |
There was a problem hiding this comment.
AGENTS (saved files), medium confidence. shadow is a new serialized field, and it is saved whenever it is above 0. crates/engine/src/project.rs:56-58 says FORMAT must advance "when an older reader would lose meaningful saved information on resaving". An older RustGIS parses this variant without shadow (serde ignores unknown fields here), so it silently resets the setting to 0 and saves the file again without it. docs/maintaining.md (Saved files) asks for a format bump in that case, and a coordinate-first decision.
Please either advance FORMAT (and state the release-ordering plan), or show that an older reader keeps the field unchanged.
There was a problem hiding this comment.
Not changed here: this is a maintainer decision, and I've asked it in the PR body (Saved files).
- An older RustGIS reads
shadowas absent, draws without shadows, and drops it on saving again: what you describe. - Advancing
FORMATwould make every project saved by the next release unreadable to 0.6.0, with or without shadows.docs/maintaining.mdprefers shipping the reader a release before anything writes it. - feat(layout): a legend shows a layer under a label instead of its name (#1654) #1739 added a saved field (legend labels) the same way without advancing it.
- I'll follow whichever giswqs prefers.
| } | ||
| } | ||
| let mut results: Vec<Result<(), Unlit>> = refused.into_iter().filter_map(take).map(run).collect(); | ||
| results.extend(handles.into_iter().map(|h| h.join().unwrap_or(Err(Unlit::Stopped)))); |
There was a problem hiding this comment.
Quality / Never silent, low confidence. h.join().unwrap_or(Err(Unlit::Stopped)) turns a panicked worker into Unlit::Stopped. The caller then reports "its cast shadows were stopped", though nothing was stopped and a panic happened. Map the join error to a distinct reason (for example, a panic notice like the one in start_light). The worker's catch_unwind is not reached from here, since the panic is in a scoped thread.
There was a problem hiding this comment.
Fixed in 0306284.
- A panicked scoped worker is now
Unlit::Failed, and the notice says "working them out failed", as thecatch_unwindaround the whole trace does. Stoppedis only for work superseded on purpose.
Code reviewReviewed the full diff (12 files, +778/−52) against Bugs
Security
Performance
Quality
AGENTS.md
Change size: Change sizeCompared with the base branch: the whole PR against
|
📖 Docs preview
GitHub Pages takes a minute or two to publish a new commit. |
…upersedes the old (opengeos#1782) Review round 1 of opengeos#1796: - The map's frames never wait for cast shadows: an export waits for the visible layers' grids before it draws (`complete`), while the Layout view's page (`render_pending`, on the interface's thread) is drawn without them, says so (`layout::shadows_to_come`) and is drawn again as they land, natively too. The layout export (`layout::render`) still waits. - `cast_shadows` takes the drawing layer's id as its owner: a new sun from the same owner stops the work for its last one unless it is done or an export waits for it, and eviction past the 3 kept skips grids an export waits for, so another map can't make an export fail with "were stopped". - The work is started after the jobs lock is let go. - A trace thread that panics is said as a failure, not as stopped. - Docs: only the direct light is darkened (full shadow keeps the ambient light, now tested: a quarter of the colour, not black); the Layout view draws without them until they land; a 0-360 grid casts nothing across its seam. Tests: a new sun supersedes the owner's last work while another owner's goes on; an export waits for cast shadows and draws them; the PMTiles export refuses a layer read in place with cast shadows and says why. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The review summary's items without a thread, after 0306284:
🤖 Generated with Claude Code |
| if shadows { | ||
| PAGE_SHADOWS.with(|c| c.set(true)); | ||
| } | ||
| *pending |= img.tiles_pending || img.data_pending || shadows; |
There was a problem hiding this comment.
Bug (Never silent), medium-high confidence. frame_canvas only reads img.tiles_pending and img.data_pending from the frame's Rendered. Its tile_errors are dropped, and nothing else in layout.rs reads them. render_pending returns (Pixmap, bool) and render returns a Pixmap, so the new CastShadows::Unavailable reasons never reach the user on a Layout page.
Failure case: a Shaded Relief layer with shadow > 0 that is read in place with only its overview held, or that is over 500,000 cells in the browser, or whose grid can't be allocated. On the Layout view, or in a layout export, the page is drawn with no shadows and no notice. The PR's "a notice names the layer and the reason" holds for the map view (mapview.rs shows tile_errors) but not for pages.
Fix direction: collect each frame's img.tile_errors into the page result and show them as a notice in the Layout view, and refuse the layout export the way the map export does. Add a test that a layout page with an unavailable shadow reports it.
There was a problem hiding this comment.
Fixed in 4e4a3d0.
- Each frame's reasons are kept for the page (
take_shadow_notices, thenlayout::shadow_notices()). - The Layout view says them once each through the same set the map view uses.
- A layout export (PNG, PDF, series) says them as notices.
- The export doesn't refuse: the map export doesn't refuse either. It writes, and says what's missing (its
gaps). - Test:
a_page_says_why_a_frame_drew_no_cast_shadows. It checks that a frame read in place gives one notice naming the layer, and that a page with shadows, or with none asked for, gives none. - Only the cast-shadow reasons are carried to the page; other
tile_errorsstay as they were onmain.
| } | ||
| // Nothing farther rises above what is kept: the angle seen, or the disc's lower | ||
| // edge (below it the cell has the whole sun); or the whole disc is hidden. | ||
| if (self.top - z0) / d <= steepest.max(self.sun.low) || steepest >= self.sun.high { |
There was a problem hiding this comment.
Performance, medium confidence (from reading, not measured). The trace only stops when the terrain left can't rise above the angle kept, or the disc is hidden. The bound is (self.top - z0) / d <= max(steepest, low), where top is the highest cell in the whole raster. For a sun at 5° or lower, low is 0 or negative, so over gentle or flat ground the bound is never met and every cell traces to the raster's edge. Each cell costs up to max(w, h) bilinear samples, so the total is O(cells × raster size), roughly 10^11 to 10^12 steps for a 10,000 × 10,000 DEM. A raster like that is a plausible input.
Two related points:
- Nothing in the loop checks the run's Cancel. Only the superseded
stopflag is read, and a cancelled export waits inresultand then gives up, while this grid runs to completion on its own thread. The grid is kept for the layer, so it isn't lost, but the AGENTS rule about looking at Cancel in long loops applies. - The PR's limits section and user guide don't mention the cost at low suns.
Options: a maximum trace distance (changes output for distant terrain, so it needs a decision and a line in the limits), or per-block maxima in place of the global top so the bound becomes local. Either way, a test for a low sun on a larger grid would pin the cost.
There was a problem hiding this comment.
Measured on the Ouro Preto DEM (1966 × 1827, 3.59 M cells), whole-DEM export, one run each:
| Sun altitude | --threads 16, 0 → 0.8 |
--threads 1, 0 → 0.8 |
|---|---|---|
| 5° | 1.35 → 4.95 s | 1.53 → 20.99 s |
| 3° | 1.45 → 5.56 s | 1.90 → 22.38 s |
| 2° | 1.72 → 3.94 s | 1.55 → 21.95 s |
At 30° it was about +0.5 s on 16 threads and +5.8 s on one, so a low sun costs several seconds more.
What changed in 4e4a3d0:
- The trace looks at the run's Cancel between rows on a thread that has one. That is the browser, which works the grid out where it is asked.
- Work stopped part-way is dropped and started again on the next draw, not kept as a failure.
- On the desktop the trace runs on its own thread, and the map draws without it meanwhile. Its stop is the superseding, which the review rounds made exact. An export's wait gives up on Cancel.
- The cost is now in the user guide and in the PR's limits.
What I left out:
- A maximum distance would change the output exactly where low suns cast their long shadows.
- Per-block maxima don't help with a disc whose lower edge is at or below the horizon: any higher ground ahead still counts.
- If you'd rather cap it, a cap at the distance where Earth's curvature hides the raster's relief would be the honest one. I'd do that in part 2 with a test.
Code reviewTwo new findings were posted inline. Three smaller findings and the prior threads are summarised below. Bugs
Performance
Quality (nits, grouped)
AGENTS.md
Prior review threads
Left alone: 3 concerns already raised (ambient, refused-thread, FORMAT). 3 more are resolved in the current code. Change sizeCompared with
|
… count (opengeos#1782) `landed` is called on the worker just after the grid is kept, so a count read as soon as the export's wait returned could still be 0 (CI on Linux). The test now receives each landing on a channel, waiting for the first, and checks that none comes after the kept grid is used again. Under load (16 busy loops, --test-threads=32) the old test failed 3 times in 40, this one none. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| PAGE_SHADOWS.with(|c| c.set(true)); | ||
| } | ||
| *pending |= img.tiles_pending || img.data_pending || shadows; | ||
| Some(finish_frame(img, ppp)) |
There was a problem hiding this comment.
Re-raising the earlier note on this function, since it changed in this PR and the problem still holds. img.tile_errors is never read in frame_canvas, and nothing else in layout.rs reads it (grep confirms). The new CastShadows::Unavailable reasons go into tile_out.errors in render_frame, the same channel, so on a Layout page they never reach the user. The PR body's "a notice names the layer and the reason" holds for the map view only. Suggested direction: collect each frame's tile_errors into the page result, show them in the Layout view, and refuse the layout export as the map export does.
Confidence: high.
There was a problem hiding this comment.
The same as the thread above: fixed in 4e4a3d0.
| jobs.retain(|j| j.pixels.strong_count() > 0); | ||
| match jobs.iter().position(|j| j.key == key && Weak::ptr_eq(&j.pixels, &Arc::downgrade(r))) { | ||
| Some(i) => { | ||
| // The latest used last, so the oldest goes first. | ||
| let mut job = jobs.remove(i); | ||
| job.owner = owner; | ||
| let work = job.work.clone(); | ||
| jobs.push(job); | ||
| work | ||
| } | ||
| None => { | ||
| // The same owner's work for another sun, not done and not waited for, is | ||
| // superseded: it stops (as it is dropped). Grids done are kept, for going back. | ||
| jobs.retain(|j| j.owner != owner || j.work.is_done() || j.work.waited()); |
There was a problem hiding this comment.
The earlier thread on superseding was fixed for a new sun, but not for returning to a sun already held. Here the Some(i) branch keeps the owner's other unfinished job. Example: owner 1 is on sun A (done), moves to sun B (unfinished), then back to A. A is found, so B is never stopped, and it runs to completion for nothing. The doc comment says a new sun from the same owner stops its last work. Suggested change (untested, I could not build it here):
| jobs.retain(|j| j.pixels.strong_count() > 0); | |
| match jobs.iter().position(|j| j.key == key && Weak::ptr_eq(&j.pixels, &Arc::downgrade(r))) { | |
| Some(i) => { | |
| // The latest used last, so the oldest goes first. | |
| let mut job = jobs.remove(i); | |
| job.owner = owner; | |
| let work = job.work.clone(); | |
| jobs.push(job); | |
| work | |
| } | |
| None => { | |
| // The same owner's work for another sun, not done and not waited for, is | |
| // superseded: it stops (as it is dropped). Grids done are kept, for going back. | |
| jobs.retain(|j| j.owner != owner || j.work.is_done() || j.work.waited()); | |
| jobs.retain(|j| j.pixels.strong_count() > 0); | |
| let same = |j: &LightJob| j.key == key && Weak::ptr_eq(&j.pixels, &Arc::downgrade(r)); | |
| // The owner's work for another sun, not done and not waited for, is superseded | |
| // whichever sun it now draws: it stops (as it is dropped). Grids done are kept. | |
| jobs.retain(|j| j.owner != owner || same(j) || j.work.is_done() || j.work.waited()); | |
| match jobs.iter().position(same) { | |
| Some(i) => { | |
| // The latest used last, so the oldest goes first. | |
| let mut job = jobs.remove(i); | |
| job.owner = owner; | |
| let work = job.work.clone(); | |
| jobs.push(job); | |
| work | |
| } | |
| None => { |
The None branch's own retain line then goes, since the one above covers it. Add a test for the back-and-forth case next to a_new_sun_supersedes_the_owners_last_work.
Confidence: medium.
There was a problem hiding this comment.
Fixed in 4e4a3d0, as suggested: the owner's retain now runs before the lookup, so going back to a sun already kept stops the unfinished one just left. The test is extended for the back-and-forth case.
| /// What it has: with `wait`, once it is done, unless the run waiting is cancelled (see | ||
| /// [`rustgis_data::progress`]). | ||
| fn result(&self, wait: bool) -> CastShadows { | ||
| let _waiting = wait.then(|| Waiting::on(&self.waiting)); |
There was a problem hiding this comment.
Possible race: cast_shadows releases the jobs lock before result(wait) registers the waiter (Waiting::on, here). In the gap, a new sun from the same owner (the map view on the UI thread, while an export runs on a background thread with the same layer.id) can run retain and stop the export's job. The export then gets Unavailable("…were stopped"), which is a misleading refusal. A fix is to register the waiter inside the lock when wait is set, and pass the guard into result.
Confidence: low. I have not reproduced it, and the window is small.
There was a problem hiding this comment.
Left as it is. The window is real but narrow, and its result isn't silent.
- For it to bite, the map view must ask for a new sun for the same layer in the microseconds between the lock being let go and the export's waiter being counted.
- The export then refuses with "its cast shadows were stopped". That is a refusal that says why, not a wrong image, and exporting again gives the right one.
- Counting the waiter under the lock means passing a guard tied to the lock's lifetime into
result. I'd rather not add that for this case. If a maintainer wants it closed, it's a small follow-up.
| view draw without them for the moment that takes (under a second for a few million cells on a | ||
| multi-core computer, several seconds on one thread), then with them, and an export waits for them. |
There was a problem hiding this comment.
The timing claim is only measured at a 30° sun. The PR's table gives about 0.5 s extra for 3.6 million cells on 16 threads, and 5.8 s on one thread. At a 20° sun the trace goes farther, and the PR reports 2.5 s with shadows on 16 threads for its 1:75,000 view, and 10.7 s on one thread. Suggested wording (untested):
| view draw without them for the moment that takes (under a second for a few million cells on a | |
| multi-core computer, several seconds on one thread), then with them, and an export waits for them. | |
| view draw without them for the moment that takes (about half a second extra for 3.6 million cells at a | |
| 30° sun on 16 threads; several seconds on one thread, and longer at a lower sun), then with them, | |
| and an export waits for them. |
Confidence: medium.
There was a problem hiding this comment.
Fixed in 4e4a3d0 with the measured numbers, a little differently from the suggestion. The guide now says:
- under a second for a few million cells on a multi-core computer, and several seconds on one thread;
- a sun under about 5° traces much farther: some seconds on a multi-core computer, and twenty or so on one thread.
Code reviewBugs
Security
Performance
Quality
AGENTS.md
Earlier review threads (8 on this PR)
Change sizeCompared: the whole PR against main (merge base 780e071 to PR head ca6eda2).
I did not build or run the tests. Everything above comes from reading the diff and the surrounding code. |
…nd low suns (opengeos#1782) Review rounds 2 and 3 of opengeos#1796, and the merge of main: - Two `draw_with` calls that came with main take `light` (None). - Why a layer was drawn without its cast shadows reaches a Layout page: frames keep the reasons (`layout::shadow_notices`), the Layout view says them once each as the map view does, and a layout export says them as notices. - The trace looks at the run's Cancel between rows where the thread has one (the browser). Work stopped part-way is dropped and started again, not kept as a failure. - Going back to a sun already kept stops the unfinished work for the one just left, as a new sun does. - `shadows_to_come` in the render crate is `frame_shadows_to_come`, apart from `layout::shadows_to_come`. - The user guide gives the cost of a sun under about 5 degrees, measured on the Ouro Preto DEM (3.59 M cells): 2.5-4 s more on 16 threads, about 20 s on one. Tests: a page says why a frame drew no cast shadows (read in place), and nothing when it has them or none was asked for; going back to a sun kept stops the one left. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The summary items of review rounds 2 and 3 without a thread, after 4e4a3d0:
🤖 Generated with Claude Code |
| let chunks: Vec<(usize, &mut [u8])> = out.chunks_mut(rows * w).enumerate().collect(); | ||
| let run = |(i, chunk): (usize, &mut [u8])| -> Result<(), Unlit> { | ||
| for (dy, row) in chunk.chunks_mut(w).enumerate() { | ||
| if stop.load(Ordering::Relaxed) || rustgis_data::progress::check().is_err() { |
There was a problem hiding this comment.
Bug (AGENTS: Never hold the window), high confidence. The Cancel check here can never fire on the threads that run this loop. progress::check() reads a thread-local probe (crates/data/src/progress.rs:25). start_light spawns a fresh thread with no probe installed, and the spawn_scoped row threads don't inherit one either. So natively this always returns Ok, and a cancelled export leaves a background thread tracing the whole grid to completion. Only the export thread gives up, in CastShadows::result. The earlier thread on this line said nothing checks Cancel. The check is now present, but it is inert.
Suggested direction: pass the caller's probe into start_light and install it on the worker and on each scoped row thread (rustgis_data::progress::install(Some(probe))), or give light a cancel closure from the caller.
There was a problem hiding this comment.
Fixed in 6a30910. You're right that the per-row progress::check() can't fire on the desktop: the worker and its row threads have no probe.
- An export cancelled while it alone waits for a grid now sets that work's
stopinLightWork::result. The row threads already look atstopbetween rows. - The map, drawing again, starts the work over: stopped work is dropped (round 2) and now calls
landed. - The
progress::check()stays for the browser, which traces on the run's own thread. - With others also waiting, the work goes on for them.
- Test:
a_cancelled_export_stops_the_work_it_waited_for. A 2000 × 2000 grid under a 2° sun, waited for under a cancelled probe, ends stopped with no grid, and the next draw starts it again.
| let light = match layer.cast_shadows(true, u32::MAX, || {}) { | ||
| rustgis_map::raster::CastShadows::Ready(grid) => Some(grid), | ||
| rustgis_map::raster::CastShadows::Unavailable(why) => return Err(format!("{}: {why}, so the archive isn't written", r.name)), | ||
| rustgis_map::raster::CastShadows::Pending | rustgis_map::raster::CastShadows::None => None, |
There was a problem hiding this comment.
Bug (Never silent), medium confidence. Pending is returned by cast_shadows(true, …) when the wait gives up on Cancel (crates/map/src/raster.rs, CastShadows::result). Mapping it to None here means the archive is drawn and written without its shadows, with no notice, if the caller doesn't discard a cancelled result. raster_export::export has no progress::step of its own, so nothing here stops it. The map path has the same shape: wait_for_cast_shadows ignores the outcome, and complete then draws without shadows.
Suggested fix: treat Pending as an error here, for example Err(format!("{}: its cast shadows weren't worked out (the run was cancelled), so the archive isn't written", r.name)), and check progress::check() after the wait in complete.
There was a problem hiding this comment.
Fixed in 6a30910 for the PMTiles export: Pending from a wait that gave up is now an error ("its cast shadows weren't worked out (the export was cancelled), so the archive isn't written"). Test: a_cancelled_wait_for_cast_shadows_writes_no_archive.
complete is left as it is. It already returns early on Cancel for its tile wait ("An export cancelled meanwhile stops waiting: it writes nothing"). Its callers look at Cancel before writing: the map's PNG and GeoTIFF exports run progress::check() right after drawing (share.rs), and the layout export calls progress::step per page. So a cancelled map or layout export writes nothing.
A run that isn't cancelled always gets the grid, or Unavailable, which render_frame then says as a notice.
| })); | ||
| match traced { | ||
| // Stopped, it was replaced: nothing is drawn again for it. | ||
| Ok(Err(crate::shadow::Unlit::Stopped)) => work.finish(Err(STOPPED.into())), |
There was a problem hiding this comment.
Bug, medium-low confidence. The Stopped branch calls work.finish but not landed(). The Ok(out) branch does call it. A frame or page that was drawn Pending on a job that is then superseded, or evicted by another owner's grids, is not told that the state changed. The Layout page waits on shadows_to_come() and is redrawn only when notify_at fires, so it can keep drawing without shadows until the next interaction. This is most likely when a Layout page's sun is replaced by another owner's eviction.
Suggested fix: call landed() after finish in every branch, including Stopped. Stopped work has no grid, so a redraw just asks again.
There was a problem hiding this comment.
Fixed in 6a30910: the Stopped branch calls landed() after finish, so a frame or page drawn while that work was to come is drawn again and asks for what it draws now.
Code reviewThree new findings, all posted inline. Earlier concerns were left alone where still open: 9 threads raised again or still open (the Waiting race in Bugs
Security
Performance
Quality
AGENTS.md
Change sizeCompared: the whole PR against main (merge base 2c75bda to PR head 4e4a3d0).
|
|
Round 5's items without a thread, after 5a081a2:
🤖 Generated with Claude Code |
| None => { | ||
| // Past the few kept, the oldest nothing waits for goes. | ||
| if jobs.len() >= KEPT_LIGHTS | ||
| && let Some(i) = jobs.iter().position(|j| !j.work.waited()) |
There was a problem hiding this comment.
Bug, medium-high confidence (from reading, not run). With KEPT_LIGHTS (3) grids already kept, this evicts the oldest one nothing waits for, even when it is still being worked out. Dropping it sets its stop; the worker returns Stopped and calls landed() (the branch just above). That redraws the map or page, which asks for the evicted grid again and evicts another unfinished one. With four Shaded Relief layers in one map, or four relief frames on a page, each pass stops one of the four grids, so none finishes. The view keeps redrawing and the shadows never appear.
Suggested fix (untested): evict only finished grids that nothing waits for. Unfinished grids are bounded by the layers that want them, and retain already drops the dead and superseded ones.
| && let Some(i) = jobs.iter().position(|j| !j.work.waited()) | |
| && let Some(i) = jobs.iter().position(|j| j.work.is_done() && !j.work.waited()) |
There was a problem hiding this comment.
Reproduced, and fixed in dff3689.
- Reproduction: a page of 5 maps, each showing a copy of one relief as its first layer under its own sun, drawn again until no shadows were to come. It never settled in 60 s (
many_maps_lit_otherwise_on_a_page_all_get_their_shadows). - Fix: each owner's latest grid is never let go, since it is what that owner draws. The cap of 3 now applies only to grids of suns their owners have left (done, nothing waiting), kept for going back. The test now settles at once.
- Memory: live grids are bounded by the owners drawing the layer (one byte a cell each), plus those 3.
- Different layers: four separate relief layers in one map never shared a list, since each layer has its own grids, so they weren't affected. Only copies of one layer lit otherwise were.
| // says so as tiles to come; a page's, as shadows to come. | ||
| let light = match rl.cast_shadows(false, (map_instance, layer.instance_id()), shadows_landed) { | ||
| rustgis_map::raster::CastShadows::Ready(grid) => Some(grid), | ||
| rustgis_map::raster::CastShadows::Pending => { |
There was a problem hiding this comment.
Never silent, low-medium confidence. On a full-resolution draw (a map or PDF export, or the Layout page), Pending only sets SHADOWS_TO_COME. render_png and layout::render never read that flag, so the file is written with the layer drawn without its shadows and no notice.
complete waits for the grid before it draws, but the wait does not pin it. Once the wait returns, the waiter count drops to zero, so the cap can evict the finished grid (another job from the UI thread) before this lookup. That is the case the earlier answer ("a run that isn't cancelled always gets the grid") does not cover.
Suggested direction: when full_res_rasters gets Pending, record a notice or tile error as the Unavailable arm does, so the export refuses or names the gap. Or keep the grid pinned from the wait until the draw.
There was a problem hiding this comment.
Left as it is, since after dff3689 the window is down to a change of sun during the export.
- The grid an export waited for is its owner's latest, which is no longer let go. A finished grid of an owner's current sun stays until the pixels go.
- The only way left between the wait and the draw is the same map, in the interface, moving to a new sun in that instant, and three other left-behind grids then evicting this one. The export would then draw its snapshot of the old sun without shadows.
- If you'd like it closed fully, pinning the grids from the wait to the draw is a small follow-up. For this PR I'd rather not add it for a race that needs a sun change mid-export.
Code reviewBugs
Security
Performance
Quality
AGENTS.md
Checked and found correct
Prior reviews
Change sizeCompared: the whole PR against main (merge base 2c75bda to PR head 5a081a2).
|
…on a page get theirs (opengeos#1782) Review round 6 of opengeos#1796: past 3 grids kept, the oldest unfinished one was let go; a page with more maps showing copies of one relief under other suns (an atlas of several frames) then had each landing redraw a frame that started its grid again and let another go, so the shadows never came. Reproduced: 5 maps on a page never settled within 60 s. Each owner's latest grid is now kept, as it is what that owner draws; the cap of 3 applies to grids of suns their owners have left (kept, done, for going back). Test: the two-map page test is shared with a five-map one, which settles. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| n.push(e.clone()); | ||
| } | ||
| }); | ||
| if !tile_out.errors.contains(&e) { |
There was a problem hiding this comment.
Never silent, medium-high confidence. The PNG map export (share.exportMap, map_png, map_png_fitted) drops the shadows without any notice when this arm returns Unavailable. The path is png_of → render_png → render_exported (lib.rs ~6507), which refuses only pending_in_place and partial. Nothing in the engine reads tile_errors for a PNG. So a browser export of a raster over 500,000 cells, or one where the grid can't be allocated or the worker panics, is written as a plain relief with no message. The GeoTIFF export does say something, through gaps, but that's a different function.
This also answers the earlier thread on the layout notices. The author's reply said "the map export ... says what's missing (its gaps)". That holds for render_placed only, not for render_png.
Suggested fix (small): mark the layer as partial, so render_exported refuses with the reason already in tile_errors ("name: its cast shadows aren't drawn: …, so the image isn't written"). Needs a regression test that a browser-sized or failed grid refuses the PNG, and that a clean relief still writes.
| if !tile_out.errors.contains(&e) { | |
| if !tile_out.errors.contains(&e) { | |
| tile_out.errors.push(e); | |
| } | |
| // Refused by the exports that draw whole, as a part drawn from less than it needs is. | |
| if !tile_out.partial.contains(&layer.id) { | |
| tile_out.partial.push(layer.id); | |
| } |
There was a problem hiding this comment.
Fixed in ae2c369, without marking the layer partial, which would make the export refuse.
- PNG and PDF: these map exports now say the reason as a warning (": : its cast shadows aren't drawn: "), as the layout export does.
rustgis_render::take_shadow_notices()gives what the frames drawn on the thread said, andrender_exportedclears it first. - GeoTIFF:
render_placedgave everytile_errorsentry as "a tile layer couldn't be drawn, so its part is empty", so a layer drawn without its shadows read as a failed tile layer. Its gaps now give the cast-shadow reasons as themselves and count only the real tile-layer errors under that sentence. - Test:
a_map_export_says_a_layer_drawn_without_its_cast_shadows. A PNG and a GeoTIFF of a layer read in place with only its overview held each give one warning naming the layer and why, not as a tile layer. A clean export gives none. - Browser limit: the case asked for in the review can't be reached by native tests (
cfg(target_arch = "wasm32")). It goes through the sameUnavailablepath as the read-in-place one tested here.
Code reviewI read the full diff, the 19 existing review threads, and the current code at the points the diff touches. I posted one inline comment. A second finding is in unchanged lines outside the diff, so it appears below instead. Bugs
Security
Performance
Quality
AGENTS.md
Prior reviews
Change sizeCompared with
|
…as such (opengeos#1782) Review round 7 of opengeos#1796: - A PNG or PDF export of the map drew a layer whose cast shadows were unavailable (read in place, past the browser's cells, no memory) as a plain relief and said nothing: the reason only reached `tile_errors`, which those exports don't read. They now say it as a warning, as the layout export does; `rustgis_render::take_shadow_notices` gives what the frames drawn on this thread said. - A GeoTIFF export said it as "a tile layer couldn't be drawn, so its part is empty": its gaps now give it as itself ("<layer>: its cast shadows aren't drawn: <why>"), apart from tile layers that failed. Test: a PNG and a GeoTIFF of a layer read in place with only its overview held each say why its shadows aren't drawn, not as a tile layer; a clean export says nothing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| // going back. | ||
| jobs.retain(|j| same(j) || j.owner != owner || j.work.is_done() || j.work.waited()); | ||
| match jobs.iter().position(same) { | ||
| Some(i) => { |
There was a problem hiding this comment.
Bug, medium-low confidence. When a caller joins an existing unfinished job (this Some(i) branch), its landed callback is never stored. Only the caller that started the job gets start_light(…, landed) (line 805). The one that starts a job with a no-op is raster_export::export (landed = || {}, owner (u64::MAX, u64::MAX)), and same ignores the owner. So if a PMTiles export starts the grid first and the map view or a Layout frame joins it afterwards, the joiner's redraw request is dropped. The joiner then never redraws when the grid lands (see the separate comment on mapview.rs about redraw keys).
Suggested fix: keep a list of landing callbacks in LightWork (push on join, call all in finish), or call the one passed by the caller that started the job plus each joiner's.
There was a problem hiding this comment.
Fixed in 5541646.
LightWorkkeeps every landing call (at most 32), and the worker makes them all once it is done, stopped or failed.- A joiner's call is added under the calls' lock after checking the work isn't done. The worker drains them after
finish, so a call is either kept and made, or not needed: the joiner then gets the grid at once. - Test:
who_joins_work_under_way_is_told_when_it_lands. A starter and a joiner on another owner are both told.
| // view says them. | ||
| for e in rustgis_engine::render::layout::shadow_notices() { | ||
| if app.map.tile_errors.insert(e.clone()) { | ||
| app.session.notify(e, true); |
There was a problem hiding this comment.
Quality / consistency, low-medium confidence. These cast-shadow notices are raised with notify(e, true), which is Severity::Error. The map export (share.rs map_work, map_pdf_inner) raises the same reason as s.warn (Warning). The layout export (engine/src/cmd/layout.rs:4093) also uses notify(e, true). A shadow the export still wrote shouldn't be an error. Suggested: use the same severity everywhere, most likely warn/Warning.
There was a problem hiding this comment.
Fixed in 5541646: the Layout view and the layout export now say these as warnings (Session::warn), as the map exports do. What was drawn is written, so it isn't an error.
| let written = (|| { | ||
| let png = png_of(&mut map, view_size, width, height, dpi)?; | ||
| // Layers drawn without their cast shadows (see `rustgis_render::take_shadow_notices`). | ||
| shadows = rustgis_render::take_shadow_notices(); |
There was a problem hiding this comment.
Never silent, medium-high confidence; also a UI-thread hold. The shadow notices are taken here for share.exportMap and in map_pdf_inner, but map_png (share.rs:204-220) calls render_png and never takes them. map_png_fitted backs the app's Export Map download and Capture To Clipboard (ui-egui/src/lib.rs:3754, :4087), and sharing.rs:509 calls render_png directly. A layer drawn without its shadows there is silent. Suggested fix: in map_png, drain take_shadow_notices() and s.warn(...) each one.
Also, ui.captureMap (lib.rs:3751) and ui.exportMap run map_png_fitted synchronously on the UI thread. Through complete → wait_for_cast_shadows, they now block on the grid with no Cancel. The earlier fix (0306284) covered only the Layout view. These need a non-waiting draw or a background path (exports::runs_in_background).
There was a problem hiding this comment.
Fixed in 5541646.
- Notices:
map_pngnow says why layers were drawn without their cast shadows, and, when it was drawn without waiting, that they were still being worked out. That coversmap_png_fitted, the MCP snapshot, the time series and thumbnails. The web map page (sharing.rs) says the reasons too. - UI-thread hold:
ui.captureMapand theui.exportMapdownload draw inside the new publicrustgis_render::without_shadow_wait, as the Layout view does: drawn without the grids still to come, and said. - Usually no wait at all: when the map already shows its shadows, the grid is the same owner's and key, so it is ready at once.
- Left waiting: the thumbnail and the time series still wait as other exports do. They run off the frame or after the map has drawn.
- Test:
a_map_export_says_a_layer_drawn_without_its_cast_shadowsnow also checksmap_png.
| attribution: Vec<String>, | ||
| /// Tile layer errors already reported. | ||
| tile_errors: std::collections::HashSet<String>, | ||
| /// Tile layer errors, and why layers were drawn without their cast shadows (in the map or |
There was a problem hiding this comment.
Bug, medium confidence (from reading, not run). key.tiles only follows cache::generation() when fetches_tiles is true (line ~890). That is false for a map with no tile layers, no tile terrain and no layer read in place. A cast-shadow grid landing bumps the generation through notify_at, but the key stays equal and no new frame is drawn. The map view never reads tiles_pending, so nothing else triggers a redraw.
Failure case: a DEM-only project. A Style change draws without shadows, the landing repaints, and the key is unchanged, so the shadows only appear after a pan, zoom or other revision. Suggested fix: count a visible relief layer with shadow > 0 in fetches_tiles, or fold the generation into the key while a frame reports shadows to come. Check the live_rev branch, which pins key.tiles = 0. The PR's headless screenshot may have come from an unrelated redraw, so it's worth checking.
There was a problem hiding this comment.
Confirmed and fixed in 5541646. The first window check didn't show it because opening the Style pane resized the view, which redrew the map.
- The bug:
fetches_tileswas false for a map of a DEM alone, sokey.tilesstayed 0, and the landing'snotify_atmoved a generation the key never read. - The fix:
fetches_tilesnow also counts a map with a visible layer casting shadows (casts_shadows), asreads_in_placedoes, and the live-tiles shortcut that zeroeskey.tilesleaves such a map alone. - Checked in the window with
ui_shot, on a map with only the Ouro Preto DEM:- set the style with
shadow1; - give no input, just let 6 s of real time pass;
- the later frame differs from the one right after the change in 13.8 % of its pixels: the shadows came in by themselves.
- A trace of the key showed
fetches=false, and sotilesstuck at 0, before the fix.
- set the style with
- Test:
a_relief_casting_shadows_is_drawn_again_as_they_land. Not casting, or hidden, it doesn't count.
Code reviewBugs
Security
Performance
Quality
AGENTS.md
Earlier threads: 19 existing review threads. I did not re-raise any of them. The PNG and PDF notice thread ( Change sizeCompared with
|
…ing moved (opengeos#1782) Review round 8 of opengeos#1796: - The map view draws again on the tile cache's generation only for a map that fetches tiles, so on a map of a DEM alone the shadows' landing (which moves that generation) drew nothing until the map was moved. A map with a visible layer casting shadows now counts, as one reading in place does. Checked in the window (ui_shot): with no input after the style is set, the frame 6 s later shows the shadows (13.8% of it changed); before, it didn't. - A frame or export joining work under way kept no landing call, so the map wasn't drawn again when it landed: the work now keeps every landing call (32 at most) and makes them once done. - The app's Capture To Clipboard and Export Map download draw on the interface's thread without waiting for cast shadows still being worked out (`rustgis_render::without_shadow_wait`), and `map_png` says both why a layer was drawn without them and when they were still to come. The web map page says the reasons too. - Cast-shadow notices on Layout pages and layout exports are warnings, as they are in map exports: what was drawn is written. Tests: a relief casting shadows makes the map draw again as they land (and not without, or hidden); who joins work under way is told when it lands; `map_png` says a layer drawn without its shadows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Keeps the frame's cast-shadow flags and notices beside main's grid drawn under the attribution (opengeos#1797) in frame_canvas. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| // On the interface's thread: cast shadows still being worked out aren't waited for | ||
| // (the capture says so). | ||
| let png = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { | ||
| rustgis_engine::render::without_shadow_wait(|| rustgis_engine::cmd::share::map_png_fitted(session, 192.0)) |
There was a problem hiding this comment.
Never hold the window, medium-high confidence. This wrap keeps ui.captureMap off the wait, but the same UI-thread pattern still waits for shadow grids with no Cancel:
ui.printMap(lib.rs~3841) callsshare::map_pdfsynchronously. That goesmap_pdf_inner→render_drawing→render_complete_recorded→complete, which callswait_for_cast_shadowsbecauseNO_SHADOW_WAITis not set.ui.captureLayout(~3816) andui.exportLayout/ui.printLayout(layout_bytes, ~1678 and ~3625) calllayout::render, which goes throughrender_page(.., true)and waits too.
On the UI thread, a low sun on a large DEM blocks the window for seconds with no way to cancel. The PR's own numbers give about 10 s at 20° on one thread. Suggested fix: wrap these calls in rustgis_engine::render::without_shadow_wait, as captureMap now does, and let the existing notices report what was drawn without shadows. Alternatively, run them through exports::runs_in_background.
There was a problem hiding this comment.
Fixed in d453900, for the whole class rather than call by call. rustgis_render::on_the_interface_thread() now marks the app's thread at the top of logic and ui. Nothing drawn there waits for a grid: ui.printMap, ui.captureLayout, ui.exportLayout and ui.printLayout when they draw in place, Copy Map, the presentation and report previews, and an attached agent's snapshot. Each draws without the shadows still being worked out and says so ("…still being worked out: draw it again once the map shows them"). The previews are drawn again as the shadows land. Exports run off that thread (exports::runs_in_background) and wait as before. The per-call without_shadow_wait wraps in captureMap and exportMap are gone, since the thread mark covers them. Test: every_drawing_of_the_map_says_a_layer_drawn_without_its_cast_shadows (last part).
| PAGE_SHADOWS.with(|c| c.set(true)); | ||
| } | ||
| // Why a frame's layers were drawn without their cast shadows, kept for the page. | ||
| let notices = crate::take_shadow_notices(); |
There was a problem hiding this comment.
Never silent, medium-high confidence. Frame notices are collected into PAGE_NOTICES here and drained only by layout::shadow_notices(). Two callers never drain them:
presentation.rs(draw_page~403 andpdf_page~420) callslayout::renderandrender_drawingdirectly.report.rs(~283 and ~306) does the same for report pages.report.exportruns in the background.
If a frame's Shaded Relief layer is unavailable (read in place with only its overview, over 500,000 cells in the browser, or no memory), the presentation or report file is written with no shadows and no notice. The reason also stays in the thread-local buffer. On the UI thread it then shows up later as a stale notice in the Layout view, naming a layer from an unrelated page.
Suggested fix: drain shadow_notices() in those two modules and warn with the same wording as layout_bytes_inner. Alternatively, move the drain into render and render_drawing so each caller gets the notices.
There was a problem hiding this comment.
Fixed in d453900. PAGE_NOTICES, PAGE_SHADOWS, layout::shadow_notices, layout::shadows_to_come and the public take_shadow_notices and frame_shadows_to_come are gone. What frames say is read only through rustgis_render::shadows_said(f) -> (T, ShadowsSaid { notices, to_come }), which first clears what earlier frames on the thread left, so a stale notice can't reach the Layout view. In the engine, share::saying_shadows(s, what, …) wraps every drawing for someone: map image, map PDF, share_bytes (web map and web layer), layout export, presentation export (export_bytes) and report export (drawn_bytes). Off the session, Export Map's job uses shadow_warnings. A new test runs each of those entry points (map image, PDF, export, web map, layout PNG and PDF, presentation PDF and PNG, report PDF) with an unavailable grid and checks that each one names the layer.
| "webMap" => { | ||
| let mut m = map_of(s, p)?; | ||
| m.layers.iter_mut().filter(|l| shared_layer("webMap", l)).try_for_each(load_whole)?; | ||
| rustgis_render::take_shadow_notices(); |
There was a problem hiding this comment.
Never silent, medium confidence. The web map case drains and says the notices, but the webLayer branch of the same function (~line 511) calls rustgis_render::render_png(&mut only, &view) for a raster layer and never drains take_shadow_notices(). In the browser, a raster over 500,000 cells is therefore shared with no shadows and no warning. The same goes for any Unavailable result on that path. Suggested fix: take the notices after the render_png at ~511 and say_shadows them, as webMap does.
There was a problem hiding this comment.
Fixed in d453900: share_bytes as a whole now goes through share::saying_shadows, so webLayer (through web_map_html's render_png at ~511) says its rasters drawn without cast shadows, as webMap does. The web map case is in the new entry-point test.
Code reviewReviewed PR #1796 (Shaded Relief cast shadows) against Bugs
Security
Performance
Quality
Never silent (AGENTS)
AGENTS.md
Prior concerns
Change sizeCompared: the whole PR against main (merge base fbc5bab to PR head e67c85d).
Only the top 19 largest changes are listed; |
…t shadows, and the window never waits for them (opengeos#1782) What frames say of their cast shadows is read only through `rustgis_render::shadows_said`, which clears what earlier frames on the thread left: the per-page thread-locals and the public readers are gone, so no caller can forget to drain them or leave a stale notice for the Layout view. The engine draws through `share::saying_shadows` (map image and PDF, web map and web layer, layout, presentation and report exports) or, off the session, `shadow_warnings` (Export Map's job). `on_the_interface_thread` marks the app's thread in `logic` and `ui`: nothing drawn there (Copy Map and Copy Layout, printing a map or a layout, the presentation and report previews, an attached agent's snapshot) waits for a grid; it says the shadows were still being worked out. The previews are drawn again as they land. Exports drawn off that thread wait as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| let mut out = Vec::new(); | ||
| out.try_reserve_exact(cells).map_err(|_| Unlit::NoMemory)?; | ||
| out.resize(cells, 255u8); | ||
| let Some(data) = r.bands.get(band).map(|b| b.data.as_slice()).filter(|d| d.len() >= cells) else { |
There was a problem hiding this comment.
Never silent, low-medium confidence. When the band is missing or the data is shorter than cells, this returns an all-lit grid and nothing says so. The relief is then drawn with no cast shadows, and no notice names the layer. The new test stopped_flat_and_missing_cells asserts this silent result as intended.
AGENTS asks that a result drawn without data it should have says so, and that unusable results are errors. I'd return Err(Unlit::…) with a reason (for example "the band isn't in the raster"), so cast_shadows reports it as Unavailable. Flat ground and fully masked terrain are different: there the lit grid is correct, so they should stay silent. Confidence is low that this is reachable, since the style's band is clamped on symbology, but a reload with fewer bands could reach it.
There was a problem hiding this comment.
Not reachable from the app: raster.symbology refuses a band the raster doesn't have, and the grid is traced from the same raster the relief draws from (its key holds that raster). This guard only stops a mismatched grid from indexing past the end, as answered in the earlier round, so I'm leaving it as is.
| /// too (the worker has no probe of its own): the map, drawing again, starts it over. | ||
| fn result(&self, wait: bool) -> CastShadows { | ||
| let _waiting = wait.then(|| Waiting::on(&self.waiting)); | ||
| let Ok(mut done) = self.done.lock() else { return CastShadows::None }; |
There was a problem hiding this comment.
Never silent, low confidence. A poisoned done lock here returns CastShadows::None, and cast_shadows returns None on a poisoned jobs lock (line 765). In both, the layer is drawn with no shadows and no notice, for as long as the lock stays poisoned. The same goes for the landings lock in on_landing.
A poisoned lock means some earlier panic happened while it was held, so the grid state can't be trusted. I'd return CastShadows::Unavailable("its cast shadows aren't drawn: their state failed") on these paths, so the frame says it. Confidence is low: the locked sections don't panic in practice.
There was a problem hiding this comment.
A poisoned lock needs a panic while it was held. The sections under these locks only push, remove or swap entries and don't panic, and the trace itself runs under catch_unwind and lands with an error, so I'm leaving these paths as they are.
Code reviewI read the full diff (2,399 lines), the prior review threads, and the surrounding code for the changed paths. Most of the earlier concerns are already fixed in this head. I found no bug that would crash, drop data silently on the main path, or break the wasm build. I posted two low-confidence inline comments and left the rest as nits. Bugs
Security
Performance
Quality
AGENTS.md
Prior threads
Change sizeCompared with
Largest file-size changes are in the first table; the other files in the PR are unchanged in size to within a few bytes. The stats file reports |
| ui.ctx().load_texture("presentation_preview", img, egui::TextureOptions::LINEAR) | ||
| }); | ||
| app.docs.preview = Some((key, tex)); | ||
| app.docs.preview = Some((crate::layoutview::preview_key(&key, &said), tex)); |
There was a problem hiding this comment.
Bug, low confidence. preview_key reads cache::generation() after the preview is drawn and its texture is loaded. If a shadow grid lands in that window, the key records the new generation. preview_current then accepts the frame, which was drawn without that grid, and the preview stays unshadowed until something else moves the generation. Nothing else in this path redraws it.
Suggested direction: read the generation before the draw (before shadows_said), and pass it to preview_key in place of the one read inside it. A landing during the draw then differs from the recorded value, so the preview is drawn again. The same applies to reportpane.rs:347.
| /// Waits for the cast shadows of the map's visible Shaded Relief layers, as an export does (it | ||
| /// runs off the interface's thread); the wait gives up when the run is cancelled. | ||
| fn wait_for_cast_shadows(map: &Map) { | ||
| for layer in map.layers.iter().filter(|l| l.visible) { |
There was a problem hiding this comment.
Performance, medium confidence. wait_for_cast_shadows checks only l.visible. render_frame (line ~3989) also skips a layer outside the scale range (!layer.in_scale_range(scale)) and a layer held by a group. An export therefore waits for, and starts, whole-raster shadow grids for layers that are never drawn at that scale. A low sun on a large DEM can make that wait several seconds, and it can end in a cancelled export for no reason.
Suggested direction: pass the map's scale (map.ground_res_at(&view.camera) / rustgis_geom::PIXEL_M, as render_frame computes it) and filter with the same test render_frame uses: l.visible && l.in_scale_range(scale), and skip held layers as well.
Code reviewScope: all 2,398 diff lines, the staged prior-review threads, and the code they point at. I found three issues that aren't already raised. I posted two as inline comments. The third is on a line outside the diff's changed hunks, so GitHub rejected it as an inline comment and it appears here only. Bugs
Security
Performance
Quality
AGENTS.md
Already raised, left alone
Change sizeCompared with the base branch, merge base fbc5bab to PR head d453900.
Sizes are file sizes from |
Part of #1782: the cast shadows. Sky visibility will follow in a second pull request, as the issue suggested, so each stays reviewable.
What it does
Shaded Relief gets one more setting, Cast shadows (
raster.symbology"shadow", 0 to 1): how dark the slopes the terrain hides from the sun are drawn. A valley behind a ridge is now in shadow, with a soft edge, instead of looking as lit as a slope in the open.raster.symbology, a Shaded Relief keeps its own value (as Stretch keeps its inversion); a new one casts none.How
rustgis_map::shadow::lighttraces each cell toward the sun's azimuth, one column or row at a time. It reads heights between cells (bilinear) and keeps the steepest angle it meets. The sun is a disc 5° either side of its altitude, and the light left is the share of that disc above the angle. Reading the nearest cell instead over-shadows diagonal suns (ray_shade marks lit slopes as shadowed on diagonal azimuths (cell read at the wrong distance) franciscoparrao/surtgis#153).1 − shadow × (1 − light), so the ambient light stays (ambient + direct × shadow, as the issue proposed). The grid doesn't depend on the strength, so changing the strength is immediate.RasterLayer::cast_shadowsworks the grid out once per pixels, band, sun, Z factor and mask, on a thread of its own, and keeps it.rustgis_tiles::cache::notify_at).rustgis_data::threads::max()threads, with the same grid on one thread, bit for bit (tested). A thread the system refuses runs its part on the caller.rustgis_data::progress).layout::shadows_to_comesays so, and the page is drawn again as they land, natively too.The notice shows in the map view, in the Layout view and with a map or layout export. The PMTiles export refuses in those cases, as it refuses a part drawn from less than it needs.
Measured
Ouro Preto and Serra do Caraça, Minas Gerais: Copernicus DEM GLO-30 reprojected to UTM 23S at 30 m, 1966 × 1827 cells (3.59 million). The whole DEM was exported at 1600 × 1500 with
rustgis-cli(release), sun 315° / 30°, three runs each, on a 16-thread Windows PC with other builds running, so the times are indicative.shadow0 (today)shadow0.8--threads 1--threads 16With 1 and with 16 threads, the export is the same, pixel for pixel.
A lower sun traces farther. In a 1:75,000 view of the Caraça with the sun at 20°, one run each took 2.9 → 10.7 s on one thread and 2.5 s with shadows on 16.
With the sun at 5° or lower, the disc's lower edge is at or below the horizon, so over gentle ground a cell traces to the raster's edge. The whole DEM, one run each:
--threads 16, 0 → 0.8--threads 1, 0 → 0.8How much it changes depends on the sun:
Tests
crates/map/src/shadow.rs):h / tan(alt + 5°)and fully lit pasth / tan(alt − 5°).RasterLayer::cast_shadows:raster.symbologykeeps the setting when it is left out, clamps it to 0–1, saves it only when it isn't 0, and a map export with shadows draws.hostile_params_never_panicpasses.Saved files
shadowis a new field in a saved project, written only when it isn't 0.crates/engine/src/project.rssaysFORMATadvances when an older reader would lose meaningful saved information on resaving.@giswqs, should this PR advance
FORMAT, or leave it as #1739 did?Limits
cargo check --target wasm32-unknown-unknowncan't run on this PC (zstd-sys needs clang).Screenshots
Antes / Depois = before / after:
rustgis-clifrom this branch.shadow0 (left) and 0.8 (right).shadow0, which drawsmain's pixels (tested).The Style pane (new UI): Cast shadows under Z factor, with the same DEM at 1:50,000. A headless
ui_shotof the window; the map shows its shadows once they had landed (the script waited 4 s after setting the style).Where the code comes from
Written here from the geometry. The bilinear read and the early stop follow what I found testing
surtgis-relief(MIT OR Apache-2.0) for the issue, but no code is copied from it. rayshader (GPL-3) was looked at only for what it does, through its documentation.🤖 Generated with Claude Code
Summary by CodeRabbit