Ngl: Double and triple bond depiction

[fredludlow]

Would be great to have this! (Related: #133).

Very happy to have a go at this, though would definitely need a few pointers as to how to best do it and perhaps not the easiest first foray into the code... My thoughts so far, (thinking out loud so apologies if nonsense :) ).

• Presumably only want to do this for a few representations (line, licorice and ball+stick mode).
• I'm guessing some kind of offset line or (slimmer) cylinder would be a good way to represent higher order bonds?
• Do the extra lines/cylinders live in the same LineBuffer/CylinderBuffer as the current bonds...
  
  
  
  • This could break the alignment of the LineBuffer with internal bondSet, so might be problematic?
  
  
  • ...or create multiple buffers (main bond, secondary bond, tertiary bond)?
  
  
• Sensible positioning of the extra lines/cylinders requires placing in plane plus (basic) ring detection to ensure they're placed on the inside of rings
• Should calculation of the position of extra lines/cylinders happen in the structure object or representation? (I guess it's a representation property?).
• Would want to maintain option to turn off

[arose]

Presumably only want to do this for a few representations (line, licorice and ball+stick mode).

Sure, eventually. In the beginning it is probably good to just create an extra representation to simplify things. When creating new representations I like to start by just creating BufferRepresentations.

var sphereBuffer = new NGL.SphereBuffer(...);
component.addBufferRepresentation();

This avoids needing to care about all sorts of details when creating a new representation.

I'm guessing some kind of offset line or (slimmer) cylinder would be a good way to represent higher order bonds?

Yes, just smaller cylinders, shifted in a plane.

Do the extra lines/cylinders live in the same LineBuffer/CylinderBuffer as the current bonds...

• This could break the alignment of the LineBuffer with internal bondSet, so might be problematic?
• ...or create multiple buffers (main bond, secondary bond, tertiary bond)?

Create extra buffers

Single bonds
var cylinderBuffer1 = new NGL.CylinderBuffer(...);

Double bonds
var cylinderBuffer1 = new NGL.CylinderBuffer(...);
var cylinderBuffer2 = new NGL.CylinderBuffer(...);

Triple bonds
var cylinderBuffer1 = new NGL.CylinderBuffer(...);
var cylinderBuffer2 = new NGL.CylinderBuffer(...);
var cylinderBuffer3 = new NGL.CylinderBuffer(...);

Quadruple bonds
...

Sensible positioning of the extra lines/cylinders requires placing in plane plus (basic) ring detection to ensure they're placed on the inside of rings

I would handle rings the same, at least at first. I would suggest placing them like in the image below, with parameters to control spacing and thickness (as a ratio). Having the code to get a sensible plane for each higher order bond would already be a huge step forward. I suggest starting with that. I am happy to integrate any such code myself.

Should calculation of the position of extra lines/cylinders happen in the structure object or representation? (I guess it's a representation property?).

In the representation, definitely not in the structure. Some preprocessing on how the plane should calculated (e.g. from which three atoms) could be done in the NGL.ResidueType object so that it can be reused for all residues of that type.

Would want to maintain option to turn off

Sure!

[fredludlow]

Cool, that's helpful thanks.

I've been playing around a bit getting used to looking up bonds/atoms with the fancy store/proxy classes and a couple of quick questions (as always, typing whilst thinking so possibly silly questions):

1. structure.bondStore seems to store bonds twice (e.g. load the mol2 example (adrenaline) in the demo page, then stage.compList[0].structure.bondStore.count returns 52 (for a molecule with 26 bonds). Is there a de-duplicated view that would be better to use?
2. Is there a mapping from atoms to bonds? (It's easy enough to generate one but this would be handy for (eventual) ring detection too). One idea, possibly a silly one would be for AtomStore to have fast typed array fields for the first 3(4?) bond indexes and in the (rare) cases where the valence is higher bonds could be stored in a plain old javascript array? Or maybe this is all better done in some helper class...

[fredludlow]

Ah, the mol2/sdf parser sets
s._dontAutoBond = true;
but the _afterParse function checks for this.dontAutoBond (no underscore) so the bonds get read from the mol2 file and then deduced from atom positions. I guess this is a typo/refactoring artefact?

EDIT:
Sorry, it's the parser's dontAutoBond property that's checked, not the structure.

[fredludlow]

So I've got a very (very!) rough version in my double-bond branch.
https://github.com/fredludlow/ngl/tree/double-bond
(This is very ugly code that basically just fleshes out the different steps and draws some offset lines at a harcoded distance)

Demo here:
http://fredludlow.com/ngl/db/ngl/examples/bondorders.html

Relevant code here:
https://github.com/fredludlow/ngl/blob/02f6ec4f947517c221487b5530291d297b194728/examples/js/bondorder.js

Thoughts/questions:
1. I think ring detection is going to be pretty essential
- This probably means having a way to traverse the graph of the structure (then ring detection is some variation on a depth-first search).
- Not calculated by default (not helpful for virus capsids!)
- Is this best done as another class? (StructureGraph), or just some changes to Structure/AtomStore?

2.The (unit) vector describing the direction of offset for a bond would be common between different representations - is that worth storing as a field in BondStore (maybe not populated until first required?)

Anyway - I can work this up into a proper representation class (or a parameter on the line/licorice/etc. classes) but I'd rather get your input on the organisation of it all before I start making any real changes/additions to the core code :)

Thanks,
Fred

[arose]

Ah, the mol2/sdf parser sets s._dontAutoBond = true;

thanks, fixed it dea82e27679e753d50dcb784bf923aafe75f410b

[arose]

Demo here: http://fredludlow.com/ngl/db/ngl/examples/bondorders.html

Awesome!

1. I think ring detection is going to be pretty essential

Well, depends on the style of the double bonds. If you want them inside the ring, then yes! If the style is similar to the image I posted in an earlier comment, then not, right?

This probably means having a way to traverse the graph of the structure (then ring detection is some variation on a depth-first search).
Not calculated by default (not helpful for virus capsids!)
Is this best done as another class? (StructureGraph), or just some changes to Structure/AtomStore?

I think at first this can be a private function in store/residue-type.js. The function should do the ring detection for ResidueType objects, not actual residues/atoms. This will scale nicely even to the largest structures. The only caveat is that this would not handle rings created by bonds between two or more residues, but I presume this is okay.

So, given two atomIndices (of a bond) we only need to get another atomIndex to calculate a plane. That atomIndex can be pre-calculated in the corresponding ResidueType instance. The result of the ring detection could be folded into that, by ensuring that that atomIndex is also part of the ring. For this to work, the bonding pattern/the connectivity must be part of the identity of ResidueType instance and should be set (or calculated) in the constructor.

2.The (unit) vector describing the direction of offset for a bond would be common between different representations - is that worth storing as a field in BondStore (maybe not populated until first required?)

No, I think the vector can be calculated fast enough on-the-fly.

I would push the actual offsetting of the bonds to the LineBuffer and CylinderImpostorBuffer/CylinderGeometryBuffer classes, by adding an optional shiftDirections (with an entry for each bond, normalized) argument and a shift parameter (distance and sign of the shift).

I hope this makes some sense. It is somewhat convoluted but that way it should be scalable :-)

If you don't mind, I would work on making the changes to the ResidueType class (also adjusting parsers) and creating a helper method to get an extra atomIndex as part of the BondProxy class. Please let me know what you think and which part you want to work on.

[fredludlow]

Well, depends on the style of the double bonds. If you want them inside the ring, then yes! If the style is similar to the image I posted in an earlier comment, then not, right?

True, and in the interests of getting something done easily this would save some effort, but I think aesthetically they're nicer placed within the rings :)

I think at first this can be a private function in store/residue-type.js. The function should do the ring detection for ResidueType objects, not actual residues/atoms. This will scale nicely even to the largest structures.

Okay - I hadn't quite understood when you suggested this earlier, but that makes sense now.

The only caveat is that this would not handle rings created by bonds between two or more residues, but I presume this is okay.

Yup, this must be pretty rare - stapled peptides formed by ring closing metathesis? - and basically only large macrocycles, where getting the bond inside the ring doesn't matter too much.

I would push the actual offsetting of the bonds to the LineBuffer and CylinderImpostorBuffer/CylinderGeometryBuffer classes, by adding an optional shiftDirections (with an entry for each bond, normalized) argument and a shift parameter (distance and sign of the shift).

Sounds sensible, I guess it also needs something like shiftOrders to specify double/triple/(quadruple?!) bond?

I hope this makes some sense. It is somewhat convoluted but that way it should be scalable :-)

Yup, sounds sensible.

If you don't mind, I would work on making the changes to the ResidueType class (also adjusting parsers) and creating a helper method to get an extra atomIndex as part of the BondProxy class. Please let me know what you think and which part you want to work on.

Happy to work on any bit of it (equally happy to leave any bit to you - I'm slightly anxious it might seem I'm barging in and trampling all over your beautifully organised code, which isn't my intention at all :) ).

I'll start looking at ResidueType (but will probably end up asking more questions before I get too far...)

[arose]

True, and in the interests of getting something done easily this would save some effort, but I think aesthetically they're nicer placed within the rings :)

Indeed

Sounds sensible, I guess it also needs something like shiftOrders to specify double/triple/(quadruple?!) bond?

no, I would create extra buffers

Single bonds
var cylinderBuffer1 = new NGL.CylinderBuffer(...);

Double bonds
var cylinderBuffer1 = new NGL.CylinderBuffer(...);
var cylinderBuffer2 = new NGL.CylinderBuffer(...);

Triple bonds
var cylinderBuffer1 = new NGL.CylinderBuffer(...);
var cylinderBuffer2 = new NGL.CylinderBuffer(...);
var cylinderBuffer3 = new NGL.CylinderBuffer(...);

Quadruple bonds
...

Happy to work on any bit of it (equally happy to leave any bit to you - I'm slightly anxious it might seem I'm barging in and trampling all over your beautifully organised code, which isn't my intention at all :) ).

The only reason the code is somewhat organised is to enable others to contribute, so please, barge in ;-)

[fredludlow]

Sorry, got sidetracked the last couple of days

no, I would create extra buffers

Single bonds
var cylinderBuffer1 = new NGL.CylinderBuffer(...);

Double bonds
var cylinderBuffer1 = new NGL.CylinderBuffer(...);
var cylinderBuffer2 = new NGL.CylinderBuffer(...);

CylinderBuffer is instantiated with data from getBondData, which can take a bondSet parameter - so something like?

var singleBondSet = ... // Some method for getting the bitset of single bonds
var doubleBondSet = ...// ditto 

singleBondData = structure.getBondData({bondset: singleBondSet, what: {as before})
doubleBondData = structure.getBondData({bondset: doubleBondSet, what: {shiftVector: true})

Then in getBondData when doing bondSet.forEach look up bp.atom1.residueType.getReferenceAtom(bp.atomIndex2)?
Probably not that precise invocation but that sort of general path through the objects?

And then, to get back to where I was going to start: ResidueType provides some method that: given a bond belonging to an residue of that type, tells you which the appropriate 3rd atom is...

The only reason the code is somewhat organised is to enable others to contribute, so please, barge in ;-)

Organisation and hand-holding both appreciated!

[arose]

doubleBondData = structure.getBondData({bondset: doubleBondSet, what: {shiftVector: true})

looks good

You can now pass bonds data when creating ResidueType objects and there are methods to get it (509a0d3b2363359cc76aa712aeaf5925b75d00f4).

o.structure.eachAtom( function( ap ){
    // atomProxy.getResidueBonds( firstOnly )
    console.log( ap.getResidueBonds(), ap.getResidueBonds( true ) )
} );

This should work with pdb, cif, mmtf and gro files. Will add support for sdf and mol2 files later today.

[arose]

Added support for sdf and mol2 files. You can now use residueProxy.getBonds() and atomProxy.getResidueBonds() to get bond data for atoms and residues. This should help getting the shift vectors. Let me know if you need access to anything else.

It might be fast enough to just use atomProxy.getResidueBonds( true ) without caching anything in ResidueType objects.

Next, I will work on the CylinderBuffer to support shiftVector data and shift parameter.

[arose]

I started with support for shiftDir data for CylinderBuffer in #146 using some fake vectors.

The vectors in shiftDir are expected to be normalized. By making them signed they could point to the center of a ring.

TODO

• [ ] use multiple bondSets in BallAndStickRepresentation to create a series of data for CylinderBuffer objects
• [ ] tweak shift parameter to get nice renderings
• [ ] calculate shiftDir array in Structure.getBondData()
• [ ] ring detection

[fredludlow]

I made a start on the ring/reference atom detection
fredludlow/ngl@f2df26a

There's a function assignBondReferenceAtoms that populates a (sparse) array aligned with the ResidueType.bonds arrays, containing the reference atom for double bonds (where it could be determined). It picks a reference atom in the smallest ring it can find for that bond.

I can tidy it up a bit but if you want to integrate it or change it significantly (it's at the "I've just splurged it into a file but not necessarily done it sensibly" stage) let me know and I'll hold off.

[arose]

I can tidy it up a bit but if you want to integrate it or change it significantly (it's at the "I've just splurged it into a file but not necessarily done it sensibly" stage) let me know and I'll hold off.

Great, that looks good enough to get a first working version and is modular so we can move bits and pieces around later.

Would you be okay with looking into "calculate shiftDir array in Structure.getBondData()" while I continue with making the Representation objects ready to use the data?

[fredludlow]

Would you be okay with looking into "calculate shiftDir array in Structure.getBondData()" while I continue with making the Representation objects ready to use the data?

Sure, will have a go.

[fredludlow]

Sorry, I managed to commit that referencing the wrong issue the first time round (trying to be too clever).

If you pass {what: {shiftDir: true, ...}} into getBondData then it will return an array with what I hope are correct normal-vectors, but only for those bonds with order > 1 (other elements of the array are zero). Not sure if this is ideal behaviour. Also current flow of logic notices if the reference atom is colinear, but doesn't try to do anything about it. If it's impossible to generate a reference atom it uses the origin.

[arose]

Hi, I abandoned the "shiftDir" approach and shift the position in getBondData, see the double-bond branch.

I think you can incorporate your code into it and make a Pull Request to my double-bond branch. We can than optimize it a bit before merging with the master branch.

Rendering the multiple bonds is activated by the multipleBond parameter in "ball+stick" and similar representations.

[arose]