-
Notifications
You must be signed in to change notification settings - Fork 193
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
Dashed bonds for bond order < 1 #641
Conversation
Codecov Report
@@ Coverage Diff @@
## master #641 +/- ##
==========================================
- Coverage 83.15% 83.14% -0.02%
==========================================
Files 141 141
Lines 11459 11481 +22
Branches 2120 2123 +3
==========================================
+ Hits 9529 9546 +17
- Misses 1601 1605 +4
- Partials 329 330 +1
|
Will need to add a test to |
I see images for both partialbonds.js and partialbonds2.js, but only a single partialbonds2.js test file. Is there a file missing? |
@dkoes I revised the logic and I'm getting really nice looking results now. I also added the tests. The calculation for dashes and gaps can probably be further improved, as the bond radius actually affects the "thickness" of each dash. Hardcoding the number of dashes isn't the right answer either, because bond lengths can vary. Perhaps allowing users to supply dashLength and gapLength as parameters could help for unique cases (the function supports it, but they are not able to be user-supplied at the moment). It wouldn't be that difficult to add, but I'm not sure if that should be added on a global basis (e.g., in the It seems like doing it on a per-bond basis would be better, but I don't know if the existing data model supports that. |
No, you already had a test for partialbonds.js, which was an existing feature to support bondOrder < 1. But, with this change, it adds the dashed appearance so I regenerated that image. |
Currently, bond styling is basically determined by which atom is processed first, which isn't ideal, but it often sufficient. Do you think it is good enough for a merge? |
Just made some updates to the stick style spec, to enable support for more custom options and specific dashed bond parameters. I think it can be merged, but welcome any add'l feedback! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much!
Nice idea. I had been looking at shaders that discard segments, but I like this idea of simply drawing a few pieces. |
@dkoes I'm working on an improved algorithm to approach the dashed calculation in a more logical and structured way. My rough requirements/assumptions were:
Do you agree with the above? For example, we can also scale the dash/gap sizes instead. One of the issues I'm struggling with is how to best understand radius. It definitely affects the "apparent" gap size, but we need to be careful because I'm not sure how we need to account for this. Do you have any suggestion? My earlier testing indicated it wasn't quite as simple as combining radius with gapSize. The code below is a simplified variant, and probably closer to something more optimal than the first version merged into the code. If you have any advice on how to approach this and improve it further would welcome it. function calculateDashGapCoordinates(cylinderLength, gapLength = 0.25, dashLength = 0.1, radius = 0.05, precision = 3) {
// Ensure non-negative values and set a minimum dash length
gapLength = Math.max(gapLength, 0);
dashLength = Math.max(dashLength, 0.01); // Minimum dash length
radius = Math.max(radius, 0);
// Handle cases where dashLength or radius + dashLength exceeds cylinderLength
if (dashLength > cylinderLength) {
dashLength = cylinderLength;
radius = 0; // Reset radius as the entire cylinder is a dash
gapLength = 0; // No gap as the dash fills the cylinder
} else if (radius + dashLength > cylinderLength) {
// Adjust dashLength to fit within the cylinder
dashLength = Math.min(dashLength, cylinderLength - radius);
}
// TODO: handle case of gapLength being too large within above combinations
// Calculate the number of segments and adjust the gap length accordingly
let segmentLength = dashLength + radius + gapLength;
let totalSegments = Math.floor((cylinderLength - dashLength) / segmentLength);
let remainingSpace = cylinderLength - totalSegments * segmentLength - dashLength;
let adjustedGapLength = gapLength + (totalSegments > 0 ? remainingSpace / totalSegments : 0);
// Construct coordinates for each segment
let coordinates = [];
let currentPosition = 0;
// Iterate over each segment
for (let i = 0; i <= totalSegments; i++) {
let dashStart = currentPosition;
let dashEnd = dashStart + dashLength;
// Add dash coordinates
coordinates.push({ dash: [dashStart.toFixed(precision), dashEnd.toFixed(precision)] });
currentPosition = dashEnd + radius;
// Add gap coordinates if there's enough space remaining
if (i < totalSegments && currentPosition + adjustedGapLength < cylinderLength) {
let gapEnd = currentPosition + adjustedGapLength;
coordinates.push({
gap: [currentPosition.toFixed(precision), gapEnd.toFixed(precision)]
});
currentPosition = gapEnd;
}
}
// Add the final dash
if (currentPosition < cylinderLength) {
coordinates.push({
dash: [currentPosition.toFixed(precision), cylinderLength.toFixed(precision)]
});
}
return coordinates;
}
function runTests() {
const testCases = [
{
description: "Standard case",
cylinderLength: 2,
gapLength: 0.25,
dashLength: 0.1,
radius: 0.05
},
{
description: "Large dash and gap lengths",
cylinderLength: 2,
gapLength: 1,
dashLength: 1,
radius: 0.05
},
{
description: "Precision number",
cylinderLength: 2.33333,
gapLength: 0.25,
dashLength: 0.1,
radius: 0.05
},
{
description: "Very small gap length",
cylinderLength: 2,
gapLength: 0.01,
dashLength: 0.1,
radius: 0.05
},
{
description: "Very large radius",
cylinderLength: 2,
gapLength: 0.25,
dashLength: 0.1,
radius: 1
}
];
testCases.forEach(testCase => {
const { cylinderLength, gapLength, dashLength, radius, description } = testCase;
console.log(description);
console.log(JSON.stringify(calculateDashGapCoordinates(cylinderLength, gapLength, dashLength, radius)));
console.log("--------------------");
});
}
// Run the test suite
runTests();
|
I think what I would do is have gapLength, dashLength etc. be treated literally with no attempt to make things pretty or even reasonable, but have an auto setting (which would be the default) that makes things pretty (possibly only supporting a variable radius; a gap to dash ratio if you want to be fancy). |
That makes sense, although I'm not sure how I'm also wondering if it should just be left as is in a "best fit" kind of scenario -- meaning, it will draw as many dashes/gaps as it can until it hits the end of the cylinder, with the constraint that it will try to start and end with a dash. One question though, do you have any feedback on how radius impacts the overall appearance/calculation? I'm not familiar with how that part of the code is working, only that a larger radius definitely impacts the apparent gap between the molecules. |
For example, we could override the color argument, e.g.
color = new $3Dmol.Color(0xC8C8C8);
Sample SDF file: