mirror of
https://github.com/OPM/ResInsight.git
synced 2026-10-24 22:27:55 -05:00
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.
This commit is contained in:
1 parent
5a9d38d53b
commit
63eaa1266d
6 files changed
+123
-31
No files matched your search
@@ -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;
|
||||
|
||||
@@ -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() );
|
||||
|
||||
@@ -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() );
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -770,20 +770,24 @@ std::pair<std::vector<double>, std::vector<QString>> RimStimPlanModelCalculator:
|
||||
RimEclipseCase* eclipseCase = m_stimPlanModel->eclipseCaseForProperty( RiaDefines::CurveProperty::FACIES );
|
||||
std::vector<QString> 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<double>( std::numeric_limits<int>::min() ) || value > static_cast<double>( std::numeric_limits<int>::max() ) )
|
||||
return "_";
|
||||
|
||||
int idx = static_cast<int>( value );
|
||||
if ( idx >= 0 && idx < static_cast<int>( formationNamesVector.size() ) ) return formationNamesVector[idx];
|
||||
|
||||
return "_";
|
||||
};
|
||||
|
||||
std::vector<QString> formationNames;
|
||||
for ( auto value : values )
|
||||
{
|
||||
if ( !RiaCurveDataTools::isValidValue( value, false ) )
|
||||
{
|
||||
formationNames.push_back( "_" );
|
||||
continue;
|
||||
}
|
||||
|
||||
int idx = static_cast<int>( value );
|
||||
if ( idx >= 0 && idx < static_cast<int>( formationNamesVector.size() ) )
|
||||
formationNames.push_back( formationNamesVector[idx] );
|
||||
else
|
||||
formationNames.push_back( "_" );
|
||||
formationNames.push_back( formationNameForValue( value ) );
|
||||
}
|
||||
|
||||
return std::make_pair( values, formationNames );
|
||||
|
||||
@@ -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 );
|
||||
}
|
||||
@@ -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);
|
||||
|
||||
Reference in new issue
Block a user