mirror of
https://github.com/IfcOpenShell/IfcOpenShell.git
synced 2026-08-09 01:11:40 +00:00
viewport: move first-model false-origin guess out of refresh()
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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) {
|
||||
|
||||
@@ -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() {
|
||||
|
||||
@@ -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;
|
||||
|
||||
Reference in New Issue
Block a user