Merge PR #1100: classify highlights in one pass

src/scene/pipeline/mod.rs

@@ -2612,6 +2612,72 @@ impl Pipeline {
     /// refreshes just this overlay instead of re-tessellating the model. The
     /// xray pass applies neither scissor nor mesh-edge skip, so everything
     /// merges into one order-preserving run.
+    /// Split highlighted wires into analytic instances and the wires that need
+    /// real geometry, in one extraction pass per wire.
+    #[allow(clippy::type_complexity)]
+    fn classify_highlight_wires<'a>(
+        wires: &[&'a WireModel],
+        color: Option<[f32; 4]>,
+        depth_map: &rustc_hash::FxHashMap<u64, [f32; 2]>,
+    ) -> (
+        Vec<CircleInstance>,
+        Vec<EllipseInstance>,
+        Vec<&'a WireModel>,
+        Vec<&'a WireModel>,
+    ) {
+        use crate::par::prelude::*;
+        // Pure per-wire work, so it fans out; `collect` on an indexed
+        // parallel iterator keeps the order, and the order is what decides
+        // the instance layout.
+        let per: Vec<(Option<Vec<CircleInstance>>, Option<Vec<EllipseInstance>>, bool)> =
+            wires
+                .par_iter()
+                .map(|&wire| {
+                    if wire.render_instance.is_some() {
+                        return (None, None, true);
+                    }
+                    let depth = wire_gpu::wire_draw_depth(wire, depth_map);
+                    let mut circles = circle_gpu::extract_circle_instances(wire, depth);
+                    let mut ellipses = ellipse_gpu::extract_ellipse_instances(wire, depth);
+                    if let Some(color) = color {
+                        if let Some(instances) = circles.as_mut() {
+                            for instance in instances {
+                                instance.color = color;
+                            }
+                        }
+                        if let Some(instances) = ellipses.as_mut() {
+                            for instance in instances {
+                                instance.color = color;
+                            }
+                        }
+                    }
+                    (circles, ellipses, false)
+                })
+                .collect();
+
+        let mut circles: Vec<CircleInstance> = Vec::new();
+        let mut ellipses: Vec<EllipseInstance> = Vec::new();
+        let mut regular: Vec<&'a WireModel> = Vec::new();
+        let mut blocks: Vec<&'a WireModel> = Vec::new();
+        for (&wire, (wire_circles, wire_ellipses, is_block)) in wires.iter().zip(per) {
+            if is_block {
+                blocks.push(wire);
+                continue;
+            }
+            let shaped = wire_circles.is_some() || wire_ellipses.is_some();
+            if let Some(instances) = wire_circles {
+                circles.extend(instances);
+            }
+            if let Some(instances) = wire_ellipses {
+                ellipses.extend(instances);
+            }
+            if !shaped {
+                regular.push(wire);
+            }
+        }
+        (circles, ellipses, regular, blocks)
+    }
+
     pub fn upload_selected_wires(
         &mut self,
         device: &wgpu::Device,
@@ -2635,11 +2701,20 @@ impl Pipeline {
         // O(highlighted), no per-wire string parse or deep geometry clone.
         let mut selected_wires: Vec<&WireModel> = Vec::new();
         let mut hover_wires: Vec<&WireModel> = Vec::new();
+        // The index is built by walking the wires in order, so each handle's
+        // slots come out ascending, and `patch_handle_index` only shifts them
+        // by a constant, which keeps that. Cloning and re-sorting per handle
+        // was one allocation and one sort for every selected entity — 186 468
+        // of each on a whole-drawing selection. The assert holds the invariant
+        // in place: if it ever breaks, a debug build says so here rather than
+        // drawing the highlight in the wrong order.
         for h in selected {
             if let Some(idxs) = self.wire_handle_index.get(&h.value()) {
-                let mut slots = idxs.clone();
-                slots.sort_unstable();
-                for &i in &slots {
+                debug_assert!(
+                    idxs.windows(2).all(|pair| pair[0] <= pair[1]),
+                    "wire_handle_index slots must stay ascending",
+                );
+                for &i in idxs {
                     if let Some(w) = wires.get(i as usize) {
                         if !w.display_visible {
                             continue;
@@ -2651,9 +2726,7 @@ impl Pipeline {
         }
         for h in hovered.iter().filter(|handle| !selected.contains(handle)) {
             if let Some(idxs) = self.wire_handle_index.get(&h.value()) {
-                let mut slots = idxs.clone();
-                slots.sort_unstable();
-                for &i in &slots {
+                for &i in idxs {
                     if let Some(w) = wires.get(i as usize) {
                         if !w.display_visible {
                             continue;
@@ -2673,26 +2746,42 @@ impl Pipeline {
                 hover_wires.push(wire);
             }
         }
-        let mut selected_circles: Vec<CircleInstance> = Vec::new();
-        let circle_sel_tint = selected_tint.unwrap_or(WireModel::SELECTED);
-        for &wire in &selected_wires {
-            let depth = wire_gpu::wire_draw_depth(wire, depth_map);
-            if let Some(insts) = circle_gpu::extract_circle_instances(wire, depth) {
-                for mut inst in insts {
-                    inst.color = circle_sel_tint;
-                    selected_circles.push(inst);
-                }
-            }
-        }
-        for &wire in &hover_wires {
-            let depth = wire_gpu::wire_draw_depth(wire, depth_map);
-            if let Some(insts) = circle_gpu::extract_circle_instances(wire, depth) {
-                for mut inst in insts {
-                    inst.color = WireModel::HOVER;
-                    selected_circles.push(inst);
-                }
-            }
-        }
+        let t_gather = perf_started.map(|t| t.elapsed().as_secs_f64() * 1000.0);
+        // One extraction pass per wire, where there used to be four.
+        //
+        // The old shape asked each extractor twice: once at the draw depth to
+        // build the instances, then once more at depth 0.0 purely to ask
+        // *whether* the wire is a circle or an ellipse — allocating a vector
+        // each time and dropping it. Neither extractor's `Some`/`None` answer
+        // depends on the depth: every `return None` in them turns on the
+        // geometry (which `TangentGeom` variant, the radius, the axis lengths,
+        // the parameters), and the depth is only written into the instance,
+        // never read to decide. So the extraction that builds the instances
+        // already answers the classification, and the second pair is pure
+        // waste — 116 ms of shapes plus 57 ms of splitting, out of a 299 ms
+        // highlight at 186 468 entities.
+        //
+        // A wire with a `render_instance` is a block wire, and both extractors
+        // reject those outright, so it skips extraction altogether.
+        // Selected keeps its colour unless a tint is configured; hover is
+        // always recoloured. Both vectors carry selected first, then hover,
+        // exactly as the four separate loops produced them.
+        let (mut selected_circles, mut selected_ellipses, selected_regular, selected_blocks) =
+            Self::classify_highlight_wires(
+                &selected_wires,
+                Some(selected_tint.unwrap_or(WireModel::SELECTED)),
+                depth_map,
+            );
+        let (hover_circles, hover_ellipses, hover_regular, hover_blocks) =
+            Self::classify_highlight_wires(
+                &hover_wires,
+                Some(WireModel::HOVER),
+                depth_map,
+            );
+        selected_circles.extend(hover_circles);
+        selected_ellipses.extend(hover_ellipses);
+
+        let t_shapes = perf_started.map(|t| t.elapsed().as_secs_f64() * 1000.0);
         self.gpu_selected_circles = if selected_circles.is_empty() {
             vec![]
         } else {
@@ -2703,27 +2792,6 @@ impl Pipeline {
                 &selected_circles,
             )]
         };
-
-        let mut selected_ellipses: Vec<EllipseInstance> = Vec::new();
-        let ellipse_sel_tint = selected_tint.unwrap_or(WireModel::SELECTED);
-        for &wire in &selected_wires {
-            let depth = wire_gpu::wire_draw_depth(wire, depth_map);
-            if let Some(insts) = ellipse_gpu::extract_ellipse_instances(wire, depth) {
-                for mut inst in insts {
-                    inst.color = ellipse_sel_tint;
-                    selected_ellipses.push(inst);
-                }
-            }
-        }
-        for &wire in &hover_wires {
-            let depth = wire_gpu::wire_draw_depth(wire, depth_map);
-            if let Some(insts) = ellipse_gpu::extract_ellipse_instances(wire, depth) {
-                for mut inst in insts {
-                    inst.color = WireModel::HOVER;
-                    selected_ellipses.push(inst);
-                }
-            }
-        }
         self.gpu_selected_ellipses = if selected_ellipses.is_empty() {
             vec![]
         } else {
@@ -2734,35 +2802,7 @@ impl Pipeline {
                 &selected_ellipses,
             )]
         };
-
-        let selected_regular: Vec<&WireModel> = selected_wires
-            .iter()
-            .copied()
-            .filter(|wire| {
-                wire.render_instance.is_none()
-                    && circle_gpu::extract_circle_instances(wire, 0.0).is_none()
-                    && ellipse_gpu::extract_ellipse_instances(wire, 0.0).is_none()
-            })
-            .collect();
-        let hover_regular: Vec<&WireModel> = hover_wires
-            .iter()
-            .copied()
-            .filter(|wire| {
-                wire.render_instance.is_none()
-                    && circle_gpu::extract_circle_instances(wire, 0.0).is_none()
-                    && ellipse_gpu::extract_ellipse_instances(wire, 0.0).is_none()
-            })
-            .collect();
-        let selected_blocks: Vec<&WireModel> = selected_wires
-            .iter()
-            .copied()
-            .filter(|wire| wire.render_instance.is_some())
-            .collect();
-        let hover_blocks: Vec<&WireModel> = hover_wires
-            .iter()
-            .copied()
-            .filter(|wire| wire.render_instance.is_some())
-            .collect();
+        let t_split = perf_started.map(|t| t.elapsed().as_secs_f64() * 1000.0);
         let mut gpu = if let Some(tint) = selected_tint {
             WireGpu::from_highlight_refs(
                 device,
@@ -2783,6 +2823,7 @@ impl Pipeline {
             depth_map,
             self.wire_const_bgl.as_ref(),
         ));
+        let t_regular = perf_started.map(|t| t.elapsed().as_secs_f64() * 1000.0);
         self.gpu_selected_wires = gpu;
         let mut block_gpu = if let Some(tint) = selected_tint {
             BlockWireGpu::from_wires(
@@ -2816,6 +2857,24 @@ impl Pipeline {
         if let Some(started) = perf_started {
             let elapsed_ms = started.elapsed().as_secs_f64() * 1000.0;
             if elapsed_ms >= 1.0 {
+                // Cumulative marks, differenced here: gathering the wires
+                // from the handle index, classifying them and extracting their
+                // analytic instances, uploading those, building the regular
+                // wire instances, and the block ones. A one-time cost when the
+                // selection changes, which puts it directly between the user's
+                // second click and seeing the selection.
+                let gather = t_gather.unwrap_or(0.0);
+                let classify = t_shapes.unwrap_or(0.0);
+                let analytic = t_split.unwrap_or(0.0);
+                let regular = t_regular.unwrap_or(0.0);
+                crate::perf_record!(
+                    "[perf] wire-highlight-detail gather={gather:.1} classify={:.1} \
+analytic={:.1} regular={:.1} blocks={:.1}",
+                    classify - gather,
+                    analytic - classify,
+                    regular - analytic,
+                    elapsed_ms - regular,
+                );
                 crate::perf_record!(
                     "[perf] wire-highlight {:>7.1}ms handles={} wires={}",
                     elapsed_ms,
@@ -5627,3 +5686,97 @@ impl iced::widget::shader::Pipeline for MultiPipeline {
         }
     }
 }
+
+#[cfg(test)]
+mod highlight_classification_tests {
+    use super::*;
+    use crate::scene::model::wire_model::TangentGeom;
+
+    fn plain(name: &str) -> WireModel {
+        WireModel::solid(
+            name.to_string(),
+            vec![[0.0, 0.0, 0.0], [1.0, 1.0, 0.0]],
+            [1.0; 4],
+            false,
+        )
+    }
+
+    fn circle(name: &str) -> WireModel {
+        let mut wire = plain(name);
+        wire.tangent_geoms.push(TangentGeom::PlanarCircle {
+            center: [0.0, 0.0, 0.0],
+            axis_x: [1.0, 0.0, 0.0],
+            axis_y: [0.0, 1.0, 0.0],
+            radius: 2.0,
+        });
+        wire
+    }
+
+    fn ellipse(name: &str) -> WireModel {
+        let mut wire = plain(name);
+        wire.tangent_geoms.push(TangentGeom::PlanarEllipse {
+            center: [0.0, 0.0, 0.0],
+            major_axis: [2.0, 0.0, 0.0],
+            normal: [0.0, 0.0, 1.0],
+            minor_axis_ratio: 0.5,
+            start_param: 0.0,
+            end_param: std::f64::consts::TAU,
+        });
+        wire
+    }
+
+    // The classification used to be a second pair of extractions at depth 0.0,
+    // run purely to ask which bucket a wire belongs in. It now rides the
+    // extraction that builds the instances. Nothing renders in a test, so a
+    // wrong answer here would show up only as circles drawn wrong when
+    // selected — this asserts the buckets directly, against the predicate the
+    // old code used.
+    #[test]
+    fn each_wire_lands_in_the_bucket_the_old_predicate_chose() {
+        let wires = vec![plain("1"), circle("2"), ellipse("3"), plain("4")];
+        let refs: Vec<&WireModel> = wires.iter().collect();
+        let depth_map = rustc_hash::FxHashMap::default();
+
+        let (circles, ellipses, regular, blocks) =
+            Pipeline::classify_highlight_wires(&refs, None, &depth_map);
+
+        for wire in &refs {
+            let was_regular = wire.render_instance.is_none()
+                && circle_gpu::extract_circle_instances(wire, 0.0).is_none()
+                && ellipse_gpu::extract_ellipse_instances(wire, 0.0).is_none();
+            assert_eq!(
+                was_regular,
+                regular.iter().any(|kept| std::ptr::eq(*kept, *wire)),
+                "wire {} disagrees with the old predicate",
+                wire.name,
+            );
+        }
+
+        assert_eq!(regular.len(), 2, "two plain lines");
+        assert_eq!(circles.len(), 1, "one circle instance");
+        assert_eq!(ellipses.len(), 1, "one ellipse instance");
+        assert!(blocks.is_empty(), "no block wires in this sample");
+    }
+
+    // Hover recolours every instance; a selection only recolours when a tint is
+    // configured. Both go through the same argument, so it has to actually
+    // reach the instances.
+    #[test]
+    fn the_colour_override_reaches_the_instances() {
+        let wires = vec![circle("2"), ellipse("3")];
+        let refs: Vec<&WireModel> = wires.iter().collect();
+        let depth_map = rustc_hash::FxHashMap::default();
+
+        let (tinted_circles, tinted_ellipses, _, _) =
+            Pipeline::classify_highlight_wires(&refs, Some(WireModel::HOVER), &depth_map);
+        assert_eq!(tinted_circles[0].color, WireModel::HOVER);
+        assert_eq!(tinted_ellipses[0].color, WireModel::HOVER);
+
+        let (plain_circles, _, _, _) =
+            Pipeline::classify_highlight_wires(&refs, None, &depth_map);
+        assert_ne!(
+            plain_circles[0].color, WireModel::HOVER,
+            "without a tint the extractor's own colour stands",
+        );
+    }
+}

src/scene/pipeline/wire_gpu.rs

@@ -1148,8 +1148,14 @@ impl WireGpu {
 
         let _ = const_bgl;
         let max_packed_instances = super::gpu_budget::max_elements::<PackedWireInstance>(device);
+        // Same fan-out the native path above uses: emitting a wire reads that
+        // wire and the depth map and writes nothing shared, and a selection
+        // large enough to be slow is exactly where it pays. `collect` on an
+        // indexed parallel iterator keeps the order, which the instance indices
+        // depend on.
+        use crate::par::prelude::*;
         let per: Vec<Vec<PackedWireInstance>> = wires
-            .iter()
+            .par_iter()
             .map(|wire| {
                 emit_wire_packed(wire, color, wire_draw_depth(wire, depth_map))
             })