3dmol / 3Dmol.js

WebGL accelerated JavaScript molecular graphics library
https://3dmol.org/
Other
813 stars 195 forks source link

Dashed bonds for bond order < 1 (revised) #740

Closed dxdc closed 11 months ago

dxdc commented 11 months ago

Builds on https://github.com/3dmol/3Dmol.js/pull/641

codecov[bot] commented 11 months ago

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (2e1c453) 83.17% compared to head (84c512a) 83.20%.

Files Patch % Lines
src/GLModel.ts 94.73% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #740 +/- ## ========================================== + Coverage 83.17% 83.20% +0.03% ========================================== Files 141 141 Lines 11484 11488 +4 Branches 2126 2125 -1 ========================================== + Hits 9552 9559 +7 + Misses 1602 1599 -3 Partials 330 330 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

dxdc commented 11 months ago

@dkoes one comment here:

gapLength = (cylinderLength - totalDashLength) / totalSegments;

This is actually not the correct way to calculate it. Probably it should be more exactly something like:

gapLength = totalSegments > 1 ? (cylinderLength - totalDashLength) / (totalSegments - 1) : 0;

However, that yields some strange behavior as far as the cylinder coloration - because it looks as if the bonds are created in equal halves - and you can get some half bonds of differing colors.

I'm not an expert on the downstream steps here, and I'm not entirely sure why having a slightly smaller gapLength fixes that issue. I imagine there is something more robust.

dkoes commented 11 months ago

I'm not sure what the issue is with coloration - the test cases look like an improvement to me, so I'm going to go ahead and merge.