From aacaa48e5fcd6d7b74c2595be899853257bcd31f Mon Sep 17 00:00:00 2001 From: Geoff Hutchison Date: Thu, 21 Nov 2024 11:51:36 -0500 Subject: [PATCH 1/5] Initial work to diagnose crashes with dipole moment Signed-off-by: Geoff Hutchison --- avogadro/qtplugins/dipole/dipole.cpp | 10 +++++++++- avogadro/qtplugins/dipole/dipole.h | 4 ++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/avogadro/qtplugins/dipole/dipole.cpp b/avogadro/qtplugins/dipole/dipole.cpp index 7fb8014f94..5d39ff9250 100644 --- a/avogadro/qtplugins/dipole/dipole.cpp +++ b/avogadro/qtplugins/dipole/dipole.cpp @@ -50,7 +50,10 @@ void Dipole::process(const QtGui::Molecule& molecule, if (molecule.hasData("dipoleMoment")) { m_dipoleVector = molecule.data("dipoleMoment").toVector3(); } else { - if (!molecule.isInteractive()) { + if (!molecule.isInteractive() && m_updateNeeded) { + m_updateNeeded = false; + // 500ms delay to allow the molecule to be updated + QTimer::singleShot(500, this, &Dipole::updateFinished); m_dipoleVector = Calc::ChargeManager::instance().dipoleMoment(m_type, molecule); } @@ -60,6 +63,11 @@ void Dipole::process(const QtGui::Molecule& molecule, arrow->addSingleArrow(m_dipoleVector.cast(), origin); } +void Dipole::updateFinished() +{ + m_updateNeeded = true; +} + QWidget* Dipole::setupWidget() { return nullptr; diff --git a/avogadro/qtplugins/dipole/dipole.h b/avogadro/qtplugins/dipole/dipole.h index 3ae56c4620..ba79f9db4a 100644 --- a/avogadro/qtplugins/dipole/dipole.h +++ b/avogadro/qtplugins/dipole/dipole.h @@ -41,12 +41,16 @@ class Dipole : public QtGui::ScenePlugin QWidget* setupWidget() override; bool hasSetupWidget() const override { return false; } +public slots: + void updateFinished(); + private: std::string m_name = "Dipole Moment"; std::string m_type = "MMFF94"; std::vector m_types; Vector3 m_dipoleVector; bool m_customDipole = false; // Custom dipole moment set + bool m_updateNeeded = true; }; } // end namespace QtPlugins From 96abbe86663b032f7ebbcc01439aa48557de8386 Mon Sep 17 00:00:00 2001 From: Geoff Hutchison Date: Fri, 6 Dec 2024 16:02:53 -0500 Subject: [PATCH 2/5] Don't bother calculating dipole moments for empty or 1-atom mols Signed-off-by: Geoff Hutchison --- avogadro/calc/chargemanager.cpp | 4 ++++ avogadro/calc/chargemodel.cpp | 3 +++ 2 files changed, 7 insertions(+) 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); From 2a1cc30b15373f1192d46b36a2df17cb76160303 Mon Sep 17 00:00:00 2001 From: Geoff Hutchison Date: Fri, 6 Dec 2024 16:59:31 -0500 Subject: [PATCH 3/5] Update the dipole moment only when the molecule changes Not a full solution yet - loading a molecule doesn't recompute Signed-off-by: Geoff Hutchison --- avogadro/qtplugins/dipole/dipole.cpp | 57 ++++++++++++++++++++-------- avogadro/qtplugins/dipole/dipole.h | 2 + 2 files changed, 43 insertions(+), 16 deletions(-) diff --git a/avogadro/qtplugins/dipole/dipole.cpp b/avogadro/qtplugins/dipole/dipole.cpp index 5d39ff9250..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,28 +64,33 @@ void Dipole::process(const QtGui::Molecule& molecule, geometry->addDrawable(arrow); Vector3f origin = Vector3f::Zero(); - - // 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_updateNeeded) { - m_updateNeeded = false; - // 500ms delay to allow the molecule to be updated - QTimer::singleShot(500, this, &Dipole::updateFinished); - m_dipoleVector = - Calc::ChargeManager::instance().dipoleMoment(m_type, molecule); - } - } - } - arrow->addSingleArrow(m_dipoleVector.cast(), origin); } void Dipole::updateFinished() { m_updateNeeded = true; + emit drawablesChanged(); +} + +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 ba79f9db4a..be45f18cc6 100644 --- a/avogadro/qtplugins/dipole/dipole.h +++ b/avogadro/qtplugins/dipole/dipole.h @@ -42,6 +42,7 @@ class Dipole : public QtGui::ScenePlugin bool hasSetupWidget() const override { return false; } public slots: + void updateDipole(); void updateFinished(); private: @@ -49,6 +50,7 @@ public slots: 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; }; From ae8c2ce87ac34fbafbb8bfd9403e32d78dfd9749 Mon Sep 17 00:00:00 2001 From: Geoff Hutchison Date: Fri, 6 Dec 2024 18:09:34 -0500 Subject: [PATCH 4/5] Handle molecule switching with a delay update request Signed-off-by: Geoff Hutchison --- avogadro/qtopengl/glwidget.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/avogadro/qtopengl/glwidget.cpp b/avogadro/qtopengl/glwidget.cpp index 3dcee7a3cc..b76fb3edb1 100644 --- a/avogadro/qtopengl/glwidget.cpp +++ b/avogadro/qtopengl/glwidget.cpp @@ -47,6 +47,12 @@ void GLWidget::setMolecule(QtGui::Molecule* mol) m_molecule = mol; foreach (QtGui::ToolPlugin* tool, m_tools) tool->setMolecule(m_molecule); + + if (m_molecule) { + // update properties like dipole rendering + QTimer::singleShot(500, m_molecule, &QtGui::Molecule::update); + } + connect(m_molecule, SIGNAL(changed(unsigned int)), SLOT(updateScene())); } From 5a66f83b1baa6d1a4baec11a224620b13ac81783 Mon Sep 17 00:00:00 2001 From: Geoff Hutchison Date: Fri, 6 Dec 2024 21:51:58 -0500 Subject: [PATCH 5/5] 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 --- avogadro/qtopengl/glwidget.cpp | 43 ++++++++++++++++++++-------------- avogadro/qtopengl/glwidget.h | 9 +++++-- 2 files changed, 33 insertions(+), 19 deletions(-) diff --git a/avogadro/qtopengl/glwidget.cpp b/avogadro/qtopengl/glwidget.cpp index b76fb3edb1..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); } @@ -48,12 +48,12 @@ void GLWidget::setMolecule(QtGui::Molecule* mol) foreach (QtGui::ToolPlugin* tool, m_tools) tool->setMolecule(m_molecule); - if (m_molecule) { + if (m_molecule != nullptr) { // update properties like dipole rendering QTimer::singleShot(500, m_molecule, &QtGui::Molecule::update); } - connect(m_molecule, SIGNAL(changed(unsigned int)), SLOT(updateScene())); + connect(m_molecule, &QtGui::Molecule::changed, this, &GLWidget::updateScene); } QtGui::Molecule* GLWidget::molecule() @@ -66,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. @@ -130,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); @@ -157,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) @@ -166,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); } } @@ -190,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) @@ -199,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); } } @@ -208,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