From 03662d201644759570c11bde02134918e866ac6e Mon Sep 17 00:00:00 2001
From: Dion Moult <dion@thinkmoult.com>
Date: Wed, 15 Apr 2026 18:30:03 +1000
Subject: [PATCH] ifcviewer: fix pick-pass cull corruption and cached-model ID
 collisions
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Two stability bugs:

1. Clicking an object left the scene with wrong shading until the camera
   moved.  The pick pass re-culls every model with its own parameters
   (min_pixel_radius=0, no HiZ) and overwrites each model's visible_ssbo
   and indirect buffer.  The next render() saw an unchanged camera,
   skipped the cull via the have_cached_cull_ shortcut, and drew the
   stale pick-pass buffers.  Fix: invalidate have_cached_cull_ at the
   end of pickObjectAt().

2. Loading two sidecar-cached models made the second model's picked
   properties resolve to the first model's elements.  Sidecars store raw
   object_id / model_id values from the session that wrote them, and
   both files start at object_id=1, so element_map_ entries collided.
   Fix: on load, rebase every PackedElementInfo and InstanceCpu by
   (next_object_id_ - min_id_in_sidecar) and overwrite model_id with
   the freshly-assigned handle before the elements hit element_map_.

Also document both in the README — the pick-pass note under 3A
contribution culling, the sidecar rebase under the sidecar format
section.
---
 src/ifcviewer/MainWindow.cpp     | 27 ++++++++++++++++++++++-----
 src/ifcviewer/README.md          | 15 +++++++++++++++
 src/ifcviewer/ViewportWindow.cpp |  7 +++++++
 3 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/src/ifcviewer/MainWindow.cpp b/src/ifcviewer/MainWindow.cpp
index 0e8162f043..e75f7cf0dd 100644
--- a/src/ifcviewer/MainWindow.cpp
+++ b/src/ifcviewer/MainWindow.cpp
@@ -248,11 +248,28 @@ void MainWindow::applySidecarData(ModelId mid, SidecarData data) {
     QElapsedTimer t;
     t.start();
 
-    // Update next_object_id_ past all objects in this model before the
-    // extracted `elements` is moved out of `data`.
-    for (const auto& elem : data.elements) {
-        if (elem.object_id >= next_object_id_)
-            next_object_id_ = elem.object_id + 1;
+    // Sidecars store raw object_ids and model_ids from the session that wrote
+    // them.  On load we must rebase both onto the current session's ID space,
+    // or two cached models collide (both starting at object_id=1, both
+    // claiming the original model_id).  Offset by (next_object_id_ - min_id)
+    // so the first cached object takes the next free slot.
+    uint32_t min_oid = UINT32_MAX;
+    for (const auto& pe : data.elements) {
+        if (pe.object_id < min_oid) min_oid = pe.object_id;
+    }
+    uint32_t oid_offset = 0;
+    if (!data.elements.empty() && min_oid < UINT32_MAX) {
+        oid_offset = next_object_id_ - min_oid;
+    }
+    for (auto& pe : data.elements) {
+        pe.object_id += oid_offset;
+        pe.model_id   = mid;
+        if (pe.object_id >= next_object_id_)
+            next_object_id_ = pe.object_id + 1;
+    }
+    for (auto& inst : data.instances) {
+        inst.object_id += oid_offset;
+        inst.model_id   = mid;
     }
 
     // Hand off geometry to GPU in a single call.
diff --git a/src/ifcviewer/README.md b/src/ifcviewer/README.md
index 8c09ed661f..70540abfb1 100644
--- a/src/ifcviewer/README.md
+++ b/src/ifcviewer/README.md
@@ -307,6 +307,14 @@ uint32_t + char[]        string table
 Staleness check: `source_file_size` vs actual file size. Mismatched →
 reject and rebuild. Endianness marker rejects cross-arch caches.
 
+Sidecars store the raw `object_id` / `model_id` values from the session
+that wrote them. On load they are rebased onto the current session's ID
+space (`object_id += next_object_id_ - min_id_in_sidecar`, `model_id`
+overwritten with the freshly-assigned handle) before the elements hit
+`element_map_` or the viewport. Without this, two cached models loaded
+back-to-back collide — both start at `object_id=1` and the second model's
+property lookups return the first model's data.
+
 ### GPU Instancing pipeline (the central pillar)
 
 Everything above plugs into a single data-flow, worth documenting on its
@@ -451,6 +459,13 @@ and per-instance level. Short-circuits when the camera is inside the
 AABB so nothing-you're-standing-next-to is ever lost. Pick pass uses
 threshold 0 so sub-pixel objects remain clickable.
 
+Because the pick pass re-runs the cull with its own parameters (no
+contribution cull, no HiZ) and writes into each model's shared
+`visible_ssbo` / indirect buffer, `pickObjectAt()` must invalidate
+`have_cached_cull_` on exit. Otherwise the next `render()` sees an
+unchanged camera, skips the cull, and draws the pick-pass buffers —
+the user sees obviously-wrong shading until they nudge the camera.
+
 Sphere-based (centre = AABB midpoint, radius = half-diagonal,
 r_px = focal_px · radius / distance). Loses a little precision on
 very elongated bounds vs. 8-corner projection, but costs ~5× less per
diff --git a/src/ifcviewer/ViewportWindow.cpp b/src/ifcviewer/ViewportWindow.cpp
index e96b45f909..a97714950a 100644
--- a/src/ifcviewer/ViewportWindow.cpp
+++ b/src/ifcviewer/ViewportWindow.cpp
@@ -1282,6 +1282,13 @@ uint32_t ViewportWindow::pickObjectAt(int x, int y) {
 
     renderPickPass();
 
+    // The pick pass overwrote each model's visible_ssbo / indirect_buffer with
+    // pick-specific cull params (no contribution cull, no HiZ).  Invalidate
+    // the cached cull so the next render() rebuilds them with main-render
+    // params; otherwise the viewport draws with stale pick-pass buffers and
+    // shading looks wrong until the camera moves.
+    have_cached_cull_ = false;
+
     int px = x * devicePixelRatio();
     int py = (height() - y) * devicePixelRatio();
     uint32_t pixel = 0;