Add multithreaded precomputation of floodfill #696

louis-e · 2026-01-06T17:15:22Z

No description provided.

github-actions · 2026-01-06T17:17:53Z

⏱️ Benchmark run finished in 0m 27s
🧠 Peak memory usage: 809 MB

📈 Compared against baseline: 30s
🧮 Delta: -3s
🔢 Commit: 7bbee28

🟢 Generation time is unchanged.

📅 Last benchmark: 2026-01-06 18:28:44 UTC

You can retrigger the benchmark by commenting retrigger-benchmark.

Copilot

Pull request overview

This PR adds multithreaded precomputation of flood fill operations to improve performance by parallelizing polygon filling across multiple CPU cores. The implementation introduces a new FloodFillCache module that precomputes flood fills for all relevant elements before the main processing loop, then retrieves cached results during sequential element processing.

Key changes:

New floodfill_cache.rs module providing parallel precomputation and caching of flood fill results
Rayon thread pool configuration with CPU usage cap (90%) to maintain system responsiveness
Refactored element processing functions to accept and use the flood fill cache

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Description
src/floodfill_cache.rs	New module implementing FloodFillCache with parallel precomputation, cache retrieval methods, and thread pool configuration
src/main.rs	Added thread pool configuration call at CLI entry point
src/gui.rs	Added thread pool configuration call at GUI entry point
src/data_processing.rs	Added cache precomputation before element processing and passed cache to all element processing functions
src/element_processing/natural.rs	Updated to accept and use flood fill cache instead of computing on demand
src/element_processing/leisure.rs	Updated to accept and use flood fill cache instead of computing on demand
src/element_processing/landuse.rs	Updated to accept and use flood fill cache instead of computing on demand
src/element_processing/highways.rs	Updated to accept and use flood fill cache instead of computing on demand
src/element_processing/buildings.rs	Updated to accept and use flood fill cache instead of computing on demand
src/element_processing/amenities.rs	Updated to accept and use flood fill cache instead of computing on demand

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/floodfill_cache.rs

src/element_processing/leisure.rs

src/floodfill_cache.rs

src/element_processing/natural.rs

src/element_processing/landuse.rs

src/floodfill_cache.rs

src/main.rs

src/floodfill_cache.rs

louis-e · 2026-01-06T18:26:13Z

retrigger-benchmark

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

src/element_processing/buildings.rs:1579

The bridge_y_offset calculation is duplicated. It's computed inside the loop (lines 1541-1550) and then again after the flood fill (lines 1570-1579). The first calculation should be moved outside the loop before line 1536, as it's loop-invariant and only depends on element tags. This eliminates redundant computation and improves code maintainability.

        // Calculate bridge level based on the "level" tag
        let bridge_y_offset = if let Some(level_str) = element.tags.get("level") {
            if let Ok(level) = level_str.parse::<i32>() {
                (level * 3) + 1
            } else {
                1 // Default elevation
            }
        } else {
            1 // Default elevation
        };

        // Create bridge path using Bresenham's line
        if let Some(prev) = previous_node {
            let bridge_points: Vec<(i32, i32, i32)> =
                bresenham_line(prev.0, bridge_y_offset, prev.1, x, bridge_y_offset, z);

            for (bx, by, bz) in bridge_points {
                // Place railing blocks
                editor.set_block(railing_block, bx, by + 1, bz, None, None);
                editor.set_block(railing_block, bx, by, bz, None, None);
            }
        }

        previous_node = Some((x, z));
    }

    // Flood fill the area between the bridge path nodes (uses cache)
    let bridge_area: Vec<(i32, i32)> = flood_fill_cache.get_or_compute(element, _floodfill_timeout);

    // Calculate bridge level based on the "level" tag
    let bridge_y_offset = if let Some(level_str) = element.tags.get("level") {
        if let Ok(level) = level_str.parse::<i32>() {
            (level * 3) + 1
        } else {
            1 // Default elevation
        }
    } else {
        1 // Default elevation
    };

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/floodfill_cache.rs

src/element_processing/amenities.rs

src/element_processing/buildings.rs

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-06T18:48:31Z

src/floodfill_cache.rs

+    fn way_needs_flood_fill(way: &ProcessedWay) -> bool {
+        way.tags.contains_key("building")
+            || way.tags.contains_key("building:part")
+            || way.tags.contains_key("landuse")
+            || way.tags.contains_key("leisure")
+            || way.tags.contains_key("amenity")
+            || way
+                .tags
+                .get("natural")
+                .map(|v| v != "tree")
+                .unwrap_or(false)
+            // Highway areas (like pedestrian plazas) use flood fill when area=yes
+            || (way.tags.contains_key("highway")
+                && way.tags.get("area").map(|v| v == "yes").unwrap_or(false))
+    }


The way_needs_flood_fill logic doesn't account for all cases where flood fill might be used. For example, checking the amenities.rs code, there are amenity types like "parking" and "shelter" that use flood fill, but the function only checks if "amenity" tag exists, not specific amenity types. However, this is actually correct because it caches all amenities. Consider adding a comment to clarify that the presence of any tag key is sufficient, even if only certain values of that tag use flood fill, since caching extra results is harmless.

Copilot · 2026-01-06T18:48:32Z

src/element_processing/amenities.rs

 use crate::bresenham::bresenham_line;
 use crate::coordinate_system::cartesian::XZPoint;
-use crate::floodfill::flood_fill_area;
+use crate::floodfill::flood_fill_area; // Needed for inline amenity flood fills


This import of flood_fill_area is kept for "inline amenity flood fills" according to the comment, but it doesn't appear to be used anywhere in the visible changes. If all flood fill operations now go through the cache, this import may be unnecessary and should be removed to avoid confusion.

Suggested change

use crate::floodfill::flood_fill_area; // Needed for inline amenity flood fills

Copilot · 2026-01-06T18:48:32Z

src/gui.rs

                fillground: fillground_enabled,
                debug: false,
-                timeout: Some(std::time::Duration::from_secs(floodfill_timeout)),
+                timeout: Some(std::time::Duration::from_secs(40)),


The hardcoded timeout value of 40 seconds replaces the previously configurable floodfill_timeout parameter. This removes user flexibility and could cause issues for users who previously needed different timeout values. Consider either keeping the parameter or documenting why 40 seconds is now the fixed choice.

Copilot · 2026-01-06T18:48:32Z

src/gui.rs

+    // Configure thread pool with 90% CPU cap to keep system responsive
+    crate::floodfill_cache::configure_rayon_thread_pool(0.9);


The thread pool is configured with a 90% CPU cap in both CLI and GUI modes. However, if both run_cli() and run_gui() could potentially be called in the same process (or if the rayon global pool is already configured), the second call will silently fail according to the error handling at line 166-168. Consider adding a warning message or check to inform users if the thread pool was already configured.

Copilot · 2026-01-06T18:48:33Z

src/floodfill_cache.rs

+        way: &ProcessedWay,
+        timeout: Option<&Duration>,
+    ) -> Vec<(i32, i32)> {
+        if let Some(cached) = self.way_cache.get(&way.id) {


The cached flood fill results are cloned on every access (line 78). For large polygons with many coordinates, this could have a performance impact if the same result is accessed multiple times. Consider using Arc or Rc to share the data without cloning, or document that this trade-off is acceptable since each polygon is typically only accessed once during sequential processing.

Suggested change

if let Some(cached) = self.way_cache.get(&way.id) {

if let Some(cached) = self.way_cache.get(&way.id) {

// NOTE: We intentionally clone here to return an owned Vec.

// In the intended usage, each cached flood fill result is normally

// accessed at most once during the sequential processing phase,

// so this per-access clone cost is acceptable and keeps the API

// simple (no Arc/Rc in the public interface).

Copilot · 2026-01-06T18:48:33Z

src/gui/js/main.js

-    // Validate floodfill_timeout and ground_level
-    floodfill_timeout = isNaN(floodfill_timeout) || floodfill_timeout < 0 ? 20 : floodfill_timeout;
+    // Validate ground_level
    ground_level = isNaN(ground_level) || ground_level < -62 ? 20 : ground_level;


The validation logic for ground_level is incorrect. When ground_level is -62 (the default), the condition ground_level < -62 is false, but then if it were NaN or less than -62, it would be set to 20, which seems inconsistent. The fallback value of 20 doesn't make sense in the context where the default is -62. This should likely be reviewed to ensure the validation logic matches the intended behavior.

Suggested change

ground_level = isNaN(ground_level) || ground_level < -62 ? 20 : ground_level;

ground_level = isNaN(ground_level) || ground_level < -62 ? -62 : ground_level;

Add multithreaded precomputation of floodfill

118335b

Copilot AI review requested due to automatic review settings January 6, 2026 17:15

Copilot started reviewing on behalf of louis-e January 6, 2026 17:15 View session

Remove floodfill timeout parameter and refine minor changes

4215e76

Copilot AI reviewed Jan 6, 2026

View reviewed changes

louis-e added 2 commits January 6, 2026 18:21

Remove floodfill timeout parameter

ffbc5e5

Address code review feedback

fa3384c

louis-e requested a review from Copilot January 6, 2026 18:25

Copilot started reviewing on behalf of louis-e January 6, 2026 18:26 View session

Copilot AI reviewed Jan 6, 2026

View reviewed changes

src/floodfill_cache.rs Outdated Show resolved Hide resolved

src/floodfill_cache.rs Outdated Show resolved Hide resolved

src/element_processing/amenities.rs Outdated Show resolved Hide resolved

src/element_processing/buildings.rs Outdated Show resolved Hide resolved

louis-e added 2 commits January 6, 2026 19:36

Address code review feedback

0fef27e

Add clarifying comment

6fa76bc

louis-e requested a review from Copilot January 6, 2026 18:44

Copilot started reviewing on behalf of louis-e January 6, 2026 18:44 View session

Copilot AI reviewed Jan 6, 2026

View reviewed changes

louis-e merged commit 99d1f8e into main Jan 6, 2026
8 checks passed

louis-e deleted the floodfill-precompute branch January 6, 2026 19:00

louis-e restored the floodfill-precompute branch January 6, 2026 19:01

louis-e deleted the floodfill-precompute branch January 6, 2026 19:01

		// Configure thread pool with 90% CPU cap to keep system responsive
		crate::floodfill_cache::configure_rayon_thread_pool(0.9);

-        if let Some(cached) = self.way_cache.get(&way.id) {
+        if let Some(cached) = self.way_cache.get(&way.id) {
+            // NOTE: We intentionally clone here to return an owned Vec.
+            // In the intended usage, each cached flood fill result is normally
+            // accessed at most once during the sequential processing phase,
+            // so this per-access clone cost is acceptable and keeps the API
+            // simple (no Arc/Rc in the public interface).

	ground_level = isNaN(ground_level) \|\| ground_level < -62 ? 20 : ground_level;
	ground_level = isNaN(ground_level) \|\| ground_level < -62 ? -62 : ground_level;

Uh oh!

Add multithreaded precomputation of floodfill #696

Add multithreaded precomputation of floodfill #696

Uh oh!

Conversation

louis-e commented Jan 6, 2026

Uh oh!

github-actions bot commented Jan 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

louis-e commented Jan 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions bot commented Jan 6, 2026 •

edited

Loading