jupyter-widgets / pythreejs

A Jupyter - Three.js bridge
https://pythreejs.readthedocs.io
Other
951 stars 188 forks source link

making threejs external for mixing pythreejs with other libraries #109

Closed maartenbreddels closed 7 years ago

maartenbreddels commented 7 years ago

cc @martinal In order for all threejs based libraries (pythreejs, ipyvolume, and unray?) to work together, they need to share the same threejs library. While for jupyter lab that isn't a problem, for the notebook it is. I propose the following solution:

Any show stoppers to do it this way?

vidartf commented 7 years ago

I agree this should be sorted but I just want to check: Are there any complications related to three.js being loaded into window.THREE? I know some of the three.js examples require THREE to be available like that, e.g. the OrbitControls.

maartenbreddels commented 7 years ago

We could modify all of these example to behave properly, since it may happen that a different library will have it's private version of threejs, that doesn't do this. So if we go this route, I think we shouldn't keep THREE / THREEx a global.

jasongrout commented 7 years ago

I think we shouldn't keep THREE / THREEx a global.

+1, if possible. I'm dealing with this issue right now regarding jquery and jqueryui, global objects, and objects provided by various extensions.

maartenbreddels commented 7 years ago

First steps are taken in #110, but embedding needs to wait till https://github.com/jupyter-widgets/ipywidgets/issues/1551 gets resolved. I have no global THREE variable anymore, and all the THREEx are 'private' modules, and are not exported (and there is no real need for this anyway I guess).

maartenbreddels commented 7 years ago

I'm not sure this is needed actually, threejs seems to have moved away from using instanceof https://github.com/mrdoob/three.js/issues/4776#issuecomment-55868878 but i'm not 100%.

vidartf commented 7 years ago

The code still uses instanceof several places (just from my memory of digging in the code).

vidartf commented 7 years ago

On further inspection three actually does not use instanceof, but the example code did. I've fixed this in the autogen branch (I incorporated your PR #110, but the history got lost in the rebasing 😞 ).

maartenbreddels commented 7 years ago

It will also make embedding a bit more difficult btw, to have a separate three.js. It needs to fetch it somewhere.

vidartf commented 7 years ago

With #88 merged, this should now be in master. I'm going to close this for now, but feel free to reopen (or open another issue) if there are some issues with that implementation.