Ngl: Shader error when upgrading to Three.js r112 (WebXR)

[jussty]

I wrote a custom minimal Viewer to embed NGL Representations into existing projects based on Three.js. Full control over the scene is maintained by the parent project, only the geometries are exported into a THREE.Group that can be freely manipulated. It was some effort but works quite well (see demo, branch ThreeJSViewer in my fork, glitch to play around with the API).

Besides needing this for another project I wanted to update @KJStrand A-Frame component and create some vanilla Three.js WebXR demo. WebXR is a first class citizen in Three.js now and it is really awesome even with a mobile headset like the Oculus Quest, but projects need to follow a few conventions (no forced camera movement, units are meters, floor plane for walking around an object etc.), so it would make sense to include NGL objects in a template like this. In the long run this should be done properly by enabling renderer.xr in NGL.Stage, but it will be some effort to make everything compatible (e.g. labels). Also, NGLs rendering pipeline is fairly advanced with its special treatment of still frames and the different cameras, so it might make sense to keep it completely separate (related issue).

Unfortunately, WebXR was included into Three.js with version 112, while we are on 111.
I made a branch to make NGL compile up to 115, but between 111 and 112 something changed so that the shaders don't work anymore. Stuff is rendered but stays invisible, and WebGL warnings like "GL ERROR :GL_INVALID_OPERATION : glDrawElements: Source and destination textures of the draw are the same." are thrown (in Chrome, Firefox has slightly different warnings).

I did some research but have actually no idea what could cause this. Three.js changelogs and commits look unsuspicious to me. Maybe https://bugs.chromium.org/p/chromium/issues/detail?id=421695 is related, but it certainly happens on the version change, while this bug would probably be a deeper issue.

You can see the effect here: https://jussty.github.io/ngl-r112/examples/embedded.html or in the other examples.

A big disadvantage of including NGL into other projects is that Three.js version needs to match exactly. I don't mind maintaining releases of ThreeJSViewer for different versions if you don't want to have that burden in the main repo. A-Frame even ships with their own fork, but the 1.0.0 releases are also based off some WebXR enabled r111.5 which has the same problems.

Also I am certain the API could be even simpler, without needing references to renderer, camera AND scene, but since some of the low level shader utils need these objects for their magic I kept it in for now.

[jussty]

I just realized you are working on Mol* now, so maybe it makes more sense to base the components off there? Currently trying to work through the API.

[garyo]

I'm embedding NGL into my app as well, and I've been porting the three.js versions forward (I'll be happy to do 112 and forward as well -- please send patches if you have them.) It's true that the three.js version has to pretty much match. In my app I've set things up so NGL does _not_ include its own copy of three.js; it has to use the global one that the rest of the app uses. Even if the versions match, two copies of three.js causes subtle havoc. (Three.js uses a global var for object index for instance, and with two copies of that, things get confused fast.) So make sure NGL is using the _same_ three.js (not just the same version).

[jussty]

Thanks for the answer and the heads up, this would have been my next step! Nevertheless at least the API needs to match, so if its used by others it needs to be very clear which Three.js version should be brought in as peer dependency. Still needs to be maintained with every upgrade of the parent project, especially with abstract high level stuff like A-Frame.

I can make a PR from my patch branch (https://github.com/arose/ngl/compare/master...jussty:three_upgrade) but at the moment there are only a few basic commits to make sure TS compiles. Three.js changelog does not suggest any really problematic changes, nevertheless I wanted to test everything before making a PR. Show stopped on r112 already unfortunately...

[bergwerf]

@garyo Could you explain a bit about your setup for one THREEjs? Are you importing ngl as a module and compiling all at once? In my current setup I do not use a module bundler for my main app and I didn't know the THREE version inside ngl.js would interfere with other THREE copies on the same page (I didn't try yet). It would be nice if I can get an ngl.js that uses a THREE that is available in the global JavaScript context (which you seemed to imply), but I have no idea how to do that.

[jussty]

@garyo Could you explain a bit about your setup for one THREEjs? Are you importing ngl as a module and compiling all at once?

The relevant commit should be this: https://github.com/arose/ngl/commit/a1a8c594c0103fa7f173b9ddff98a3fb83b7d491 , @garyo please correct me if I am wrong or missed something.

In my current setup I do not use a module bundler for my main app and I didn't know the THREE version inside ngl.js would interfere with other THREE copies on the same page (I didn't try yet). It would be nice if I can get an ngl.js that uses a THREE that is available in the global JavaScript context (which you seemed to imply), but I have no idea how to do that.

If NGL stays in its own embedded viewer/canvas/stage there should be no problem at all, since NGL doesn't export its internal instance of THREE into the global namespace. So you can have several Three.js instances even with different versions running on the same web page, as long as you don't try to e.g. copy objects between them. You may run into a shortage of WebGL rendering contexts if you overdo it (depends on hardware/implementation how many you can have simultaneously).

I would even argue it is better to keep them separated for this use case, as you will run into a slurry of problems if different embedded viewers share the same internal state.

The purpose of my exporter is to directly work with NGL objects in the same WebGLRenderer, e.g. as part of a WebXR project or as A-Frame component. In this case it is important to use the same _instance_ of Three.js, not just the same _version_.

[garyo]

Yes, that's exactly the commit. You're right that if you never interact with three.js objects in the NGL window, you might be safe. But in my case, as soon as I created a three.js object into the NGL scene, I had problems because my app has one copy of three and NGL has its own. As for not sharing state, I think three.js is fine with multiple viewports, so that should work. I think overall any embedding will be more robust with a single instance of three.js.

[garyo]

I'd like to create a PR for that commit (a1a8c59) as well as a couple of other fixes I have queued up, but at this point I have enough PRs open, so I'm waiting for others to be merged first.

[giagitom]

Hi @garyo ,
Sorry but I am new to this. If i try to set the environment variable and build NGL with this command NGL_EXTERNAL_THREEJS=1 npm run-script build i get errors like these when loading NGL in my page:

colormaker.js:16 Uncaught TypeError: Cannot read property 'Color' of undefined at colormaker.js:16 at ngl.dev.js:4 at ngl.dev.js:5

Uncaught TypeError: NGL.Matrix4 is not a constructor

How should I import three in this case?

[garyo]

Indeed -- you have to add three.js to your project (npm or yarn) and probably import Three from 'three' yourself. NGL just has Three set as a global dependency, so it doesn't try to import it itself.
If you look at the generated Javascript in (e.g.) build/js/ngl.esm.js you'll see import lines like

import {Vector2, Vector3, ... Color, ...} from 'three';

so you have to provide three in your node_modules so it can be imported.

If I get some time I'll try to put together a small webpack/three/NGL project but I'm pretty busy right now.

[fredludlow]

I guess our current situation stems from the fact that back in the day JS build tools were a lot less mature and not all the big libraries played nicely with npm/yarn

There are a few ways you might want to deploy NGL but I think it comes down to:

1. A script tag pointing to a ngl.js including Three.js (either because you're keeping it simple or want to make sure ngl is cached or whatever)
2. Imported into your own project (either ES6 module import or the older require('ngl') syntax) in which case supplying Three.js should be a concern of npm/yarn - by default it should 'just work' but give you the flexibility to declare Three as external/global/specific version or whatever

Proposal:

We move Three.js to a dependency (not a devDependency) with an appropriately specified version range. This means that when you npm install ngl, three will get installed alongside (and if versions are compatible your other code that uses Three will use the same instance).

We create three build targets:

• A UMD modlue (.js file) including Three.js - (reproducing the current behaviour of ngl[.dev].js and still the quickest way to get "something" working in a web-page)
• A UMD module (.js file) without Three.js - (cf. @garyo's patch, but this becomes the 'main' entry in package.json too. so you can require('ngl') from within node)
• An ES6 module (.esm.js file) without Three.js - and leave it up to npm/yarn etc to satisfy the dependency - this allows e.g. import { Stage } from 'ngl' in an ES6 project

I made a quick attempt at this here:
https://github.com/fredludlow/ngl/tree/pr-737

You can add this in npm as a git dependency if you want to try it:
npm install -D fredludlow/ngl#pr-737
OR
npm install -D fredludlow/ngl#82feda5 (the exact commit at time of writing)

Very minimal usage example here:
https://github.com/fredludlow/ngl-module-test

If people are in favour of this approach there are a few TODOs

• rollup.config.js needs a tidy
• Naming of the built files in dist (and updating package.main and package.module)
• Checking this doesn't break 101 other things :) (like typescript in downstream projects!)

(If people aren't in favour please speak up - I'm by no means an expert on this)

[garyo]

This certainly seems like a good idea to me! (Though it would; I'm definitely a "downstream project".) I'm also 100% typescript (currently relying on "npm run dts" before the build) so I can check that at least. I'm using es6 import everywhere so I'll be able to test the module build.

[giagitom]

In my situation I need to access both ngl and three in my web app. Now.. if i can use the same instance of three for both ngl and my needs it would be nice i think.
Which method should be better for my needs?

[fredludlow]

@giagitom: How are you building your app at the moment - is it a javascript project with a package.json? If so you would be able to add ngl (and possibly three*) as deps and either use require('ngl') or import { Something } from 'ngl' depending on whether you're using node-style or ES6 imports.

My change would mean that adding ngl as a dep automatically brings in a compatible version of three - you could be explicit about which version of three you want by adding it as a dep and I *think npm will try to use the same copy if versions are compatible, so your code would then be:

import { Stage } from 'ngl'   // Or var ngl = require('ngl')
import { Stuff } from 'three' // Or var three = require('three')

and they'd share a three instance (unless you specify an incompatible version)

You could test by replacing/adding the ngl dependency in your project as "fredludlow/ngl#pr-737"

@garyo - looking forward to hear if it works or not :)

We'd also need to coordinate with nglview (cc'ing @hainm). It looks like nglview uses ES6 imports and webpack, the end result should be the same but when developing nglview npm would install three separately and webpack should bundle it in (if I understand it right) - this is definitely something that would need to work in order to proceed.

[giagitom]

@fredludlow and @garyo thanks for the clarifications.

[garyo]

@fredludlow -- just tried your pr-737 branch. I just cloned it (I'm using ngl as a git submodule in a yarn monorepo), reset my yarn and node_modules, built everything, and it works! At least for basic stuff.

I had to make one small fix, and I need typescript 3.8.3, so here's my patch to your PR:

% git diff package.json
diff --git a/package.json b/package.json
old mode 100644
new mode 100755
index eb9dea8d..16fc49c0
--- a/package.json
+++ b/package.json
@@ -26,7 +26,7 @@
     "deploy": "./scripts/deploy.sh prerelease",
     "gallery": "./scripts/gallery.sh",
     "prerelease": "./scripts/release.sh prerelease",
-    "min": "uglifyjs build/js/ngl.dev.js -cm > dist/ngl.js && cpy build/js/ngl.esm.js dist && cpy build/js/ngl.umd.js dist",
+    "min": "uglifyjs build/js/ngl/ngl.dev.js -cm > dist/ngl.js && cpy build/js/ngl.esm.js dist && cpy build/js/ngl.umd.js dist",
     "build-min": "npm run build && npm run min"
   },
   "jest": {
@@ -95,7 +95,7 @@
     "standard": "^11.0.1",
     "ts-jest": "^25.2.1",
     "typedoc": "^0.16.10",
-    "typescript": "^3.7.3",
+    "typescript": "^3.8.3",
     "uglify-js": "^3.4.6"
   },
   "dependencies": {

[fredludlow]

Thanks for the test and patch @garyo, and apologies for delay (mumble, mumble, lockdown, childcare :) ) - The first of those changes reflects a typo of mine (I inadvertently changed where ngl.dev.js gets put) - I've reverted that in a new commit.

With more recent TS I'm now seeing some typescript errors. One is a quick fix in molecularsurface-representation, the other is multiple versions of this:

src/buffer/tetrahedron-buffer.ts:9:16 - error TS2300: Duplicate identifier 'TetrahedronBufferGeometry'.

9   export class TetrahedronBufferGeometry extends BufferGeometry {
                 ~~~~~~~~~~~~~~~~~~~~~~~~~

  node_modules/three/src/geometries/TetrahedronGeometry.d.ts:3:14
    3 export class TetrahedronBufferGeometry extends PolyhedronBufferGeometry {
                   ~~~~~~~~~~~~~~~~~~~~~~~~~
    'TetrahedronBufferGeometry' was also declared here.

NGL's src/buffer/tetrahedron-buffer.ts explicitly declares them like this:

declare module 'three' {
  export class TetrahedronBufferGeometry extends BufferGeometry {
    constructor(radius: number, detail: number);
  }
}

I'm guessing this was because three didn't declare them at the time?? Anyway - removing them seems to get the build working and I can't see any problems with e.g. the octahedron example.

I've pushed what seems to be a working version to my pr-737 branch. I still need to figure out how to check nglview isn't affected.

[garyo]

The TetrahedronBufferGeometry fix makes sense to me. At some point a long time ago, that wasn't in core Three.js, it was in examples/.
As for building & testing nglview, I'd also like to know how to do that "properly". And we should have an example of a simple project that loads ngl using esm.

[garyo]

Ah yes, I'm seeing these too now (upgraded to tsc 3.8.3). I also had to remove IcosahedronBufferGeometry and OctahedronBufferGeometry and then remove the unused BufferGeometry imports.
What was your fix in molecularsurface-representation.ts? I got this but not sure it's the best fix:

diff --git a/src/representation/molecularsurface-representation.ts b/src/representation/molecularsurface-representation.ts
index 3fd3ef14..252d44bc 100644
--- a/src/representation/molecularsurface-representation.ts
+++ b/src/representation/molecularsurface-representation.ts
@@ -195,7 +195,7 @@ class MolecularSurfaceRepresentation extends StructureRepresentation {
       if (this.useWorker) {
         info.molsurf.getSurfaceWorker(p as MolecularSurfaceParameters, onSurfaceFinish)
       } else {
-        onSurfaceFinish(info.molsurf.getSurface(p as {name: string, type: string} & MolecularSurfaceRepresentationParameters))
+        onSurfaceFinish(info.molsurf.getSurface(p as {name: string, type?: "av"|"edt"} & MolecularSurfaceRepresentationParameters))
       }
     } else {
       callback(i)

[fredludlow]

My fix was much as yours (https://github.com/fredludlow/ngl/commit/f3ef291032504a431ea84f095aecc7cdeb939b77#diff-57224d99c3f7d45231a0a25040b4ab46)

though I didn't let type be optional.

[garyo]

I only added that because my IDE said it could be undefined. Sounds good though.

[fredludlow]

Your IDE may well know more about this than I do...

I've just submitted an issue to nglview and if that doesn't raise any flags hopefully we can merge this soon.

[jussty]

Hey @fredludlow, unfortunately the original issue is still not solved. Even though it's now easier to include three, ngl still doesn't work with three r112 required for WebXR support. Please use my rebased commit https://github.com/jussty/ngl/commit/0d1d8dc00a0e41a42c9043d86af0f8d11792eda0 to see the problem.
I believe the issue should be reopened.

[fredludlow]

Right, can confirm r112 (edited: fix typo) doesn't work. Checking out jussty/ngl@0d1d8dc I see

three.module.js:20557 WebGL: INVALID_OPERATION: useProgram: program not valid

from call to gl.useProgram, and also

[.WebGL-00000166659E4020] GL_INVALID_OPERATION: Feedback loop formed between Framebuffer and active Texture.

Any ideas? Quick google suggests feedback loops can be incorrectly identified when you read from a buffer - maybe something to do with picking? @arose - don't suppose this rings any bells if you've had to deal with something similar in molstar?

[fredludlow]

Update: The 'line', 'point' and 'trace' representations don't trigger the error

[garyo]

Three.js breaks things pretty regularly. I find it's always best to check
out https://github.com/mrdoob/three.js/wiki/Migration-Guide and see what's
called out there.

-- Gary

On Thu, Jul 2, 2020 at 12:44 PM Fred Ludlow [e-mail]
wrote:

Update: The 'line', 'point' and 'trace' representations don't trigger the
error

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/arose/ngl/issues/728#issuecomment-653114005, or
unsubscribe
https://github.com/notifications/unsubscribe-auth/AABCFRZ46X4WKDEY3QH22JTRZS2QBANCNFSM4MA37TQA
.

--
Gary Oberbrunner
Founder & CEO
Dark Star Systems, Inc https://darkstarsystems.com.