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

Unable to visualise hetatom surfaces when calling viewer.addSurface() 3Dmol 2.0.3 #709

Closed
DanBuchan opened this issue Aug 9, 2023 · 24 comments
Labels

Comments

@DanBuchan
Copy link

DanBuchan commented Aug 9, 2023

In our website we have an ancient version of 3dmol (circa 2017) that we use to visualise the embedding of proteins in an "artificial" bilayer of hetatoms. This works by calling

viewer.addSurface($3Dmol.SurfaceType.VDW, {'opacity':0.8, colorscheme: 'whiteCarbon'}, {hetflag:true});

We are porting our website to react and are updating our assorted modules and have updated to 3Dmol 2.0.3. We have the following code that draws the molecular viewer, where the function here is identical to the working code in our previous web site

import * as $3Dmol from '3dmol/build/3Dmol.js';


export function display_structure(mol_container, pdb_data)
{
  let hotspot_color = function(atom){
    if(atom.b === 1.0){atom.color = 'red'; return 'red';}
    if(atom.b === 0.5){atom.color = 'black'; return 'black';}
    if(atom.b === 50){atom.color = 'white'; return 'white';}
    if(atom.b === 100){atom.color = 'red'; return 'red';}
    atom.color = 'blue';
    return("blue");
  };
  let element = mol_container;
  let config = { backgroundColor: '#ffffff' };
  let viewer = $3Dmol.createViewer( element, config );
  viewer.addModel( pdb_data, "pdb" );                       /* load data */
  viewer.setStyle({}, {cartoon: {colorfunc: hotspot_color}});  /* style all atoms */
  viewer.addSurface($3Dmol.SurfaceType.VDW, {'opacity':0.8, colorscheme: 'whiteCarbon'}, {hetflag:true});
  viewer.zoomTo();                                      /* set camera */
  viewer.render();                                      /* render scene */
  viewer.zoom(1.7, 3000);
}

Here mol_container is a react_ref() to the div that holds the 3Dmol instance and pdb_data is the text of our pdb file.

When this renders the PDB chain appears in dark blue as expected but the Firefox console throws the following errors

ReferenceError: MarchingCubeInitializer is not defined
Uncaught (in promise) error { target: Worker, isTrusted: true, message: "ReferenceError: MarchingCubeInitializer is not defined", filename: "blob:http://bioinf.cs.ucl.ac.uk/ada9ce02-6fa5-4b5e-8c79-eff980a12dd8", lineno: 4, colno: 20, srcElement: Worker, eventPhase: 0, bubbles: false, cancelable: true, … }

We see equivalent errors in both Firefox and Chrome

Incorrect Behaviour
Screenshot 2023-08-09 at 16-22-19 PSIPRED Workbench

Correct/previous behaviour
Screenshot 2023-08-09 at 16-22-31 PSIPRED Workbench

@DanBuchan DanBuchan added the bug label Aug 9, 2023
@dkoes
Copy link
Contributor

dkoes commented Aug 9, 2023

Would it be possible to provide a small, self-contained example?

@dkoes
Copy link
Contributor

dkoes commented Aug 9, 2023

If you look at 3dmol/build/3Dmol.js you should see MarchingCubeInitializer defined in the outermost scope:

//Encapsulate marching cube algorithm for isosurface generation
// (currently used by protein surface rendering and generic volumetric data reading)
class MarchingCubeInitializer {
    constructor() {
...

It would be interesting to know if adding window.MarchingCubeInitializer = $3Dmol.MarchingCubeInitializer; somewhere helped.

@dkoes
Copy link
Contributor

dkoes commented Aug 9, 2023

There is a very good chance that your build system is renaming variables. If put a breakpoint on this line:

$3Dmol.SurfaceWorker = window.URL ? window.URL.createObjectURL(new Blob([$3Dmol.workerString],{type: 'text/javascript'})) : undefined;

you should check that the names of the classes within $3Dmol.workerString are correct.

dkoes added a commit that referenced this issue Aug 9, 2023
Get the name of MarchingCubeInitializer programmatically to be more
robust to name changes by build systems.
@dkoes
Copy link
Contributor

dkoes commented Aug 9, 2023

Please try the fix I just committed and see if it works.

@DanBuchan
Copy link
Author

DanBuchan commented Aug 11, 2023

All of the machines are down in our department for the weekend but I'll look in to these on Monday. You may well be right that react's build system is doing something screwy with the variable names

@DanBuchan
Copy link
Author

DanBuchan commented Aug 14, 2023

Ok. as you suggested here's a minimal code example of this bug

https://github.com/DanBuchan/3dmol_bug

However when I run this it works perfectly fine with or without your fix. So clearly there is something more screwy going on with the namespace in our full application (see here https://github.com/psipred/psipred_react)

When I get a moment tomorrow I'll see if I can't track down the issue. I should get a moment in the morning to try that break point and/or your fix.

@DanBuchan
Copy link
Author

To update, if I apply the fix in commit c43c34c I get a new error

_classCallCheck is not defined 

Which the debugger throws as this construct:

var MarchingCube = new e();;
function e(t,r,n){_classCallCheck(this,e),this.data=new Int32Array(t*r*n*3),this.width=r,this.height=n};

@dkoes
Copy link
Contributor

dkoes commented Aug 15, 2023

_classCallCheck isn't anything we generate - definitely something in your build system munging the code. Is this error happening in the worker? Try setting $3Dmol.syncSurface = true; to disable the webworkers.

I suspect your build system is modifying the code in 3Dmol.workerString. Since this is the code for the worker, it needs to be self-contained.

If there's a setting in your build system to tell it to leave 3Dmol.MarchingCubeInitializer alone (or even all of 3Dmol..) that would probably be the best solution. _classCallCheck is a check babel adds (http://nicholasjohnson.com/blog/what-is-babel/#classes-in-babel).

What does your babel config look like?

dkoes added a commit that referenced this issue Aug 15, 2023
Put in a stub for _classCallCheck in worker.
@dkoes
Copy link
Contributor

dkoes commented Aug 15, 2023

See if the above commit fixes the problem.

@DanBuchan
Copy link
Author

DanBuchan commented Aug 18, 2023

Ok, excellent/hilarious hack. I now get to a new error

n.initparm is not a function

FWIW it did dawn on me that this might be down to our website project being built with an older babel but I've updated that and it did not fix this issue.

@dkoes
Copy link
Contributor

dkoes commented Aug 18, 2023

The ProteinSurface class has an initparm method. babel is likely renaming it and not updating the code in workerString. Did setting syncSurface work?

@dkoes
Copy link
Contributor

dkoes commented Aug 18, 2023

I don't understand why these things are getting renamed - they are APIs and are marked public in the original TypeScript.

@DanBuchan
Copy link
Author

yeah I simply don't understand why they should be getting touched at all, and, as per the toy example I built (above) that all works fine with the same version of babel. This is obvoiously a bug with babel or maybe with how we're important various packages

@dkoes
Copy link
Contributor

dkoes commented Aug 23, 2023

Would you be willing to share your configuration file?

@DanBuchan
Copy link
Author

DanBuchan commented Aug 23, 2023

Funnily enough I have just spent a day porting our build from babel to esbuild to see if that resolves the issue. But I still get

ReferenceError: u is not defined [32bbfca2-6e79-4f6b-affe-1dac8a93984c:8:10677](blob:http://bioinf.cs.ucl.ac.uk/32bbfca2-6e79-4f6b-affe-1dac8a93984c)
Uncaught (in promise) 
error { target: Worker, isTrusted: true, message: "ReferenceError: u is not defined", filename: "blob:http://bioinf.cs.ucl.ac.uk/32bbfca2-6e79-4f6b-affe-1dac8a93984c", lineno: 8, colno: 10677, srcElement: Worker, eventPhase: 0, bubbles: false, cancelable: true,  }

I have also attempted to move import * as $3Dmol from '3dmol/build/3Dmol.js'; to the very top of the application and add it as part of Window to see if being in the global scope helped

Happy to add config files for the project, if you're curious about anything else then the project is at: (https://github.com/psipred/psipred_react);

package.json

{
  "name": "psipred_react",
  "version": "0.1.0",
  "private": true,
  "dependencies": {
    "@testing-library/jest-dom": "^5.16.5",
    "@testing-library/react": "^13.4.0",
    "@testing-library/user-event": "^13.5.0",
    "3dmol": "^2.0.3",
    "blueimp-canvas-to-blob": "^3.29.0",
    "clipboard": "^2.0.11",
    "d3": "^5.16.0",
    "d3-svg-legend": "^2.25.6",
    "datatables.net-dt": "^1.13.4",
    "file-saver": "^2.0.5",
    "filesaver": "^0.0.13",
    "jquery": "^3.7.0",
    "jszip": "^3.10.1",
    "jszip-utils": "^0.1.0",
    "lodash": "^4.17.21",
    "moment": "^2.29.4",
    "react": "^18.2.0",
    "react-copy-to-clipboard": "^5.1.0",
    "react-dom": "^18.2.0",
    "react-scripts": "5.0.1",
    "web-vitals": "^2.1.4"
  },
  "scripts": {
    "start": "node serve.mjs",
    "build": "node build.js",
  },
  "eslintConfig": {
    "extends": [
      "react-app",
      "react-app/jest"
    ]
  },
  "browserslist": {
    "production": [
      ">0.2%",
      "not dead",
      "not op_mini all"
    ],
    "development": [
      "last 1 chrome version",
      "last 1 firefox version",
      "last 1 safari version"
    ]
  },
  "devDependencies": {
    "esbuild": "^0.19.2",
    "esbuild-serve": "^1.0.1"
  }
}

and build.js for esbuild

// build.js
const esbuild = require("esbuild");
//console.log(process.env.PUBLIC_URL);
esbuild.build({
  entryPoints: ["./src/index.js"],
  outfile: "./public/static/js/app.js",
  minify: true,
  bundle: true,
  loader: {
    ".js": "jsx",
  },
  plugins: [],
  define: {"process.env.PUBLIC_URL": JSON.stringify(process.env.PUBLIC_URL), }
}).catch(() => process.exit(1));

@dkoes
Copy link
Contributor

dkoes commented Aug 23, 2023

Can you help me build your project? When I run npm start or npm run build I get:


✘ [ERROR] Expected value for define "process.env.PUBLIC_URL" to be a string, got undefined instead

    /home/dkoes/git/psipred_react/node_modules/esbuild/lib/main.js:299:10:
      299 │     throw new Error(`Expected value for ${what}${key !== void 0 ? " " + quote(key) : ""} to be a string, got ${typeof value} instead`);
          ╵           ^

    at validateStringValue (/home/dkoes/git/psipred_react/node_modules/esbuild/lib/main.js:299:11)
    at pushCommonFlags (/home/dkoes/git/psipred_react/node_modules/esbuild/lib/main.js:399:37)
    at flagsForBuildOptions (/home/dkoes/git/psipred_react/node_modules/esbuild/lib/main.js:433:3)
    at buildOrContextContinue (/home/dkoes/git/psipred_react/node_modules/esbuild/lib/main.js:1019:9)
    at buildOrContextImpl (/home/dkoes/git/psipred_react/node_modules/esbuild/lib/main.js:1003:5)
    at Object.buildOrContext (/home/dkoes/git/psipred_react/node_modules/esbuild/lib/main.js:786:5)
    at /home/dkoes/git/psipred_react/node_modules/esbuild/lib/main.js:2187:15
    at new Promise (<anonymous>)
    at Object.build (/home/dkoes/git/psipred_react/node_modules/esbuild/lib/main.js:2186:25)
    at Object.build (/home/dkoes/git/psipred_react/node_modules/esbuild/lib/main.js:2035:51)


@dkoes
Copy link
Contributor

dkoes commented Aug 23, 2023

Also, the steps to easily reproduce the problem (I can get it running if I checkout a pre-esbuild commit, but I don't know what to do next).

@DanBuchan
Copy link
Author

DanBuchan commented Aug 24, 2023

Ok, I've built a version of our app that doesn't need to call our API, and immediately loads the non-working display.

You can find it at https://github.com/DanBuchan/3d_mol_bug_complete, you can start it with

export PUBLIC_URL=''
npm install
npm run build
npm start

Then the issue should be viewable at the URLhttp://127.0.0.1:3000/interface/

The site is loading the version of 3Dmol available at ./node_modules/3dmol/build/3Dmol.js

src/shared/index.js contains the display_structure() function that actually builds the instance of 3Dmol and src/interface/results_structure,js processes the pdb file data and calls display_structure(). You can find the pdb data as a string in src/interface/data.js


While I was preparing this I realised that our site makes use of a non-standard design pattern to provide multiple entry points for a react app and also loads a lot of other javascript, either through script elements or as node modules. As this seems to be a namespacing issue I figured I'd see if I could replicate these in my previous attempt at a minimal example of the bug (https://github.com/DanBuchan/3dmol_bug). I added to this repo the same react class inheritance logic for multiple entry points and I have it load all the same node modules and js packages (either in the code or in the html template). Even with these changes it still works fine and does not replicate this issue.

@dkoes
Copy link
Contributor

dkoes commented Aug 29, 2023

Did you forget to commit some files to that repo? There's only a README.

@DanBuchan
Copy link
Author

Ah sorry about that, just got back from holiday and have pushed all those bits (clearly I would forget my head if it wasn't screwed on)

dkoes added a commit that referenced this issue Sep 13, 2023
Be more robust to name munging in surfacd worker string.
@dkoes
Copy link
Contributor

dkoes commented Sep 13, 2023

That was very helpful. The problem was indeed that your build systems was munging the names in the code, and I figured out a way to make that not an issue in the above commit (my previous attempt only half worked).

@dkoes
Copy link
Contributor

dkoes commented Sep 20, 2023

I forgot to tag this issue, but this fix should now be rolled out in 2.0.4. Please confirm and close.

@DanBuchan
Copy link
Author

Hi, I'll get this tested next week when I get a moment and then I'll close the issue.

@DanBuchan
Copy link
Author

Looks to be working fine for me now. Thanks so much!

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

No branches or pull requests

2 participants