Upgrade to latest vtk-js in dicom_viewer

[zachmullen]

This supersedes #1950

[MayeulChassagnard]

+2

[MayeulChassagnard]

I might have been too fast...

Since it was bulding correctly I forgot to verify the plugin dicom_viewer was correctly working with the latest vtk-js version.

It appears that it's not the case:

This error appears from vtk.js version 2.13.5 and above.

v2.13.4 works fine, should we merge immediately a commit with this lower version the time we fix the issue?

[zachmullen]

Hm, is this a regression in vtk-js? We may want to open a bug against vtk.js -- I think they use semver, so breakage is not expected here, unless we are depending on undefined behavior.

[MayeulChassagnard]

We should ask that to the experts I think
@fogleman @jcfr @mgrauer @manthey

[mgrauer]

@jourdain

[zachmullen]

@martinken

[jourdain]

2.13.5 is Rendering: support intermixing of surfaces and volumes

Maybe the ImageMapper that you did don't play nice with it and since there is no test on that mapper we did not notice it?

[zachmullen]

@jourdain I'm getting a warning message in the console that says "empty event loop", does that indicate a problem?

[zachmullen]

Also when dragging the mouse:

Uncaught TypeError: publicAPI.superHandleMouseMove is not a function
    at Object.publicAPI.handleAnimation (http://localhost:8080/static/built/plugins/dicom_viewer/plugin.min.js:38098:15)
    at Object.<anonymous> (http://localhost:8080/static/built/plugins/dicom_viewer/plugin.min.js:39450:44)
    at http://localhost:8080/static/built/plugins/dicom_viewer/plugin.min.js:2555:35
    at Array.forEach (native)
    at Object.publicAPI.(anonymous function) [as invokeAnimation] (http://localhost:8080/static/built/plugins/dicom_viewer/plugin.min.js:2554:15)
    at Object.publicAPI.animationEvent (http://localhost:8080/static/built/plugins/dicom_viewer/plugin.min.js:40709:17)
    at publicAPI.handleAnimation (http://localhost:8080/static/built/plugins/dicom_viewer/plugin.min.js:40541:15)
publicAPI.handleAnimation @ index.js:83
(anonymous) @ index.js:81
(anonymous) @ macro.js:577
publicAPI.(anonymous function) @ macro.js:577
publicAPI.animationEvent @ index.js:424
publicAPI.handleAnimation @ index.js:255

[martinken]

The missing image is a regression. We have an example for the class but not a test. I'll add one tomorrow. For now this MR should fix the issue. Kitware/vtk-js#212

[jourdain]

I don't think so.

Although the interaction has also been updated to use the requestAnimationFrame which could be the source of the issue here.

[MayeulChassagnard]

@jourdain Should I create a vtk-js or a girder issue for this dragging error then?

[jourdain]

The code seems to be in vtk.js

[martinken]

Should be fixed now in master via Kitware/vtk-js#215

[MayeulChassagnard]

@martinken Is there a tag to the latest vtk-js version (including your modifications)? I would like to give a try but the plugin.json in the dicom_viewer plugin requires a tag release version.

[MayeulChassagnard]

I gave a try with v2.18.3 and unfortunately we still have issues:

empty event loop  plugin.min.js:1:20810
Error: WebGL: generateMipmap: Texture at base level is not unsized internal format or is not color-renderable or texture-filterable.  plugin.min.js:1:126022
Error: WebGL: drawArrays: Active texture 0 for target 0x0de1 is 'incomplete', and will be rendered as RGBA(0,0,0,1), as per the GLES 2.0.24 $3.8.2: Because the minification filter requires mipmapping, the texture must be "mipmap complete".  plugin.min.js:1:620154

The dragging mouse seems to work but the image is still missing.
Should I open an issue in vtk-js for this?

[martinken]

You can, we may need an example for the issue. The example we have

https://kitware.github.io/vtk-js/examples/ImageMapper.html

doesn't seem to show that issue.

[martinken]

I believe I know what the issue is, just may not show up on every platform. I'll try to get in a fix this morning.

[MayeulChassagnard]

All right, let me know if you need anything!

Thanks a lot

[martinken]

give 2.18.4 a shot

[MayeulChassagnard]

nope, doesn't fix it :/
I still got the same issue:

[martinken]

Can you try

Kitware/vtk-js#217

[zachmullen]

@martinken FWIW, 2.18.4 fixed both bugs for me

[martinken]

Thanks, I think what Mayeul is seeing is hardware specific. I think I have a fix ready though that shoudl solve the issue for those that see it.

[MayeulChassagnard]

I tried to use your commit by modifying plugin.json as follow :

{
    "name": "DICOM Viewer",
    "description": "View DICOM images in the browser",
    "url": "http://girder.readthedocs.io/en/latest/plugins.html#dicom-viewer",
    "version": "0.1.0",
    "npm": {
        "dependencies": {
            "daikon": "^1.2.15",
            "kw-web-suite": "^2.0.1",
            "vtk.js": "git://github.com/Kitware/vtk-js.git#568abd097aeea910f5357732b2ebd658adeb8807"
        }
    }
}

During girder-install web I can see:

vtk.js@0.0.0-semantically-release  (git://github.com/Kitware/vtk-js.git#568abd097aeea910f5357732b2ebd658adeb8807)

Is your commit correctly used by doing this?

If yes, this commit solved the issue!

[zachmullen]

If you do cat node_modules/vtk.js/package.json, what is the value in the version field?

[MayeulChassagnard]

0.0.0-semantically-release

[zachmullen]

Ah, that makes sense, I think they use a tool that changes that value automatically at release time.

[jbeezley]

You can try npm ls vtk.js to get the installed version.

[MayeulChassagnard]

I get

girder@2.2.0 /home/mayeul/MyProjects/girder
└── vtk.js@0.0.0-semantically-release  extraneous (git://github.com/Kitware/vtk-js.git#568abd097aeea910f5357732b2ebd658adeb8807)

npm ERR! extraneous: vtk.js@0.0.0-semantically-release /home/mayeul/MyProjects/girder/node_modules/vtk.js

[martinken]

I merged the change, so the system should produce a new version soon.

[MayeulChassagnard]

Okay, I just checked out the file if it was correctly modified according to Kitware/vtk-js@568abd0 after modifying plugin.json with "vtk.js": "git://github.com/Kitware/vtk-js.git#568abd097aeea910f5357732b2ebd658adeb8807".

So, @martinken, we are good to merge Kitware/vtk-js#217 , it fixed the issue!

Thank you very much for your help guys :)

[martinken]

2.18.5

[martinken]

Awesome, glad to hear it! Thanks for testing it out.