Changes in the ball and stick engine

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 http://www.parashift.com/c++-faq-lite/inline-functions.html
    • 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”, 20
    m_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
      */

Feel free to revert if you don’t like these changes

  • Addition of custom color code inside tight loops
  • introduced extra branching and string handling

I agree that in current state it’s a crutch. I really don’t like how color map stuff works. For example, this thing occurs often:
Color *map = colorMap();
if (!map) map = pd->colorMap();
So, it’s supposed there are two color maps: global and map of painter. In addition to this stuff, customColor of Atom exist. Maybe it’s worth to change colorMap() to return relevant color for each atom, and use internal caching to prevent condition checking for rendering of each atom?


Regards,
Konstantin

Hi,

On Sat, May 29, 2010 at 5:32 PM, Marcus D. Hanwell marcus@cryos.org wrote:

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 http://www.parashift.com/c++-faq-lite/inline-functions.html
  • 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.

Code review will improve quality. It will also allow us to learn from
each other. I’ll read the articles and see how I can help.

A general remark about the bsdyengine diff below:

  • The if statements could bemoved outside the loop (lots of repeated code…)
  • These branches (if/switch) could be replaced by using polymorphism.
    Static polymorphism (using templates) would also avoids the virtual
    method calls.

Tim

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”, 20
    m_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
     */
    


Avogadro-devel mailing list
Avogadro-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/avogadro-devel

These branches (if/switch) could be replaced by using polymorphism.Static polymorphism (using templates) would also avoids the virtual method calls.

Polymorphism of… what? Do you mean subclassing of engine? or something else?
Actually, choice of these branches depends on settings, changeable at run time.


Regards,
Konstantin

On Saturday 29 May 2010 12:25:09 Tim Vandermeersch wrote:

Hi,

On Sat, May 29, 2010 at 5:32 PM, Marcus D. Hanwell marcus@cryos.org wrote:

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.

> > Code review will improve quality. It will also allow us to learn from > each other. I'll read the articles and see how I can help.

This is exactly what I am getting at. Glad you appreciate the value in the
approach. We can all learn from each other, and improve code quality. I am not
proposing picking to pieces each line in a diff, things like incorrect use of
inline would be picked up on quickly for example.

A general remark about the bsdyengine diff below:

  • The if statements could bemoved outside the loop (lots of repeated
    code…) * These branches (if/switch) could be replaced by using
    polymorphism. Static polymorphism (using templates) would also avoids the
    virtual method calls.

Totally, I was more limiting myself to a discussion of problems introduced. My
first pass would just take the branching outside of the tight loop. A cooler
approach would be an internal template or something :slight_smile:

This approach should help bring new developers into the project without too
many regressions creeping in. I know I can be over protective of Avogadro at
times, but it should be a positive experience for all involved. Especially the
project as a whole.

Marcus

A cooler approach would be an internal template or something

Couldn’t it be used to kill massive code duplication between renderQuick, renderOpaque, and renderTransparent?


Regards,
Konstantin

On Sat, May 29, 2010 at 6:32 PM, Konstantin Tokarev annulen@yandex.ru wrote:

These branches (if/switch) could be replaced by using polymorphism.Static polymorphism (using templates) would also avoids the virtual method calls.

Polymorphism of… what? Do you mean subclassing of engine? or something else?
Actually, choice of these branches depends on settings, changeable at run time.

Class polymorphism:

But it is easier with an example using runtime (dynamic) polymorphism.
The switch (m_aromRadiusType) could be replaced with:

class Radius {
public:
virtual double radius(const Atom *atom) const = 0;
};

class CovRadius {
public:
double radius(const Atom *atom) { … }
}

At runtime, when the option is changed you can do:

if (covRadius)
m_radius = new CovRadius();
else
m_radius = new VdwRadius();

It is just an example, just moving if statements outside a loop is
also good. My point was that plugins are not part of the library and
we are free to use these techniques to improve performance. I don’t
require this code to be changed in that way though.

Tim


Regards,
Konstantin

On Sat, May 29, 2010 at 6:48 PM, Konstantin Tokarev annulen@yandex.ru wrote:

A cooler approach would be an internal template or something

Couldn’t it be used to kill massive code duplication between renderQuick, renderOpaque, and renderTransparent?

Yes for example, in the code below, the compiler will optimize the if
statements away. It actually creates 3 versions of the function with a
single … each. In _render, you can have as many type == n statements
as you want for free.

class MyEngine : public Engine {

public:
void renderQuick() { _render<1>(); }
void renderOpaque() { _render<2>(); }
void renderTransparent() { _render<3>(); }

private:
template void _render() {
if (type == 1) {

} else if (type == 2){

} else if (type == 3) {

}
}
}


Regards,
Konstantin



Avogadro-devel mailing list
Avogadro-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/avogadro-devel

2010/5/29 Marcus D. Hanwell marcus@cryos.org:

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

The inline keyword isn’t ignored by most compilers, but it’s just a
hint. The compiler may inline without the keyword, and may not inline
even with the keyword, but it’s still taken into account by compilers
such as GCC.

For example, with GCC, the inline keyword has the effect of multiply
by 5 the thresholds used by GCC to determine whether to inline. See
man gcc.

Benoit

  • 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”, 20
    m_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
     */
    


Avogadro-devel mailing list
Avogadro-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/avogadro-devel

On Saturday 29 May 2010 13:24:38 Benoit Jacob wrote:

2010/5/29 Marcus D. Hanwell marcus@cryos.org:

So, my list of concerns:

  • Addition of custom color code inside tight loops

    • introduced extra branching and string handling
  • Changes to radius function

The inline keyword isn’t ignored by most compilers, but it’s just a
hint. The compiler may inline without the keyword, and may not inline
even with the keyword, but it’s still taken into account by compilers
such as GCC.

Ah, the stack overflow article I read, and a few other forum posts not totally
correct then.

For example, with GCC, the inline keyword has the effect of multiply
by 5 the thresholds used by GCC to determine whether to inline. See
man gcc.

That is good to know. I will continue using this, sparingly, and move it back
to the correct place in the ball and stick engine. That is, unless we end up
refactoring that code anyway, due to all of the branching introduced inside
the tight loops.

This is another reason why I would like to see more code review. Hopefully we
all learn more, become better developers, and may be even take over the world
some day :wink:

Thanks for the clarification Benoit. I know it couldn’t hurt, but from my
Googling was starting to suspect the keyword didn’t do very much anymore.

Marcus

That is good to know. I will continue using this, sparingly, and move it back
to the correct place in the ball and stick engine. That is, unless we end up
refactoring that code anyway, due to all of the branching introduced inside
the tight loops.

I think we could make a significant optimization to most of the engines by caching the colors (including custom ones). This would allow easy refactoring.

But yes, code review does improve code quality!

Just my $0.02,
-Geoff

On Saturday 29 May 2010 22:12:17 Geoffrey Hutchison wrote:

That is good to know. I will continue using this, sparingly, and move it
back to the correct place in the ball and stick engine. That is, unless
we end up refactoring that code anyway, due to all of the branching
introduced inside the tight loops.

I think we could make a significant optimization to most of the engines by
caching the colors (including custom ones). This would allow easy
refactoring.

But yes, code review does improve code quality!

What I would really like to do is build up a scene graph using the engines
(the painters could be adapted to do this), and effectively cache all of the
rendering in the scene graph. This can then be used most of the time (the
vertex positions, colors, everything would be cached).

It would require some work, but if we get it right then it means the engines
would be called much less often. Also, when rendering, we can assure the
vertex data is in one contiguous block of memory, the color data in another…
Every call to drawSphere is transformed from OpenGL to ‘add a sphere to the
scene graph, color c, radius r…’

In the short term we could upgrade one of two core engines to cache some of
the slowest calls internally. Effectively, building up parts of their own mini
scene. Several of the calls in most engines would be much better cached, as
they are in tight loops using virtual functions.

A peek into a few of my ideas…hoping for some time to implement a few.

Marcus

2010/5/29 Benoit Jacob jacob.benoit.1@gmail.com:

The inline keyword isn’t ignored by most compilers, but it’s just a
hint. The compiler may inline without the keyword, and may not inline
even with the keyword, but it’s still taken into account by compilers
such as GCC.

For example, with GCC, the inline keyword has the effect of multiply
by 5 the thresholds used by GCC to determine whether to inline. See
man gcc.

Hi, after a chat with Marcus it appears that this info is really very
hard to find, so here is the relevant part of “man gcc”:

       max-inline-insns-single
           Several parameters control the tree inliner used in gcc.
           This number sets the maximum number of instructions (counted
           in GCC's internal representation) in a single function that
           the tree inliner will consider for inlining.  This only
           affects functions declared inline and methods implemented in
           a class declaration (C++).  The default value is 300.

       max-inline-insns-auto
           When you use -finline-functions (included in -O3), a lot of
           functions that would otherwise not be considered for
           inlining by the compiler will be investigated.  To those
           functions, a different (more restrictive) limit compared to
           functions declared inline can be applied.  The default value
           is 50.

Thus, using the inline keyword makes the threshold jump from 50 to
300, actually a 6x increase, not 5x as I had said. Though I think it
used to be 5x until recently.

Benoit

Benoit

  • 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”, 20
    m_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
     */
    


Avogadro-devel mailing list
Avogadro-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/avogadro-devel