3dmol / 3Dmol.js

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

[BUG] After 1.6.1 upgrade, "inFront" option no longer works on labels (v1.6.0 breaks it) #484

Closed andfaulkner closed 3 years ago

andfaulkner commented 3 years ago

Description Labels no longer appear in front of atoms when the "inFront" config option is set.

To Reproduce Attach a label to an atom, and view: viewer.addLabel(atomIndex, {fontSize: 11.5, inFront: true}, {serial: atom.serial});

Note: code simplified for purpose of example.

Expected behavior The labels should appear in front of the atoms, like they used to.

Screenshots Before upgrade: Screen Shot 2020-11-23 at 5 01 26 PM

After upgrade: Screen Shot 2020-11-23 at 5 05 11 PM

Note that the colour differences on the labels don't reflect the issue - I confirmed that the config options are still getting applied, I just changed the colour settings to make it clearer where the labels are being rendered post-change.

Desktop (please complete the following information):

dkoes commented 3 years ago

I cannot reproduce this. Can you provide a reduced test case? Does the following have labels in front for you? https://3dmol.csb.pitt.edu/tests/auto/generate_test.cgi?test=testlabelfront

andfaulkner commented 3 years ago

@dkoes It does. Before creating a test case, is it possible the issue is coming from this change? https://github.com/3dmol/3Dmol.js/issues/482

I built 3dmol locally to get access to the feature early (it was a blocker on my end), so it could be that that's where the issue was introduced.

dkoes commented 3 years ago

I don't see how. Does the test case above work for you?

Have you downgraded to an older version of 3dmol without changing your code to verify the issue isn't with your code?

andfaulkner commented 3 years ago

Agreed...I looked at your code changes and also didn't see how, which is why I attributed it to v1.6.1.

I did try downgrading to v1.5.4 and the issue disappeared. However, I loaded the earlier versions in through the CDN, rather than building locally, so it's possible that's a factor. My code itself didn't change - it only affected the build/load process...but I've been using 3dmol for about 2 years now, and every other time I've had issue related to that, it's just stopped working altogether.

Based on all this, I'm going to try the following and get back to you:

BTW what version of node.js are you building with?

(And sorry for the latency...my daughter was up all teething last night and it's delayed everything today)

andfaulkner commented 3 years ago

It turns out the issue was introduced with v1.6.0.

It still happens when v1.6.1 is loaded into the app by CDN: <script src="https://cdnjs.cloudflare.com/ajax/libs/3Dmol/1.6.1/3Dmol-nojquery.js"></script>

I retested v1.5.4 to double-check, and it's still working there; then tried v1.6.0, and it broke. This is all with no code changes in between (either in the codebase itself, or the build).

andfaulkner commented 3 years ago

Found the culprit - it's caused by this commit: https://github.com/3dmol/3Dmol.js/commit/2e3152e5659c631b0544520be14064308e78aeed

When the commit is reversed (by manually editing the output file), the issue disappears. Reverting it brings the issue back.

Note: commit was reversed (and reverted after) by setting the module to use 3dmol.js instead of 3dmol.min.js, then editing the output file manually (i.e. changing the position of renderPlugins(this.renderPluginsPost, scene, camera);). We can thus confirm this is the source of the bug - no other artifacts could have crept in.

Is this a bug, given that it's an unexpected breaking change? And does the source of the issue tell us what's causing the problem on my end?

If not, I'll try to build a test case for you (it'll be very labour-intensive given the complexity of the component I've built around 3DMol - and because I wrote it in React.js & Typescript rather than vanilla JS - but if all else fails I can do it).

dkoes commented 3 years ago

Are you setting the opacity of your molecules?

dkoes commented 3 years ago

I think you must have set the opacity to something not 1.0 on your atoms as that is the only way I can reproduce the problem. I have pushed a fix. Does it work now?

andfaulkner commented 3 years ago

Will give this a try in a couple of days, I'm stuck afk for a bit

andfaulkner commented 3 years ago

@dkoes Yep, this 100% fixes it. Thanks so much!