Hi,
Some problems with the ball and stick engine were brought up recently, I would
like to address what changes were made. I worked hard on getting this engine
just right as it is one of the most basic, most people use the defaults and it
is the best suited to drawing molecules. It is also one of the only statically
linked plugins in Avogadro.
I will look at the differences between the engine in 1.0 branch, and that in
master. Repacing diff with log will show all changes were made by Konstantin
(with one commit from Geoff), in a stream of 10 commits (two merges, 2 reverts,
6 commits). The following command will give you the full diff, assuming you did
a git fetch:
$ git diff origin/1.0…origin/master libavogadro/src/engines/bsdyengine.*
I will leave the diff at the end of this post for you to look at if you are
interested/have time. I think it demonstrates a need for more review, and
topic branches facilitate that. We can reject some topic branches, or rework
them before merging them into master. There are several regressions as I see
them, i.e. changing defaults we have used for many years without clear gains.
So, my list of concerns:
-
Addition of custom color code inside tight loops
- introduced extra branching and string handling
-
Changes to radius function
- was likely to be inlined before
- had inline keyword in the right place,
see Inline functions, C++ FAQ - As most people know, most compiler after ~90s ignore this - could likely
just be removed anyway - Used in tight loop, now has lots of branchs (if/else and a switch)
- Default was changed to covalent (patch to change back is there)
- Not so great for drawing molecules anymore
It is great seeing more configuration options, but they should always be
balanced with efficiency concerns and complexity of the interface (I am not
claiming that this makes the interface too complex). Feature creep can really
hurt mature projects, constantly changing defaults can put users off who get
used to the way things are.
None of this was released, but this is another reason why i would like to
start using topic branches and review changes before they go in. It is OK if
not every topic branch is merged, there is stuff I have done in the past and
then changed my mind about it. This is meant purely as an honest review of the
changes that have gone in.
To really be in the sweet spot we need some code review to stop things like
this being merged, but also some performance testing to ensure new code is not
added that slows things down, and regression testing to ensure that features
that once worked still do. For now code review, and some dashboards. I will
work on improving this situation further.
http://www.kitware.com/InfovisWiki/index.php/Git/Workflow/Topic
The above page documents many of my thoughts, it links to some great
resources. I do not have the time to explain every detail again (I already
authored several wiki articles), in essence you branch from master (or your
stable integration branch of choice), you do your work on the branch, you
request a merge of your topic branch - a post on this list would be great.
Once the branch is merged you can delete it - its history is taken into the
main history. You can always bring back an old topic branch to merge into a
different integration branch, or continue working on. Those details are in the
linked wiki article from CMake’s branchy workflow
http://www.cmake.org/Wiki/Git/Workflow/Topic
I am very busy over this next week, I hope that gives people some time to read
over some of the wiki articles. This workflow is being used very successfully
in quite a few other projects. It is not equivalent to just cherry picking
commits from other masters, that changes the commit objects (as does
rebasing). I have cherry picked onto a topic branch before, although if people
adopt this workflow that would not be possible.
If you are curious, clone the Titan sources and look at the history. It is
using a master and next, it also has dashboards up and running. CMake is now
doing the same too, and has many more dashboards.
Have a great weekend.
Marcus
diff --git a/libavogadro/src/engines/bsdyengine.cpp
b/libavogadro/src/engines/bsdyengine.cpp
index 4ae32de…5268b1c 100644
— a/libavogadro/src/engines/bsdyengine.cpp
+++ b/libavogadro/src/engines/bsdyengine.cpp
@@ -87,7 +87,7 @@ namespace Avogadro
BSDYEngine::BSDYEngine(QObject *parent) : Engine(parent),
m_settingsWidget(0), m_atomRadiusPercentage(0.3), m_bondRadius(0.1),
-
m_showMulti(2), m_alpha(1.)
-
m_atomRadiusType(0), m_showMulti(2), m_alpha(1.){ }
Engine *BSDYEngine::clone() const
@@ -97,6 +97,7 @@ namespace Avogadro
engine->m_atomRadiusPercentage = m_atomRadiusPercentage;
engine->m_bondRadius = m_bondRadius;
engine->m_showMulti = m_showMulti; -
engine->m_atomRadiusType = m_atomRadiusType;
engine->m_alpha = m_alpha;
engine->setEnabled(isEnabled());
@@ -142,11 +143,17 @@ namespace Avogadro
if (m_showMulti) order = b->order();
map->setFromPrimitive(atom1);
-
pd->painter()->setColor( map );
-
if (atom1->customColorName().isEmpty()) -
pd->painter()->setColor( map ); -
else -
pd->painter()->setColor(atom1->customColorName()); pd->painter()->drawMultiCylinder( v1, v3, m_bondRadius, order, shift ); map->setFromPrimitive(atom2);
-
pd->painter()->setColor( map );
-
if (atom2->customColorName().isEmpty()) -
pd->painter()->setColor( map ); -
else -
}pd->painter()->setColor(atom2->customColorName()); pd->painter()->drawMultiCylinder( v3, v2, m_bondRadius, order, shift );
@@ -156,7 +163,10 @@ namespace Avogadro
// Render the atoms
foreach(const Atom *a, atoms()) {
map->setFromPrimitive(a);
-
pd->painter()->setColor(map);
-
if (a->customColorName().isEmpty()) -
pd->painter()->setColor( map ); -
else -
}pd->painter()->setColor(a->customColorName()); pd->painter()->drawSphere(a->pos(), radius(a));
@@ -324,11 +334,21 @@ namespace Avogadro
return true;
}
- inline double BSDYEngine::radius(const Atom *atom) const
- double BSDYEngine::radius(const Atom *atom) const
{
- if (atom->atomicNumber())
-
return OpenBabel::etab.GetVdwRad(atom->atomicNumber()) *
m_atomRadiusPercentage;
- return m_atomRadiusPercentage;
- if (atom->customRadius())
-
return atom->customRadius()* m_atomRadiusPercentage; - else {
-
if (atom->atomicNumber()) { -
switch (m_atomRadiusType) { -
case 0: -
return OpenBabel::etab.GetCovalentRad(atom->atomicNumber()) *
m_atomRadiusPercentage;
-
case 1: -
return OpenBabel::etab.GetVdwRad(atom->atomicNumber()) *
m_atomRadiusPercentage;
-
} -
} -
return m_atomRadiusPercentage; - }
}
void BSDYEngine::setAtomRadiusPercentage( int percent )
@@ -337,6 +357,12 @@ namespace Avogadro
emit changed();
}
- void BSDYEngine::setAtomRadiusType(int type)
- {
- m_atomRadiusType = type;
- emit changed();
- }
- void BSDYEngine::setBondRadius( int value )
{
m_bondRadius = value * 0.05;
@@ -394,6 +420,8 @@ namespace Avogadro
m_settingsWidget = new BSDYSettingsWidget();
connect(m_settingsWidget->atomRadiusSlider, SIGNAL(valueChanged(int)),
this, SLOT(setAtomRadiusPercentage(int))); -
connect(m_settingsWidget->combo_radius,
SIGNAL(currentIndexChanged(int)),
-
this, SLOT(setAtomRadiusType(int))); connect(m_settingsWidget->bondRadiusSlider, SIGNAL(valueChanged(int)), this, SLOT(setBondRadius(int))); connect(m_settingsWidget->showMulti, SIGNAL(stateChanged(int)),
@@ -406,6 +434,7 @@ namespace Avogadro
m_settingsWidget->bondRadiusSlider->setValue(int(20*m_bondRadius));
m_settingsWidget->showMulti-
setCheckState((Qt::CheckState)m_showMulti);
m_settingsWidget->opacitySlider->setValue(int(20*m_alpha));
-
}m_settingsWidget->combo_radius->setCurrentIndex(m_atomRadiusType);
return m_settingsWidget;
}
@@ -420,6 +449,7 @@ namespace Avogadro
{
Engine::writeSettings(settings);
settings.setValue(“atomRadius”, 50*m_atomRadiusPercentage); - settings.setValue(“radiusType”, m_atomRadiusType);
settings.setValue(“bondRadius”, 20m_bondRadius);
settings.setValue(“showMulti”, m_showMulti);
settings.setValue(“opacity”, 20m_alpha);
@@ -428,13 +458,15 @@ namespace Avogadro
void BSDYEngine::readSettings(QSettings &settings)
{
Engine::readSettings(settings);
- setAtomRadiusPercentage(settings.value(“atomRadius”, 3).toInt());
-
setAtomRadiusPercentage(settings.value(“atomRadius”, 25).toInt());
setBondRadius(settings.value(“bondRadius”, 2).toInt());
setShowMulti(settings.value(“showMulti”, 2).toInt());
setOpacity(settings.value(“opacity”, 100).toInt()); -
setAtomRadiusType(settings.value(“radiusType”, 1).toInt());
if (m_settingsWidget) {
m_settingsWidget->atomRadiusSlider-
setValue(int(50*m_atomRadiusPercentage));
-
m_settingsWidget->combo_radius->setCurrentIndex(m_atomRadiusType); m_settingsWidget->bondRadiusSlider->setValue(int(20*m_bondRadius)); m_settingsWidget->showMulti-
setCheckState((Qt::CheckState)m_showMulti);
m_settingsWidget->opacitySlider->setValue(int(20*m_alpha));
diff --git a/libavogadro/src/engines/bsdyengine.h
b/libavogadro/src/engines/bsdyengine.h
index fbfdd78…fc30a08 100644
— a/libavogadro/src/engines/bsdyengine.h
+++ b/libavogadro/src/engines/bsdyengine.h
@@ -80,12 +80,13 @@ namespace Avogadro {
private:
-
double radius(const Atom *atom) const;
-
inline double radius(const Atom *atom) const; BSDYSettingsWidget *m_settingsWidget; double m_atomRadiusPercentage; double m_bondRadius; -
int m_atomRadiusType; int m_showMulti; double m_alpha; // transparency of the balls & sticks
@@ -97,7 +98,10 @@ namespace Avogadro {
* @param percent percentage of the VdwRad
*/
void setAtomRadiusPercentage(int percent);
-
/** -
* @param value determines if covalent or VdW radii are used -
*/ -
void setAtomRadiusType(int value); /** * @param value radius of the bonds * 10 */
