Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for partial bonds #639

Closed
dxdc opened this issue Jan 30, 2023 · 18 comments
Closed

Support for partial bonds #639

dxdc opened this issue Jan 30, 2023 · 18 comments

Comments

@dxdc
Copy link
Contributor

dxdc commented Jan 30, 2023

Is your feature request related to a problem? Please describe.

Related to #368, would like to add a "dashed" or "partial" bond type. It could be added with bondOrder = 0.5. This would be useful to show transition states as well as hydrogen bonding, chelated metals, etc.

This is a natively supported feature in certain filetypes (e.g., MOL), but is not supported by 3Dmol. See: http://wiki.jmol.org/index.php/Support_for_bond_orders

For example, consider this MOL/SDF format, notice that the bond order is listed as "8".

Molecule002
  Mrv1903 01162316393D

  6  4  0  0  0  0            999 V2000
    0.4785    0.1547   -0.9430 H   0  0  0  0  0  0  0  0  0  0  0  0
    0.2171   -0.3394    0.0103 C   0  0  0  0  0  0  0  0  0  0  0  0
   -0.8330   -0.6810    0.0486 H   0  0  0  0  0  0  0  0  0  0  0  0
    0.5161    0.2460    0.8986 H   0  0  0  0  0  0  0  0  0  0  0  0
    1.2619   -1.9142    0.0667 Cl  0  0  0  0  0  0  0  0  0  0  0  0
   -1.6406    2.5338   -0.0811 Br  0  0  0  0  0  0  0  0  0  0  0  0
  1  2  8  0  0  0  0
  2  5  8  0  0  0  0
  2  4  8  0  0  0  0
  2  3  8  0  0  0  0
M  END
>  <anyBondsFromCoords>
true

$$$$

Describe the solution you'd like

Adding some code here would be terrific, e.g.

3Dmol.js/src/GLModel.ts

Lines 400 to 486 in a36f54c

if (atom.bondOrder[i] > 1 && atom.bondOrder[i] < 4 && !singleBond) {
var v = this.getSideBondV(atom, atom2, i);
var dir = p2.clone();
dir.sub(p1);
if (atom.bondOrder[i] == 2) { //double
v.multiplyScalar(0.1);
p1a = p1.clone();
p1a.add(v);
p1b = p1.clone();
p1b.sub(v);
p2a = p1a.clone();
p2a.add(dir);
p2b = p1b.clone();
p2b.add(dir);
if (c1 == c2) {
geoGroup.vertices += 4;
this.addLine(vertexArray, colorArray, offset, p1a, p2a, c1);
this.addLine(vertexArray, colorArray, offset + 6, p1b, p2b, c1);
}
else {
geoGroup.vertices += 8;
dir.multiplyScalar(0.5);
mpa = p1a.clone();
mpa.add(dir);
mpb = p1b.clone();
mpb.add(dir);
this.addLine(vertexArray, colorArray, offset, p1a, mpa, c1);
this.addLine(vertexArray, colorArray, offset + 6, mpa, p2a, c2);
this.addLine(vertexArray, colorArray, offset + 12, p1b, mpb, c1);
this.addLine(vertexArray, colorArray, offset + 18, mpb, p2b, c2);
}
}
else if (atom.bondOrder[i] == 3) { //triple
v.multiplyScalar(0.1);
p1a = p1.clone();
p1a.add(v);
p1b = p1.clone();
p1b.sub(v);
p2a = p1a.clone();
p2a.add(dir);
p2b = p1b.clone();
p2b.add(dir);
if (c1 == c2) {
geoGroup.vertices += 6;
this.addLine(vertexArray, colorArray, offset, p1, p2, c1);
this.addLine(vertexArray, colorArray, offset + 6, p1a, p2a, c1);
this.addLine(vertexArray, colorArray, offset + 12, p1b, p2b, c1);
}
else {
geoGroup.vertices += 12;
dir.multiplyScalar(0.5);
mpa = p1a.clone();
mpa.add(dir);
mpb = p1b.clone();
mpb.add(dir);
this.addLine(vertexArray, colorArray, offset, p1, mp, c1);
this.addLine(vertexArray, colorArray, offset + 6, mp, p2, c2);
this.addLine(vertexArray, colorArray, offset + 12, p1a, mpa, c1);
this.addLine(vertexArray, colorArray, offset + 18, mpa, p2a, c2);
this.addLine(vertexArray, colorArray, offset + 24, p1b, mpb, c1);
this.addLine(vertexArray, colorArray, offset + 30, mpb, p2b, c2);
}
}
}
else { //single bond
if (c1 == c2) {
geoGroup.vertices += 2;
this.addLine(vertexArray, colorArray, offset, p1, p2, c1);
} else {
geoGroup.vertices += 4;
this.addLine(vertexArray, colorArray, offset, p1, mp, c1);
this.addLine(vertexArray, colorArray, offset + 6, mp, p2, c2);
}
}
}
};

else { // single bond

                   var dashed = atom.bondOrder[i]  === 0.5; // note: we may need to convert the SDF input parsing to render this bondOrder as 0.5

                    if(c1 == c2) {
                        geoGroup.vertices += 2;
                        addLine(vertexArray, colorArray, offset, p1, p2, c1, dashed);
                    } else {
                        geoGroup.vertices += 4;
                        addLine(vertexArray, colorArray, offset, p1, mp, c1, dashed);
                        addLine(vertexArray, colorArray, offset+6, mp, p2, c2, dashed);                        
                    }
}

(Or, alternatively, we could just use some addLineDashed function. While addLineDashed is also used in the GLViewer code, I'm not sure how to port that over here).

Describe alternatives you've considered(Optional)

Custom cylinders or lines can be added on a per-molecule basis, but this is not very practical particularly when trying to use animations.

@dkoes
Copy link
Contributor

dkoes commented Jan 30, 2023

Making dashed bonds isn't particularly easy. What do you think of scaling the bond radius by the bond order when it is less than one? This is easy to implement. This provides flexibility in how much thinner these bonds are. For example, below there is a bond with bond order 0.5 and one with 0.1:

image

@dxdc
Copy link
Contributor Author

dxdc commented Jan 30, 2023

Thanks for getting back to this idea so quickly, @dkoes. That's an interesting option, albeit non-conventional. At these low values (like 0.1) it does start to look a bit better.

I had another thought too, what if the opacity (alpha value) of the bond color was changed to represent a dashed bond? For example, could we just "paint" the tube representing the bond with a series of low alpha values (even 0) to simulate the dashed effect?

@dkoes
Copy link
Contributor

dkoes commented Jan 30, 2023

Yes, adjusting the opacity would be a nice way to go, but unfortunately also would require a lot of work as transparency, as currently implemented, is done per-object (different bonds of the same molecule can't have different opacities).

Even though I agree it isn't the best possible visualization, I'm going to go with fractional bond orders scaling the bond radius. This is trivial to implement (I've already done it!) and gives the user some control over the style.

Perhaps in the future bond order 8 can be implemented to mean dashed and/or transparent.

dkoes added a commit that referenced this issue Jan 30, 2023
showNonBonded will show spheres for nonbonded atoms.
SDF files can now have fractional bond order which will scale the bond
radius.

Issues #639 and #640
@dxdc
Copy link
Contributor Author

dxdc commented Jan 30, 2023

@dkoes wow, thanks for the quick work on this! Looks good!

I did notice one quirk here. Is there some issue with how the bonds are being parsed now? Maybe I'm missing something.

Molecule019
  Mrv1903 01162316393D

  6  4  0  0  0  0            999 V2000
    0.3940    0.2894   -0.9430 H   0  0  0  0  0  0  0  0  0  0  0  0
   -0.1938    0.2941   -0.0093 C   0  0  0  0  0  0  0  0  0  0  0  0
   -0.9227   -0.5322    0.0461 H   0  0  0  0  0  0  0  0  0  0  0  0
    0.4329    0.3783    0.8947 H   0  0  0  0  0  0  0  0  0  0  0  0
    1.6322   -2.4898    0.0818 Cl  0  0  0  0  0  0  0  0  0  0  0  0
   -1.3425    2.0603   -0.0703 Br  0  0  0  0  0  0  0  0  0  0  0  0
  1  2  1  0  0  0  0
  2  3  1  0  0  0  0
  2  4  1  0  0  0  0
  2  6  0.1  0  0  0  0
  2  5  0.1  0  0  0  0
M  END
>  <anyBondsFromCoords>
true

$$$$

vs.

Molecule019
  Mrv1903 01162316393D

  6  4  0  0  0  0            999 V2000
    0.3940    0.2894   -0.9430 H   0  0  0  0  0  0  0  0  0  0  0  0
   -0.1938    0.2941   -0.0093 C   0  0  0  0  0  0  0  0  0  0  0  0
   -0.9227   -0.5322    0.0461 H   0  0  0  0  0  0  0  0  0  0  0  0
    0.4329    0.3783    0.8947 H   0  0  0  0  0  0  0  0  0  0  0  0
    1.6322   -2.4898    0.0818 Cl  0  0  0  0  0  0  0  0  0  0  0  0
   -1.3425    2.0603   -0.0703 Br  0  0  0  0  0  0  0  0  0  0  0  0
  1  2  1  0  0  0  0
  2  3  1  0  0  0  0
  2  6  0.1  0  0  0  0
  2  5  0.1  0  0  0  0
  2  4  1  0  0  0  0
M  END
>  <anyBondsFromCoords>
true

$$$$

Screen Shot 2023-01-30 at 10 32 49 AM

Screen Shot 2023-01-30 at 10 33 10 AM

@dkoes
Copy link
Contributor

dkoes commented Jan 30, 2023

I think you must have edited those SDF files by hand because they have 5 bond records but the bond count is 4 in the header. Change the 6 4 ... line to 6 5 ... and you'll see the additional bond.

@dxdc
Copy link
Contributor Author

dxdc commented Jan 30, 2023

🤦🏻 thank you @dkoes! I was editing these by hand to add the additional transition bond and missed that.

If I wanted to work on a feature to make the bonds actually dashed, how much effort do you think that would be and can you point to the best place in the code to tackle it? This is really useful, but I can see the advantage of having dashed bonds also.

@dkoes
Copy link
Contributor

dkoes commented Jan 31, 2023

You'd need to refactor GLShape.addDashedCylinder so that the core functionality that adds a dashed cylinder to a geometry is in its own helper function that can be called by both addDashedCylinder and drawBondSticks (like is done with GLDraw.drawCylinder).

@dxdc
Copy link
Contributor Author

dxdc commented Jan 31, 2023

@dkoes thanks for your suggestions! I did a little bit of research and had an alternate proposal, and wanted to see your thoughts:

  • Move the for loop and dash calculation from GLShape's addDashedCylinder to GLDraw.drawCylinder. GLDraw.drawCylinder will then take an additional (optional) parameter with the dash spec. In cases with no dashes, the for loop would just run once.

  • In GLShape, addDashedCylinder and addCylinder would now have a lot of overlapping code (perhaps some opportunity to combine these functions). For example, addDashedCylinder could now read:

        this.addDashedCylinder = function(cylinderSpec) {
            cylinderSpec.dashLength = cylinderSpec.dashLength || 0.25;
            cylinderSpec.gapLength = cylinderSpec.gapLength || 0.25;
            
            return this.addCylinder(cylinderSpec);
        };
  • In drawBondSticks, we can now pass an optional dash specification as the last parameter to drawCyl. I'm not sure if there would be any impact to drawStickImposter, not completely sure of the purpose for that.

@dxdc
Copy link
Contributor Author

dxdc commented Jan 31, 2023

  • We could rename GLDraw.drawCylinder to just GLDraw.drawCylinderHelper for example, and then use GLDraw.drawCylinder to actually run the for loop code and call drawCylinderHelper.

@dkoes
Copy link
Contributor

dkoes commented Jan 31, 2023

GLDraw.drawCylinder is a documented part of the API so its functionality should not change.

https://3dmol.org/doc/GLDraw.html#.drawCylinder

@dxdc
Copy link
Contributor Author

dxdc commented Jan 31, 2023

I was thinking we could make it:

drawCylinder(geo, from, to, radius, color, fromCap, toCap, dashed)

and, leave the dashed as an optional parameter, therefore making it legacy compatible. If dashed was not specified, there would be no change to the functionality.

My understanding of the for loop code in GLShape's addDashedCylinder is that basically we need to just call GLDraw drawCylinder multiple times in order to make a bond "dashed". So, there is some logic there to calculate the distance, new starting point, new ending point, etc.

Why not move that logic directly into drawCylinder? If there isn't any dash specified, then the for loop would just run once.

@dkoes
Copy link
Contributor

dkoes commented Feb 1, 2023

There are multiple configuration parameters for dashed - isn't just a boolean.
An alternative approach is to have a drawDashedCylinder that calls drawCylinder - this gets factored out of addDashedCylinder.

dxdc added a commit to dxdc/3Dmol.js that referenced this issue Feb 1, 2023
dxdc added a commit to dxdc/3Dmol.js that referenced this issue Feb 1, 2023
@dxdc
Copy link
Contributor Author

dxdc commented Feb 1, 2023

@dkoes I took your idea, and have a functional prototype. I don't really understand why the molecules I'm interested in are using drawStickImposter and not drawCylinder, but nevertheless, I've just made some code adjustments to show a working version.

There are some definite issues here, like the fact that (I think?) the bonds are being created in halves, but this could be fixed with some more clever calculations I think.

It would also be cool to have the ability to do "1.5" and "2.5" style bonds, where one of them is dashed and the other is solid, and maybe some variant of this could be used for full implementation.

Screen Shot 2023-02-01 at 12 41 11 PM

@dkoes
Copy link
Contributor

dkoes commented Feb 1, 2023

Stick imposters are more efficient and higher quality ways of drawing cylinders. The dashed code should use those.

@jaxmatrix
Copy link
Contributor

Dash can be created by rendering a dash texture in the area projected by a cylinder that connects the two atom. The height of the projected area will be taken as the height of the texture and it will be repeated along one direction to create a impression of dash.

@dkoes
Copy link
Contributor

dkoes commented Feb 6, 2023

I hadn't thought of that, but I doubt it would have the desired 3D effect with sticks. However, with lines I eventually need to reimplement them as 2D flat imposters (since line drawing has been deprecated in WebGL for a while) and this would work in that context (since the rendering is flat). Although probably makes more sense to incorporate the dashedness into the imposter code.

@dxdc
Copy link
Contributor Author

dxdc commented Feb 6, 2023

Stick imposters are more efficient and higher quality ways of drawing cylinders. The dashed code should use those

It does use the stickImposter code, I think? Open to your feedback after review.

dxdc added a commit to dxdc/3Dmol.js that referenced this issue Nov 1, 2023
@dxdc dxdc closed this as completed Nov 1, 2023
@JamMaster1999
Copy link

@dxdc Did you end up getting this to work? I am trying to visualize hydrogen bonding in DNA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants