From 63eaa1266d17bb4ff01c42869db8a3fc577a8993 Mon Sep 17 00:00:00 2001 From: Magne Sjaastad Date: Mon, 5 Oct 2026 18:40:45 +0200 Subject: [PATCH] Address review feedback on crash-fix batch - Camera::fitView/computeFitViewEyePosition: derive one corrected, non-parallel up vector and use it consistently for right, upNorm, planeTop and the final setFromLookAt(), instead of only repairing the right vector. Previously the degenerate (parallel dir/up) case still produced a singular view matrix because up itself was never corrected. Added a regression test exercising fitView() end-to-end (not just computeFitViewEyePosition()) that checks the resulting direction/up/right form a valid orthonormal basis. - RimViewController::updateCameraLink/updateTimeStepLink: guard against ownerViewLinker()->masterView() being null (RimViewLinker::setMasterView(nullptr) is a supported state), not only against a null view linker. - RicHistogramPlotTools: use RiaGuiApplication::isRunning() before calling RiaGuiApplication::instance(), since instance() itself asserts when there is no running GUI application. - RimDockWindowController::updateViewerWidget: create the view widget before the dock widget, so a failed view-widget creation does not leave a dock widget allocated without a dock manager (which deleteDockWidget() cannot clean up). - RimStimPlanModelCalculator::calculateFormation: avoid continue via a small lambda, and reject double values outside the range of int before the static_cast to avoid undefined behavior. --- .../Commands/RicHistogramPlotTools.cpp | 10 +++- .../RimDockWindowController.cpp | 13 ++++- .../ProjectDataModel/RimViewController.cpp | 12 +++-- .../RimStimPlanModelCalculator.cpp | 26 ++++++---- .../UnitTests/CvfCameraCrashTriage-Test.cpp | 42 +++++++++++++++ Fwk/VizFwk/LibRender/cvfCamera.cpp | 51 ++++++++++++++----- 6 files changed, 123 insertions(+), 31 deletions(-) diff --git a/ApplicationLibCode/Commands/RicHistogramPlotTools.cpp b/ApplicationLibCode/Commands/RicHistogramPlotTools.cpp index bdacced807..68757fbb7f 100644 --- a/ApplicationLibCode/Commands/RicHistogramPlotTools.cpp +++ b/ApplicationLibCode/Commands/RicHistogramPlotTools.cpp @@ -95,8 +95,11 @@ void RicHistogramPlotTools::createHistogramCurve( RimHistogramPlot* plot, RimHis { if ( !plot || !dataSource ) return; + // RiaGuiApplication::instance() asserts internally if called when there is no running GUI + // application, so check isRunning() first instead of relying on a null check of instance(). + if ( !RiaGuiApplication::isRunning() ) return; + RiaGuiApplication* app = RiaGuiApplication::instance(); - if ( !app ) return; RimProject* project = app->project(); if ( !project ) return; @@ -212,8 +215,11 @@ void RicHistogramPlotTools::addHistogramCurveToPlot( RimHistogramPlot* plot, Rim { if ( !plot || !curve ) return; + // RiaGuiApplication::instance() asserts internally if called when there is no running GUI + // application, so check isRunning() first instead of relying on a null check of instance(). + if ( !RiaGuiApplication::isRunning() ) return; + RiaGuiApplication* app = RiaGuiApplication::instance(); - if ( !app ) return; RimProject* project = app->project(); if ( project == nullptr ) return; diff --git a/ApplicationLibCode/ProjectDataModel/RimDockWindowController.cpp b/ApplicationLibCode/ProjectDataModel/RimDockWindowController.cpp index b1c5299599..96d65030ed 100644 --- a/ApplicationLibCode/ProjectDataModel/RimDockWindowController.cpp +++ b/ApplicationLibCode/ProjectDataModel/RimDockWindowController.cpp @@ -132,9 +132,18 @@ void RimDockWindowController::updateViewerWidget() { if ( !viewWidget() ) { + // Create the view widget first: if it fails, nothing has been allocated yet. Creating the + // dock widget first and having view widget creation fail afterwards would otherwise leave a + // dock widget without a dock manager, which deleteDockWidget() cannot clean up. + QWidget* viewWidget = viewPdmObject()->createViewWidget(); + if ( !viewWidget ) return; + ads::CDockWidget* dockWidget = viewPdmObject()->createDockWidget(); - QWidget* viewWidget = viewPdmObject()->createViewWidget(); - if ( !dockWidget || !viewWidget ) return; + if ( !dockWidget ) + { + viewPdmObject()->deleteViewWidget(); + return; + } dockWidget->setWidget( viewWidget ); dockWidget->setObjectName( viewPdmObject()->dockWindowName() ); diff --git a/ApplicationLibCode/ProjectDataModel/RimViewController.cpp b/ApplicationLibCode/ProjectDataModel/RimViewController.cpp index 39332fe5db..21671c81b1 100644 --- a/ApplicationLibCode/ProjectDataModel/RimViewController.cpp +++ b/ApplicationLibCode/ProjectDataModel/RimViewController.cpp @@ -559,8 +559,11 @@ void RimViewController::updateCameraLink() RimViewLinker* viewLinker = ownerViewLinker(); if ( !viewLinker ) return; - viewLinker->updateScaleZ( viewLinker->masterView(), viewLinker->masterView()->scaleZ() ); - viewLinker->updateCamera( viewLinker->masterView() ); + Rim3dView* masterView = viewLinker->masterView(); + if ( !masterView ) return; + + viewLinker->updateScaleZ( masterView, masterView->scaleZ() ); + viewLinker->updateCamera( masterView ); } } @@ -576,7 +579,10 @@ void RimViewController::updateTimeStepLink() RimViewLinker* viewLinker = ownerViewLinker(); if ( !viewLinker ) return; - viewLinker->updateTimeStep( viewLinker->masterView(), viewLinker->masterView()->currentTimeStep() ); + Rim3dView* masterView = viewLinker->masterView(); + if ( !masterView ) return; + + viewLinker->updateTimeStep( masterView, masterView->currentTimeStep() ); } } diff --git a/ApplicationLibCode/ProjectDataModel/StimPlanModel/RimStimPlanModelCalculator.cpp b/ApplicationLibCode/ProjectDataModel/StimPlanModel/RimStimPlanModelCalculator.cpp index f13049729c..9c9f4e7822 100644 --- a/ApplicationLibCode/ProjectDataModel/StimPlanModel/RimStimPlanModelCalculator.cpp +++ b/ApplicationLibCode/ProjectDataModel/StimPlanModel/RimStimPlanModelCalculator.cpp @@ -770,20 +770,24 @@ std::pair, std::vector> RimStimPlanModelCalculator: RimEclipseCase* eclipseCase = m_stimPlanModel->eclipseCaseForProperty( RiaDefines::CurveProperty::FACIES ); std::vector formationNamesVector = RimWellLogTrack::formationNamesVector( eclipseCase ); + auto formationNameForValue = [&formationNamesVector]( double value ) -> QString + { + if ( !RiaCurveDataTools::isValidValue( value, false ) ) return "_"; + + // Reject values outside the range of int before casting to avoid undefined behavior. + if ( value < static_cast( std::numeric_limits::min() ) || value > static_cast( std::numeric_limits::max() ) ) + return "_"; + + int idx = static_cast( value ); + if ( idx >= 0 && idx < static_cast( formationNamesVector.size() ) ) return formationNamesVector[idx]; + + return "_"; + }; + std::vector formationNames; for ( auto value : values ) { - if ( !RiaCurveDataTools::isValidValue( value, false ) ) - { - formationNames.push_back( "_" ); - continue; - } - - int idx = static_cast( value ); - if ( idx >= 0 && idx < static_cast( formationNamesVector.size() ) ) - formationNames.push_back( formationNamesVector[idx] ); - else - formationNames.push_back( "_" ); + formationNames.push_back( formationNameForValue( value ) ); } return std::make_pair( values, formationNames ); diff --git a/ApplicationLibCode/UnitTests/CvfCameraCrashTriage-Test.cpp b/ApplicationLibCode/UnitTests/CvfCameraCrashTriage-Test.cpp index 18bb49af44..398f00ced7 100644 --- a/ApplicationLibCode/UnitTests/CvfCameraCrashTriage-Test.cpp +++ b/ApplicationLibCode/UnitTests/CvfCameraCrashTriage-Test.cpp @@ -27,3 +27,45 @@ TEST( CvfCameraCrashTriageTest, FitViewWithParallelDirAndUpDoesNotCrash ) EXPECT_FALSE( eye.isUndefined() ); } + +//-------------------------------------------------------------------------------------------------- +/// fitView() must also produce a valid, non-singular camera orientation when dir and up are +/// parallel: it uses the (uncorrected) up vector both for setFromLookAt() and internally in +/// computeFitViewEyePosition()/createLookAtMatrix(), so the whole up vector derivation must be +/// consistent, not only the right vector used to compute the eye position. +//-------------------------------------------------------------------------------------------------- +TEST( CvfCameraCrashTriageTest, FitViewWithParallelDirAndUpProducesValidViewMatrix ) +{ + Camera c; + c.setViewport( 0, 0, 100, 100 ); + + BoundingBox bb; + bb.add( Vec3d( 0, 0, 0 ) ); + bb.add( Vec3d( 1, 1, 1 ) ); + + Vec3d inViewDir( 0, 0, 1 ); + Vec3d inUp( 0, 0, 1 ); + double coverageFactor = 0.9; + + c.fitView( bb, inViewDir, inUp, coverageFactor ); + + Vec3d eye; + Vec3d vrp; + Vec3d up; + c.toLookAt( &eye, &vrp, &up ); + + EXPECT_FALSE( eye.isUndefined() ); + EXPECT_FALSE( c.direction().isUndefined() ); + EXPECT_FALSE( c.up().isUndefined() ); + EXPECT_FALSE( c.right().isUndefined() ); + + // direction, up and right should form a valid, mutually orthogonal basis (within floating point + // tolerance). If the up vector correction is not applied consistently, the derived up/right + // vectors from the (singular) view matrix will be degenerate (zero length or not orthogonal). + EXPECT_NEAR( c.direction().length(), 1.0, 1e-6 ); + EXPECT_NEAR( c.up().length(), 1.0, 1e-6 ); + EXPECT_NEAR( c.right().length(), 1.0, 1e-6 ); + EXPECT_NEAR( c.direction() * c.up(), 0.0, 1e-6 ); + EXPECT_NEAR( c.direction() * c.right(), 0.0, 1e-6 ); + EXPECT_NEAR( c.up() * c.right(), 0.0, 1e-6 ); +} diff --git a/Fwk/VizFwk/LibRender/cvfCamera.cpp b/Fwk/VizFwk/LibRender/cvfCamera.cpp index c9b953cae5..698a571b6f 100644 --- a/Fwk/VizFwk/LibRender/cvfCamera.cpp +++ b/Fwk/VizFwk/LibRender/cvfCamera.cpp @@ -45,7 +45,31 @@ namespace cvf { +namespace { +//-------------------------------------------------------------------------------------------------- +/// Returns an up vector guaranteed to be non-parallel to dir. If the passed up vector is already +/// non-parallel to dir, it is returned unchanged. Otherwise a corrected, orthogonal up vector is +/// derived from an arbitrary fallback axis not parallel to dir. +//-------------------------------------------------------------------------------------------------- +Vec3d nonParallelUpVector(const Vec3d& dir, const Vec3d& up) +{ + cvf::Vec3d right = dir^up; + if (right.normalize()) + { + return up; + } + + const cvf::Vec3d& fallbackAxis = (cvf::Math::abs(dir.x()) < 0.9) ? cvf::Vec3d::X_AXIS : cvf::Vec3d::Y_AXIS; + cvf::Vec3d fallbackRight = dir^fallbackAxis; + fallbackRight.normalize(); + + cvf::Vec3d correctedUp = fallbackRight^dir; + correctedUp.normalize(); + return correctedUp; +} + +} // namespace //================================================================================================== /// @@ -288,11 +312,15 @@ void Camera::setProjectionAsPixelExact2D() //-------------------------------------------------------------------------------------------------- void Camera::fitView(const BoundingBox& boundingBox, const Vec3d& dir, const Vec3d& up, double coverageFactor) { + // dir and up may be parallel (degenerate camera orientation); derive a corrected, non-parallel + // up vector so both the eye computation and the final look-at matrix use a consistent basis. + cvf::Vec3d correctedUp = nonParallelUpVector(dir, up); + // Use old view direction, but look towards model center - Vec3d eye = computeFitViewEyePosition(boundingBox, dir, up, coverageFactor, m_fieldOfViewYDeg, viewport()->aspectRatio()); + Vec3d eye = computeFitViewEyePosition(boundingBox, dir, correctedUp, coverageFactor, m_fieldOfViewYDeg, viewport()->aspectRatio()); // Will update cached values - setFromLookAt(eye, boundingBox.center(), up); + setFromLookAt(eye, boundingBox.center(), correctedUp); } @@ -304,20 +332,17 @@ Vec3d Camera::computeFitViewEyePosition(const BoundingBox& boundingBox, const Ve cvf::Vec3d corners[8]; boundingBox.cornerVertices(corners); - cvf::Vec3d upNorm = up.getNormalized(); - cvf::Vec3d right = dir^up; - if (!right.normalize()) - { - // dir and up are parallel (degenerate camera orientation). Fall back to an arbitrary axis - // not parallel to dir to still produce a valid right vector. - const cvf::Vec3d& fallbackAxis = (cvf::Math::abs(dir.x()) < 0.9) ? cvf::Vec3d::X_AXIS : cvf::Vec3d::Y_AXIS; - right = dir^fallbackAxis; - right.normalize(); - } + // dir and up may be parallel (degenerate camera orientation); derive a corrected, non-parallel + // up vector so upNorm, planeTop and right are all consistent with each other. + cvf::Vec3d correctedUp = nonParallelUpVector(dir, up); + + cvf::Vec3d upNorm = correctedUp.getNormalized(); + cvf::Vec3d right = dir^correctedUp; + right.normalize(); cvf::Vec3d boxEyeNorm = (-dir).getNormalized(); cvf::Plane planeTop; - planeTop.setFromPointAndNormal(boundingBox.center(), up); + planeTop.setFromPointAndNormal(boundingBox.center(), correctedUp); cvf::Plane planeSide; planeSide.setFromPointAndNormal(boundingBox.center(), right);