From cc54237f51a67f45f41b3e56e0740c450c6f5b69 Mon Sep 17 00:00:00 2001 From: Dion Moult Date: Wed, 3 Jun 2026 11:27:35 +1000 Subject: [PATCH] viewport: move first-model false-origin guess out of refresh() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Loading a model whose first placement sits at the world origin stack-overflowed BonsaiViewer instantly on Windows (and macOS). WinDbg trace was a 5-frame Qt signal-slot cycle hitting the guard page ~1400 levels deep; Linux escaped only because that machine's iterator order put a non-origin instance first, which made the guess return a non-default value and naturally terminated the recursion after one step. Root cause is the architecture, not the specific guard inside the guess function. `ViewportView::refresh()` was connected to six SessionState signals (projectReset, projectOpened, modelsChanged, federationChanged, visibilityChanged, modelGeometryReady) and was calling `maybeGuessFederatedFalseOrigin` on every model on every fire. That helper called `session_state_->notifyFederationChanged()` unconditionally after the mutation, which re-emitted SessionState::federationChanged, which re-entered refresh(), which re-entered the guess — a hidden emit-in-slot loop. The "current == defaults" guard at the top of the guess prevented further mutations once the value moved off defaults, but on machines where the guess itself returned defaults the guard never fired and the loop ran forever. Cleanup: * refresh() is now terminal: it reads federation state, pushes it to the viewport, and returns. No mutations, no signal emissions. maybeGuessFederatedFalseOrigin is removed from its for-loop. * The guess is renamed to `tryGuessFirstModelFalseOrigin(uint32_t)` and is now invoked only from the modelGeometryReady connection, not from refresh(). Conditions: 1. modelIds().size() == 1 (the just-loaded model is the only model — i.e. this is the "first model added" edge) 2. federation->federatedFalseOrigin() == defaults (nobody has set the origin yet — possibly because the previous attempt guessed defaults and no-op'd, in which case we deliberately want to retry next time a model lands) No one-shot flag: add→remove→add cycles re-attempt the guess precisely while the origin is still default, which is the right semantics. * SessionState now relays Federation::federatedFalseOriginChanged onto its own bus via notifyFederationChanged. This replaces the manual `session_state_->notifyFederationChanged()` call the old guess made post-mutation. With the relay in place, any future mutation site (commands, settings dialog, project load) will propagate to views automatically — the emit point lives at the data change, not at every caller. Views still subscribe to SessionState only; Federation stays a back-end detail. Reproduced with ISSUE_053_20181220Holter_Tower_10.ifcview on Windows (build 21d3945, WinDbg `kn30` showed the recurring cycle explicitly). Diagnosis confirmed on Linux by adding tracing prints in refresh() and the guess body: same model, same code, but the iterator's first-placement happened to be non-origin so the loop terminated after one step. Co-Authored-By: Claude Opus 4.7 --- src/bonsaiviewer/SessionState.cpp | 7 ++++ src/bonsaiviewer/modules/viewport/View.cpp | 40 +++++++++++++++++----- src/bonsaiviewer/modules/viewport/View.h | 2 +- 3 files changed, 39 insertions(+), 10 deletions(-) diff --git a/src/bonsaiviewer/SessionState.cpp b/src/bonsaiviewer/SessionState.cpp index 56699f59af..eb41396ec9 100644 --- a/src/bonsaiviewer/SessionState.cpp +++ b/src/bonsaiviewer/SessionState.cpp @@ -33,6 +33,13 @@ SessionState::SessionState(QObject* parent) , element_registry_(new ElementRegistry(this)) , connector_registry_(new modules::connectors::ConnectorRegistry(this)) { + // Relay specific Federation mutations onto the SessionState bus. Views + // subscribe to SessionState signals only — Federation stays a back-end + // detail. Doing the relay here (instead of having every mutation site + // manually call notifyFederationChanged) keeps the "emit point" at one + // hop from the data change and rules out emit-in-slot recursion bugs. + connect(federation_, &Federation::federatedFalseOriginChanged, + this, &SessionState::notifyFederationChanged); } void SessionState::createLoader(WgpuViewportWindow* viewport) { diff --git a/src/bonsaiviewer/modules/viewport/View.cpp b/src/bonsaiviewer/modules/viewport/View.cpp index f17a880d71..f52474f52f 100644 --- a/src/bonsaiviewer/modules/viewport/View.cpp +++ b/src/bonsaiviewer/modules/viewport/View.cpp @@ -54,9 +54,17 @@ ViewportView::ViewportView(bonsaiviewer::SessionState* session_state, connect(session_state_, &SessionState::projectReset, this, &ViewportView::refresh); connect(session_state_, &SessionState::projectOpened, this, [this](const QString&) { refresh(); }); connect(session_state_, &SessionState::modelsChanged, this, &ViewportView::refresh); - connect(session_state_, &SessionState::federationChanged, this, &ViewportView::refresh); - connect(session_state_, &SessionState::visibilityChanged, this, &ViewportView::refresh); - connect(session_state_, &SessionState::modelGeometryReady, this, [this](uint32_t) { refresh(); }); + connect(session_state_, &SessionState::federationChanged, this, &ViewportView::refresh); + connect(session_state_, &SessionState::visibilityChanged, this, &ViewportView::refresh); + // modelGeometryReady is the right hook for "first model just finished + // loading" — count is already in modelIds() by the time the geometry + // signal lands. We try to auto-guess the false origin here (and only + // here — refresh() stays terminal) so a slot can't accidentally re-emit + // into itself through federationChanged. + connect(session_state_, &SessionState::modelGeometryReady, this, [this](uint32_t mid) { + tryGuessFirstModelFalseOrigin(mid); + refresh(); + }); // Measurement tools — input-driven, share the View's lifetime. connect(viewport_, &WgpuViewportWindow::surfacePickedInTool, this, @@ -123,7 +131,6 @@ void ViewportView::refresh() { for (uint32_t mid : session_state_->modelIds()) { applyCoordinateOperation(mid); applyModelVisibility(mid); - maybeGuessFederatedFalseOrigin(mid); } } @@ -173,24 +180,39 @@ void ViewportView::applyModelVisibility(uint32_t mid) { } } -void ViewportView::maybeGuessFederatedFalseOrigin(uint32_t mid) { +// Auto-guess the federation's false origin when a model finishes loading, +// scoped to the precise case where it's actually wanted: the just-loaded +// model is the *only* model in the session and the false origin hasn't +// been set yet. Re-firable: if the user removes the model and adds a new +// one, and the previous load's guess didn't change the origin from +// defaults (e.g. first-placement happened to land at the world origin — +// see Holter Tower), the next load will retry. +// +// This deliberately lives off the refresh() fan-in. refresh() is connected +// to half a dozen signals; calling a federation mutator from inside it +// stack-overflowed BonsaiViewer once the guess returned defaults, because +// the mutation re-emitted through SessionState → re-entered refresh(). +// Guessing only on the modelGeometryReady edge means a Federation +// mutation here propagates through SessionState's normal relay +// (federatedFalseOriginChanged → notifyFederationChanged) without +// re-entering this function. +void ViewportView::tryGuessFirstModelFalseOrigin(uint32_t mid) { + if (session_state_->modelIds().size() != 1) return; + Federation* federation = session_state_->federation(); - SceneLoader* loader = session_state_->loader(); if (!federation->filePath().isEmpty()) return; const FederatedFalseOrigin& current = federation->federatedFalseOrigin(); const FederatedFalseOrigin defaults; if (current.xyz != defaults.xyz || current.rz_deg != defaults.rz_deg) return; + SceneLoader* loader = session_state_->loader(); const Eigen::Matrix4d* placement = loader->firstPlacement(mid); const ModelGeoref* georef = loader->modelGeoref(mid); if (placement == nullptr || georef == nullptr) return; federation->setFederatedFalseOrigin(guessFederatedFalseOrigin( *placement, *georef, federation->config())); - // The federation mutation above will emit its own signal, but to keep - // views off the Federation bus we re-emit through SessionState. - session_state_->notifyFederationChanged(); } void ViewportView::updateVolumeReadout() { diff --git a/src/bonsaiviewer/modules/viewport/View.h b/src/bonsaiviewer/modules/viewport/View.h index 09b42452d3..67bf71d983 100644 --- a/src/bonsaiviewer/modules/viewport/View.h +++ b/src/bonsaiviewer/modules/viewport/View.h @@ -53,7 +53,7 @@ private: void applyCoordinateOperation(uint32_t mid); void applyModelTransformation(uint32_t mid); void applyModelVisibility(uint32_t mid); - void maybeGuessFederatedFalseOrigin(uint32_t mid); + void tryGuessFirstModelFalseOrigin(uint32_t mid); void updateVolumeReadout(); bonsaiviewer::SessionState* session_state_ = nullptr;