From a5bfe59a4d3dfd86bbe41bf425f6f767422392fb Mon Sep 17 00:00:00 2001 From: Geoff Hutchison Date: Fri, 6 Dec 2024 23:20:49 -0500 Subject: [PATCH] Fix crashing with dipole moment (#1847) * Don't bother calculating dipole moments for empty or 1-atom mols * Update the dipole moment only when the molecule changes * Handle molecule switching with a delay update request * Make sure to update the dipole when the plugins are turned on/off Also fixup some old-style Qt signal-slot behavior Signed-off-by: Geoff Hutchison --------- Signed-off-by: Geoff Hutchison --- avogadro/calc/chargemanager.cpp | 4 ++ avogadro/calc/chargemodel.cpp | 3 ++ avogadro/qtopengl/glwidget.cpp | 47 +++++++++++++++-------- avogadro/qtopengl/glwidget.h | 9 ++++- avogadro/qtplugins/dipole/dipole.cpp | 57 ++++++++++++++++++++++------ avogadro/qtplugins/dipole/dipole.h | 6 +++ 6 files changed, 96 insertions(+), 30 deletions(-) diff --git a/avogadro/calc/chargemanager.cpp b/avogadro/calc/chargemanager.cpp index e4a397b869..21775a98b6 100644 --- a/avogadro/calc/chargemanager.cpp +++ b/avogadro/calc/chargemanager.cpp @@ -164,6 +164,10 @@ Vector3 ChargeManager::dipoleMoment(const std::string& identifier, return Vector3(0.0, 0.0, 0.0); } + if (molecule.atomCount() < 2) { + return Vector3(0.0, 0.0, 0.0); + } + const auto id = m_identifiers[lowerId]; const ChargeModel* model = m_models[id]; return model->dipoleMoment(molecule); diff --git a/avogadro/calc/chargemodel.cpp b/avogadro/calc/chargemodel.cpp index 995aea758c..eedfd68fb9 100644 --- a/avogadro/calc/chargemodel.cpp +++ b/avogadro/calc/chargemodel.cpp @@ -23,6 +23,9 @@ constexpr double M_PI = 3.14159265358979323846; Vector3 ChargeModel::dipoleMoment(const Molecule& mol) const { + if (mol.atomCount() < 2) + return Vector3(0.0, 0.0, 0.0); + // default is to get the set of partial atomic charges // (some models might do something more sophisticated) const MatrixX charges = partialCharges(mol); diff --git a/avogadro/qtopengl/glwidget.cpp b/avogadro/qtopengl/glwidget.cpp index 3dcee7a3cc..9a7b3db549 100644 --- a/avogadro/qtopengl/glwidget.cpp +++ b/avogadro/qtopengl/glwidget.cpp @@ -30,10 +30,10 @@ GLWidget::GLWidget(QWidget* p) m_renderTimer(nullptr) { setFocusPolicy(Qt::ClickFocus); - connect(&m_scenePlugins, - SIGNAL(pluginStateChanged(Avogadro::QtGui::ScenePlugin*)), - SLOT(updateScene())); - connect(&m_scenePlugins, SIGNAL(pluginConfigChanged()), SLOT(updateScene())); + connect(&m_scenePlugins, &QtGui::ScenePluginModel::pluginStateChanged, this, + &GLWidget::updateScene); + connect(&m_scenePlugins, &QtGui::ScenePluginModel::pluginConfigChanged, this, + &GLWidget::updateScene); m_renderer.setTextRenderStrategy(new QtTextRenderStrategy); } @@ -47,7 +47,13 @@ void GLWidget::setMolecule(QtGui::Molecule* mol) m_molecule = mol; foreach (QtGui::ToolPlugin* tool, m_tools) tool->setMolecule(m_molecule); - connect(m_molecule, SIGNAL(changed(unsigned int)), SLOT(updateScene())); + + if (m_molecule != nullptr) { + // update properties like dipole rendering + QTimer::singleShot(500, m_molecule, &QtGui::Molecule::update); + } + + connect(m_molecule, &QtGui::Molecule::changed, this, &GLWidget::updateScene); } QtGui::Molecule* GLWidget::molecule() @@ -60,6 +66,14 @@ const QtGui::Molecule* GLWidget::molecule() const return m_molecule; } +void GLWidget::updateMolecule() +{ + if (m_molecule != nullptr) { + // update properties like dipole rendering + QTimer::singleShot(500, m_molecule, &QtGui::Molecule::update); + } +} + void GLWidget::updateScene() { // Build up the scene with the scene plugins, creating the appropriate nodes. @@ -124,7 +138,8 @@ void GLWidget::addTool(QtGui::ToolPlugin* tool) if (m_tools.contains(tool)) return; - connect(tool, SIGNAL(updateRequested()), SLOT(requestUpdate())); + connect(tool, &QtGui::ToolPlugin::updateRequested, this, + &GLWidget::requestUpdate); tool->setParent(this); tool->setGLWidget(this); tool->setActiveWidget(this); @@ -151,8 +166,8 @@ void GLWidget::setActiveTool(QtGui::ToolPlugin* tool) return; if (m_activeTool && m_activeTool != m_defaultTool) { - disconnect(m_activeTool, SIGNAL(drawablesChanged()), this, - SLOT(updateScene())); + disconnect(m_activeTool, &QtGui::ToolPlugin::drawablesChanged, this, + &GLWidget::updateScene); } if (tool) @@ -160,8 +175,8 @@ void GLWidget::setActiveTool(QtGui::ToolPlugin* tool) m_activeTool = tool; if (m_activeTool && m_activeTool != m_defaultTool) { - connect(m_activeTool, SIGNAL(drawablesChanged()), this, - SLOT(updateScene())); + connect(m_activeTool, &QtGui::ToolPlugin::drawablesChanged, this, + &GLWidget::updateScene); } } @@ -184,8 +199,8 @@ void GLWidget::setDefaultTool(QtGui::ToolPlugin* tool) return; if (m_defaultTool && m_activeTool != m_defaultTool) { - disconnect(m_defaultTool, SIGNAL(drawablesChanged()), this, - SLOT(updateScene())); + disconnect(m_defaultTool, &QtGui::ToolPlugin::drawablesChanged, this, + &GLWidget::updateScene); } if (tool) @@ -193,8 +208,8 @@ void GLWidget::setDefaultTool(QtGui::ToolPlugin* tool) m_defaultTool = tool; if (m_defaultTool && m_activeTool != m_defaultTool) { - connect(m_defaultTool, SIGNAL(drawablesChanged()), this, - SLOT(updateScene())); + connect(m_defaultTool, &QtGui::ToolPlugin::drawablesChanged, this, + &GLWidget::updateScene); } } @@ -202,8 +217,8 @@ void GLWidget::requestUpdate() { if (!m_renderTimer) { m_renderTimer = new QTimer(this); - connect(m_renderTimer, SIGNAL(timeout()), SLOT(updateTimeout())); - m_renderTimer->setSingleShot(1000 / 30); + connect(m_renderTimer, &QTimer::timeout, this, &GLWidget::updateTimeout); + m_renderTimer->setSingleShot(1000 / 30); // 30 fps m_renderTimer->start(); } } diff --git a/avogadro/qtopengl/glwidget.h b/avogadro/qtopengl/glwidget.h index c1bb8cf696..17e69bd177 100644 --- a/avogadro/qtopengl/glwidget.h +++ b/avogadro/qtopengl/glwidget.h @@ -116,6 +116,11 @@ public slots: */ void updateScene(); + /** + * Request update of molecule properties (e.g., dipole moment) + */ + void updateMolecule(); + /** * Clear the contents of the scene. */ @@ -201,7 +206,7 @@ protected slots: QTimer* m_renderTimer; }; -} // End QtOpenGL namespace -} // End Avogadro namespace +} // namespace QtOpenGL +} // namespace Avogadro #endif // AVOGADRO_QTOPENGL_GLWIDGET_H diff --git a/avogadro/qtplugins/dipole/dipole.cpp b/avogadro/qtplugins/dipole/dipole.cpp index 7fb8014f94..515a85c955 100644 --- a/avogadro/qtplugins/dipole/dipole.cpp +++ b/avogadro/qtplugins/dipole/dipole.cpp @@ -34,6 +34,26 @@ Dipole::~Dipole() {} void Dipole::process(const QtGui::Molecule& molecule, Rendering::GroupNode& node) { + // check if the molecule is empty + // (single atoms don't have a dipole moment) + if (molecule.atomCount() < 0) + return; + + // check if the molecule has the dipole set + if (!m_customDipole) { + if (molecule.hasData("dipoleMoment")) { + m_dipoleVector = molecule.data("dipoleMoment").toVector3(); + } else { + // connect to molecule changes + connect(&molecule, &QtGui::Molecule::update, this, &Dipole::updateDipole); + connect(&molecule, SIGNAL(changed(unsigned int)), SLOT(updateDipole())); + } + } else { + // custom dipole moment set + m_dipoleVector = m_customDipoleVector; + } + + // okay if we have all that, set up the arrow auto* geometry = new GeometryNode; node.addChild(geometry); @@ -44,20 +64,33 @@ void Dipole::process(const QtGui::Molecule& molecule, geometry->addDrawable(arrow); Vector3f origin = Vector3f::Zero(); + arrow->addSingleArrow(m_dipoleVector.cast(), origin); +} - // check if the molecule has the dipole set - if (!m_customDipole) { - if (molecule.hasData("dipoleMoment")) { - m_dipoleVector = molecule.data("dipoleMoment").toVector3(); - } else { - if (!molecule.isInteractive()) { - m_dipoleVector = - Calc::ChargeManager::instance().dipoleMoment(m_type, molecule); - } - } - } +void Dipole::updateFinished() +{ + m_updateNeeded = true; + emit drawablesChanged(); +} - arrow->addSingleArrow(m_dipoleVector.cast(), origin); +void Dipole::updateDipole() +{ + QtGui::Molecule* molecule = qobject_cast(sender()); + if (molecule == nullptr || molecule->isInteractive()) + return; + + // if the molecule has a dipole moment set, use it + if (molecule->hasData("dipoleMoment")) + return; + + // otherwise, calculate it + if (m_updateNeeded) { + m_updateNeeded = false; + m_dipoleVector = + Calc::ChargeManager::instance().dipoleMoment(m_type, *molecule); + // single-shot + QTimer::singleShot(0, this, SLOT(updateFinished())); + } } QWidget* Dipole::setupWidget() diff --git a/avogadro/qtplugins/dipole/dipole.h b/avogadro/qtplugins/dipole/dipole.h index 3ae56c4620..be45f18cc6 100644 --- a/avogadro/qtplugins/dipole/dipole.h +++ b/avogadro/qtplugins/dipole/dipole.h @@ -41,12 +41,18 @@ class Dipole : public QtGui::ScenePlugin QWidget* setupWidget() override; bool hasSetupWidget() const override { return false; } +public slots: + void updateDipole(); + void updateFinished(); + private: std::string m_name = "Dipole Moment"; std::string m_type = "MMFF94"; std::vector m_types; Vector3 m_dipoleVector; + Vector3 m_customDipoleVector; bool m_customDipole = false; // Custom dipole moment set + bool m_updateNeeded = true; }; } // end namespace QtPlugins