File 876.patch of Package avogadrolibs
From b088d239d51903b9c67395294bb5d7ec07b0d61f Mon Sep 17 00:00:00 2001
From: Geoff Hutchison <geoff.hutchison@gmail.com>
Date: Thu, 28 Apr 2022 16:51:14 -0400
Subject: [PATCH 1/2] WIP to find the memory bug involved in #824
Right now, this reverts #806 but there's still some sort
of memory corruption when assigning one molecule to another
Also, it seems like File -> Import Crystal adds atoms that
don't have layer information
Signed-off-by: Geoff Hutchison <geoff.hutchison@gmail.com>
---
avogadro/core/layermanager.cpp | 7 ++++++
avogadro/core/layermanager.h | 2 +-
avogadro/core/molecule.cpp | 22 +++++--------------
avogadro/qtgui/pluginlayermanager.cpp | 8 +++----
avogadro/qtgui/pluginlayermanager.h | 2 +-
avogadro/qtgui/rwlayermanager.cpp | 3 ++-
avogadro/qtgui/rwmolecule.cpp | 5 +++++
.../insertfragment/insertfragment.cpp | 4 ++++
8 files changed, 30 insertions(+), 23 deletions(-)
diff --git a/avogadro/core/layermanager.cpp b/avogadro/core/layermanager.cpp
index 0fd914b99..148d9f63e 100644
--- a/avogadro/core/layermanager.cpp
+++ b/avogadro/core/layermanager.cpp
@@ -26,6 +26,7 @@ Layer& LayerManager::getMoleculeLayer()
Layer& LayerManager::getMoleculeLayer(const Molecule* mol)
{
+ assert(mol != nullptr);
auto it = m_molToInfo.find(mol);
if (it == m_molToInfo.end()) {
m_molToInfo[mol] = make_shared<MoleculeInfo>(mol);
@@ -41,6 +42,7 @@ shared_ptr<MoleculeInfo> LayerManager::getMoleculeInfo()
shared_ptr<MoleculeInfo> LayerManager::getMoleculeInfo(const Molecule* mol)
{
+ assert(mol != nullptr);
auto it = m_molToInfo.find(mol);
if (it == m_molToInfo.end()) {
m_molToInfo[mol] = make_shared<MoleculeInfo>(mol);
@@ -51,6 +53,9 @@ shared_ptr<MoleculeInfo> LayerManager::getMoleculeInfo(const Molecule* mol)
Layer& LayerManager::getMoleculeLayer(const Molecule* original,
const Molecule* copy)
{
+ assert(original != nullptr);
+ assert(copy != nullptr);
+
auto it = m_molToInfo.find(original);
if (it == m_molToInfo.end()) {
auto molecule = make_shared<MoleculeInfo>(original);
@@ -65,6 +70,8 @@ Layer& LayerManager::getMoleculeLayer(const Molecule* original,
void LayerManager::deleteMolecule(const Molecule* mol)
{
+ assert(mol != nullptr);
+
auto aux = m_molToInfo.find(mol);
if (aux != m_molToInfo.end()) {
auto id = aux->second->mol;
diff --git a/avogadro/core/layermanager.h b/avogadro/core/layermanager.h
index 96f648166..1618f4bda 100644
--- a/avogadro/core/layermanager.h
+++ b/avogadro/core/layermanager.h
@@ -51,7 +51,7 @@ struct LayerData
/**
* @class MoleculeInfo layermanager.h <avogadro/core/layermanager.h>
* @brief All layer dependent data. Original molecule @p mol, is layer hidden
- * @p visible, accepts eddits @p locked, and key-value data like @p enable,
+ * @p visible, accepts edits @p locked, and key-value data like @p enable,
* and custom data @p settings.
*/
struct MoleculeInfo
diff --git a/avogadro/core/molecule.cpp b/avogadro/core/molecule.cpp
index a3b824f55..a52df7adf 100644
--- a/avogadro/core/molecule.cpp
+++ b/avogadro/core/molecule.cpp
@@ -45,12 +45,8 @@ Molecule::Molecule(const Molecule& other)
m_residues(other.m_residues), m_graph(other.m_graph),
m_bondOrders(other.m_bondOrders),
m_atomicNumbers(other.m_atomicNumbers),
- m_layers(LayerManager::getMoleculeLayer(this))
+ m_layers(LayerManager::getMoleculeLayer(&other, this))
{
- // Copy the layers, if they exist
- if (other.m_layers.maxLayer() > 0)
- m_layers = LayerManager::getMoleculeLayer(&other, this);
-
// Copy over any meshes
for (Index i = 0; i < other.meshCount(); ++i) {
Mesh* m = addMesh();
@@ -83,12 +79,8 @@ Molecule::Molecule(Molecule&& other) noexcept
m_bondPairs(std::move(other.m_bondPairs)),
m_bondOrders(std::move(other.m_bondOrders)),
m_atomicNumbers(std::move(other.m_atomicNumbers)),
- m_layers(LayerManager::getMoleculeLayer(this))
+ m_layers(LayerManager::getMoleculeLayer(&other, this))
{
- // Copy the layers, if they exist
- if (other.m_layers.maxLayer() > 0)
- m_layers = LayerManager::getMoleculeLayer(&other, this);
-
m_basisSet = other.m_basisSet;
other.m_basisSet = nullptr;
@@ -138,11 +130,10 @@ Molecule& Molecule::operator=(const Molecule& other)
m_basisSet = other.m_basisSet ? other.m_basisSet->clone() : nullptr;
delete m_unitCell;
m_unitCell = other.m_unitCell ? new UnitCell(*other.m_unitCell) : nullptr;
- }
- // Copy the layers, if they exist
- if (other.m_layers.maxLayer() > 0)
+ // Copy the layers, if they exist
m_layers = LayerManager::getMoleculeLayer(&other, this);
+ }
return *this;
}
@@ -184,8 +175,7 @@ Molecule& Molecule::operator=(Molecule&& other) noexcept
other.m_unitCell = nullptr;
// Copy the layers, if they exist
- if (other.m_layers.maxLayer() > 0)
- m_layers = LayerManager::getMoleculeLayer(&other, this);
+ m_layers = LayerManager::getMoleculeLayer(&other, this);
}
return *this;
@@ -193,7 +183,7 @@ Molecule& Molecule::operator=(Molecule&& other) noexcept
Molecule::~Molecule()
{
- LayerManager::deleteMolecule(this);
+ //LayerManager::deleteMolecule(this);
delete m_basisSet;
delete m_unitCell;
clearMeshes();
diff --git a/avogadro/qtgui/pluginlayermanager.cpp b/avogadro/qtgui/pluginlayermanager.cpp
index 5562984f5..1f236a254 100644
--- a/avogadro/qtgui/pluginlayermanager.cpp
+++ b/avogadro/qtgui/pluginlayermanager.cpp
@@ -40,7 +40,7 @@ PluginLayerManager::~PluginLayerManager()
bool PluginLayerManager::isEnabled() const
{
- if (m_activeMolecule == nullptr ||
+ if (m_activeMolecule == nullptr || m_molToInfo[m_activeMolecule] == nullptr ||
m_molToInfo[m_activeMolecule]->enable.find(m_name) ==
m_molToInfo[m_activeMolecule]->enable.end()) {
return false;
@@ -55,7 +55,7 @@ bool PluginLayerManager::isEnabled() const
bool PluginLayerManager::isActiveLayerEnabled() const
{
- if (m_activeMolecule == nullptr ||
+ if (m_activeMolecule == nullptr || m_molToInfo[m_activeMolecule] == nullptr ||
m_molToInfo[m_activeMolecule]->enable.find(m_name) ==
m_molToInfo[m_activeMolecule]->enable.end()) {
return false;
@@ -70,7 +70,7 @@ bool PluginLayerManager::isActiveLayerEnabled() const
void PluginLayerManager::setEnabled(bool enable)
{
- if (m_activeMolecule == nullptr) {
+ if (m_activeMolecule == nullptr || m_molToInfo[m_activeMolecule] == nullptr) {
return;
}
auto& molecule = m_molToInfo[m_activeMolecule];
@@ -88,7 +88,7 @@ void PluginLayerManager::setEnabled(bool enable)
bool PluginLayerManager::atomEnabled(Index atom) const
{
- if (m_activeMolecule == nullptr ||
+ if (m_activeMolecule == nullptr || m_molToInfo[m_activeMolecule] == nullptr ||
m_molToInfo[m_activeMolecule]->enable.find(m_name) ==
m_molToInfo[m_activeMolecule]->enable.end()) {
return false;
diff --git a/avogadro/qtgui/pluginlayermanager.h b/avogadro/qtgui/pluginlayermanager.h
index 376f9229c..09830fbe4 100644
--- a/avogadro/qtgui/pluginlayermanager.h
+++ b/avogadro/qtgui/pluginlayermanager.h
@@ -18,7 +18,7 @@ namespace QtGui {
* @class PluginLayerManager pluginlayermanager.h
* <avogadro/qtgui/pluginlayermanager.h>
* @brief The PluginLayerManager class is a set of common layer dependent
- * operators usefull for Layer dependent QtPlugins.
+ * operators useful for Layer dependent QtPlugins.
*/
class AVOGADROQTGUI_EXPORT PluginLayerManager : protected Core::LayerManager
{
diff --git a/avogadro/qtgui/rwlayermanager.cpp b/avogadro/qtgui/rwlayermanager.cpp
index 5c0caaa16..c112a1aa5 100644
--- a/avogadro/qtgui/rwlayermanager.cpp
+++ b/avogadro/qtgui/rwlayermanager.cpp
@@ -242,9 +242,10 @@ void RWLayerManager::addMolecule(const Core::Molecule* mol)
Array<std::pair<size_t, string>> RWLayerManager::activeMoleculeNames() const
{
- if (m_activeMolecule == nullptr) {
+ if (m_activeMolecule == nullptr || m_molToInfo[m_activeMolecule] == nullptr) {
return Array<std::pair<size_t, string>>();
}
+
auto& molecule = m_molToInfo[m_activeMolecule];
size_t qttyLayer = molecule->layer.layerCount();
vector<set<string>> active(qttyLayer, set<string>());
diff --git a/avogadro/qtgui/rwmolecule.cpp b/avogadro/qtgui/rwmolecule.cpp
index 73262ca6f..214918dca 100644
--- a/avogadro/qtgui/rwmolecule.cpp
+++ b/avogadro/qtgui/rwmolecule.cpp
@@ -26,6 +26,8 @@
#include <avogadro/core/spacegroups.h>
#include <avogadro/qtgui/hydrogentools.h>
+#include <QtCore/QDebug>
+
namespace Avogadro {
namespace QtGui {
@@ -413,6 +415,9 @@ void RWMolecule::modifyMolecule(const Molecule& newMolecule,
ModifyMoleculeCommand* comm =
new ModifyMoleculeCommand(*this, m_molecule, newMolecule);
+ qDebug() << " checking layers " << m_molecule.layer().maxLayer() << m_molecule.layer(0);
+ qDebug() << " and in newMol " << newMolecule.layer().maxLayer() << newMolecule.layer(0);
+
comm->setText(undoText);
m_undoStack.push(comm);
diff --git a/avogadro/qtplugins/insertfragment/insertfragment.cpp b/avogadro/qtplugins/insertfragment/insertfragment.cpp
index 2129c8f1d..bb943a437 100644
--- a/avogadro/qtplugins/insertfragment/insertfragment.cpp
+++ b/avogadro/qtplugins/insertfragment/insertfragment.cpp
@@ -124,6 +124,10 @@ void InsertFragment::performInsert(const QString& fileName, bool crystal)
if (crystal) {
Molecule::MoleculeChanges changes =
(Molecule::Atoms | Molecule::Bonds | Molecule::Added | Molecule::Removed);
+
+ // check layers in newMol
+ qDebug() << " added " << newMol.layer().maxLayer() << newMol.layer(0);
+
m_molecule->undoMolecule()->modifyMolecule(newMol, changes,
tr("Import Crystal"));
emit requestActiveTool("Navigator");
From 165305f7edcad0e7b838380e41becb28513ed611 Mon Sep 17 00:00:00 2001
From: Geoff Hutchison <geoff.hutchison@gmail.com>
Date: Thu, 28 Apr 2022 19:45:53 -0400
Subject: [PATCH 2/2] Make sure when copy / assign molecules that all atoms are
added to the active layer.
An ideal thing might be to create a new temporary layer
(e.g., a copy layer) but this is a good step
Signed-off-by: Geoff Hutchison <geoff.hutchison@gmail.com>
---
avogadro/core/molecule.cpp | 86 ++++++++++++-------
avogadro/qtgui/rwmolecule.cpp | 5 --
.../insertfragment/insertfragment.cpp | 16 +---
.../qtplugins/insertfragment/insertfragment.h | 13 +--
4 files changed, 59 insertions(+), 61 deletions(-)
diff --git a/avogadro/core/molecule.cpp b/avogadro/core/molecule.cpp
index a52df7adf..cf054bc03 100644
--- a/avogadro/core/molecule.cpp
+++ b/avogadro/core/molecule.cpp
@@ -43,8 +43,7 @@ Molecule::Molecule(const Molecule& other)
m_residues(other.m_residues), m_graph(other.m_graph),
m_graphDirty(other.m_graphDirty), m_bondPairs(other.m_bondPairs),
m_bondOrders(other.m_bondOrders), m_atomicNumbers(other.m_atomicNumbers),
- m_atomicNumbers(other.m_atomicNumbers),
- m_layers(LayerManager::getMoleculeLayer(&other, this))
+ m_layers(LayerManager::getMoleculeLayer(this))
{
// Copy over any meshes
for (Index i = 0; i < other.meshCount(); ++i) {
@@ -58,6 +57,15 @@ Molecule::Molecule(const Molecule& other)
Cube* c = addCube();
*c = *other.cube(i);
}
+
+ // Copy layers, if they exist
+ if (other.m_layers.maxLayer() > 0)
+ m_layers = LayerManager::getMoleculeLayer(&other, this);
+ else {
+ // make sure all the atoms are in the active layer
+ for (Index i = 0; i < atomCount(); ++i)
+ m_layers.addAtomToActiveLayer(i);
+ }
}
Molecule::Molecule(Molecule&& other) noexcept
@@ -79,13 +87,22 @@ Molecule::Molecule(Molecule&& other) noexcept
m_residues(std::move(other.m_residues)), m_graph(std::move(other.m_graph)),
m_bondOrders(std::move(other.m_bondOrders)),
m_atomicNumbers(std::move(other.m_atomicNumbers)),
- m_layers(LayerManager::getMoleculeLayer(&other, this))
+ m_layers(LayerManager::getMoleculeLayer(this))
{
m_basisSet = other.m_basisSet;
other.m_basisSet = nullptr;
m_unitCell = other.m_unitCell;
other.m_unitCell = nullptr;
+
+ // Copy the layers, only if they exist
+ if (other.m_layers.maxLayer() > 0)
+ m_layers = LayerManager::getMoleculeLayer(&other, this);
+ else {
+ // make sure all the atoms are in the active layer
+ for (Index i = 0; i < atomCount(); ++i)
+ m_layers.addAtomToActiveLayer(i);
+ }
}
Molecule& Molecule::operator=(const Molecule& other)
@@ -131,8 +148,14 @@ Molecule& Molecule::operator=(const Molecule& other)
delete m_unitCell;
m_unitCell = other.m_unitCell ? new UnitCell(*other.m_unitCell) : nullptr;
- // Copy the layers, if they exist
- m_layers = LayerManager::getMoleculeLayer(&other, this);
+ // Copy the layers, only if they exist
+ if (other.m_layers.maxLayer() > 0)
+ m_layers = LayerManager::getMoleculeLayer(&other, this);
+ else {
+ // make sure all the atoms are in the active layer
+ for (Index i = 0; i < atomCount(); ++i)
+ m_layers.addAtomToActiveLayer(i);
+ }
}
return *this;
@@ -175,7 +198,13 @@ Molecule& Molecule::operator=(Molecule&& other) noexcept
other.m_unitCell = nullptr;
// Copy the layers, if they exist
- m_layers = LayerManager::getMoleculeLayer(&other, this);
+ if (other.m_layers.maxLayer() > 0)
+ m_layers = LayerManager::getMoleculeLayer(&other, this);
+ else {
+ // make sure all the atoms are in the active layer
+ for (Index i = 0; i < atomCount(); ++i)
+ m_layers.addAtomToActiveLayer(i);
+ }
}
return *this;
@@ -183,7 +212,7 @@ Molecule& Molecule::operator=(Molecule&& other) noexcept
Molecule::~Molecule()
{
- //LayerManager::deleteMolecule(this);
+ // LayerManager::deleteMolecule(this);
delete m_basisSet;
delete m_unitCell;
clearMeshes();
@@ -472,12 +501,12 @@ Molecule::BondType Molecule::bond(Index atomId1, Index atomId2) const
assert(atomId1 < atomCount());
assert(atomId2 < atomCount());
- const std::vector<Index> &edgeIndices = m_graph.edges(atomId1);
+ const std::vector<Index>& edgeIndices = m_graph.edges(atomId1);
for (Index i = 0; i < edgeIndices.size(); i++) {
Index index = edgeIndices[i];
- const std::pair<Index, Index> &pair = m_graph.endpoints(index);
+ const std::pair<Index, Index>& pair = m_graph.endpoints(index);
if (pair.first == atomId2 || pair.second == atomId2)
- return BondType(const_cast<Molecule *>(this), index);
+ return BondType(const_cast<Molecule*>(this), index);
}
return BondType();
}
@@ -490,25 +519,25 @@ Array<Molecule::BondType> Molecule::bonds(const AtomType& a)
return bonds(a.index());
}
-Array<const Molecule::BondType *> Molecule::bonds(Index a) const
+Array<const Molecule::BondType*> Molecule::bonds(Index a) const
{
- Array<const BondType *> atomBonds;
+ Array<const BondType*> atomBonds;
if (a < atomCount()) {
- const std::vector<Index> &edgeIndices = m_graph.edges(a);
+ const std::vector<Index>& edgeIndices = m_graph.edges(a);
for (Index i = 0; i < edgeIndices.size(); ++i) {
Index index = edgeIndices[i];
- if (m_graph.endpoints(index).first == a
- || m_graph.endpoints(index).second == a)
- {
+ if (m_graph.endpoints(index).first == a ||
+ m_graph.endpoints(index).second == a) {
// work around to consult bonds without breaking constantness
- atomBonds.push_back(new BondType(const_cast<Molecule *>(this), index));
+ atomBonds.push_back(new BondType(const_cast<Molecule*>(this), index));
}
}
}
- std::sort(atomBonds.begin(), atomBonds.end(), [](const BondType *&a, const BondType *&b) {
- return a->index() < b->index();
- });
+ std::sort(atomBonds.begin(), atomBonds.end(),
+ [](const BondType*& a, const BondType*& b) {
+ return a->index() < b->index();
+ });
return atomBonds;
}
@@ -516,7 +545,7 @@ Array<Molecule::BondType> Molecule::bonds(Index a)
{
Array<BondType> atomBonds;
if (a < atomCount()) {
- const std::vector<Index> &edgeIndices = m_graph.edges(a);
+ const std::vector<Index>& edgeIndices = m_graph.edges(a);
for (Index i = 0; i < edgeIndices.size(); ++i) {
Index index = edgeIndices[i];
auto bond = bondPair(index);
@@ -525,9 +554,8 @@ Array<Molecule::BondType> Molecule::bonds(Index a)
}
}
- std::sort(atomBonds.begin(), atomBonds.end(), [](BondType &a, BondType &b) {
- return a.index() < b.index();
- });
+ std::sort(atomBonds.begin(), atomBonds.end(),
+ [](BondType& a, BondType& b) { return a.index() < b.index(); });
return atomBonds;
}
@@ -982,8 +1010,8 @@ bool Molecule::removeBonds(Index atom)
if (atom >= atomCount())
return false;
- while(true) {
- const std::vector<size_t> &bondList = m_graph.edges(atom);
+ while (true) {
+ const std::vector<size_t>& bondList = m_graph.edges(atom);
if (!bondList.size())
break;
size_t bond = bondList[0];
@@ -995,7 +1023,7 @@ bool Molecule::removeBonds(Index atom)
Array<std::pair<Index, Index>> Molecule::getAtomBonds(Index index) const
{
Array<std::pair<Index, Index>> result;
- const std::vector<Index> &edgeIndices = m_graph.edges(index);
+ const std::vector<Index>& edgeIndices = m_graph.edges(index);
for (Index i = 0; i < edgeIndices.size(); i++) {
result.push_back(m_graph.endpoints(edgeIndices[i]));
}
@@ -1005,7 +1033,7 @@ Array<std::pair<Index, Index>> Molecule::getAtomBonds(Index index) const
Array<unsigned char> Molecule::getAtomOrders(Index index) const
{
Array<unsigned char> result;
- const std::vector<Index> &edgeIndices = m_graph.edges(index);
+ const std::vector<Index>& edgeIndices = m_graph.edges(index);
for (Index i = 0; i < edgeIndices.size(); i++) {
result.push_back(m_bondOrders[edgeIndices[i]]);
}
diff --git a/avogadro/qtgui/rwmolecule.cpp b/avogadro/qtgui/rwmolecule.cpp
index 214918dca..73262ca6f 100644
--- a/avogadro/qtgui/rwmolecule.cpp
+++ b/avogadro/qtgui/rwmolecule.cpp
@@ -26,8 +26,6 @@
#include <avogadro/core/spacegroups.h>
#include <avogadro/qtgui/hydrogentools.h>
-#include <QtCore/QDebug>
-
namespace Avogadro {
namespace QtGui {
@@ -415,9 +413,6 @@ void RWMolecule::modifyMolecule(const Molecule& newMolecule,
ModifyMoleculeCommand* comm =
new ModifyMoleculeCommand(*this, m_molecule, newMolecule);
- qDebug() << " checking layers " << m_molecule.layer().maxLayer() << m_molecule.layer(0);
- qDebug() << " and in newMol " << newMolecule.layer().maxLayer() << newMolecule.layer(0);
-
comm->setText(undoText);
m_undoStack.push(comm);
diff --git a/avogadro/qtplugins/insertfragment/insertfragment.cpp b/avogadro/qtplugins/insertfragment/insertfragment.cpp
index bb943a437..4123fc76b 100644
--- a/avogadro/qtplugins/insertfragment/insertfragment.cpp
+++ b/avogadro/qtplugins/insertfragment/insertfragment.cpp
@@ -1,17 +1,6 @@
/******************************************************************************
-
This source file is part of the Avogadro project.
-
- Copyright 2020 Geoffrey Hutchison
-
- This source code is released under the New BSD License, (the "License").
-
- Unless required by applicable law or agreed to in writing, software
- distributed under the License is distributed on an "AS IS" BASIS,
- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- See the License for the specific language governing permissions and
- limitations under the License.
-
+ This source code is released under the 3-Clause BSD License, (see "LICENSE").
******************************************************************************/
#include "insertfragment.h"
@@ -125,9 +114,6 @@ void InsertFragment::performInsert(const QString& fileName, bool crystal)
Molecule::MoleculeChanges changes =
(Molecule::Atoms | Molecule::Bonds | Molecule::Added | Molecule::Removed);
- // check layers in newMol
- qDebug() << " added " << newMol.layer().maxLayer() << newMol.layer(0);
-
m_molecule->undoMolecule()->modifyMolecule(newMol, changes,
tr("Import Crystal"));
emit requestActiveTool("Navigator");
diff --git a/avogadro/qtplugins/insertfragment/insertfragment.h b/avogadro/qtplugins/insertfragment/insertfragment.h
index 70ee1a291..39f2b2fda 100644
--- a/avogadro/qtplugins/insertfragment/insertfragment.h
+++ b/avogadro/qtplugins/insertfragment/insertfragment.h
@@ -1,17 +1,6 @@
/******************************************************************************
-
This source file is part of the Avogadro project.
-
- Copyright 2020 Geoffrey R. Hutchison
-
- This source code is released under the New BSD License, (the "License").
-
- Unless required by applicable law or agreed to in writing, software
- distributed under the License is distributed on an "AS IS" BASIS,
- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- See the License for the specific language governing permissions and
- limitations under the License.
-
+ This source code is released under the 3-Clause BSD License, (see "LICENSE").
******************************************************************************/
#ifndef AVOGADRO_QTPLUGINS_INSERTFRAGMENT_H
