Three.js: Upgrade to r122

[paulmasson]

CC: @jcamp0x2a @kiwifb @mkoeppe @EmmanuelCharpentier @enriqueartal

Component: graphics

Keywords: threejs

Author: Paul Masson, Joshua Campbell

Branch/Commit: c097439

Reviewer: Joshua Campbell, Paul Masson, Matthias Koeppe

Issue created by migration from https://trac.sagemath.org/ticket/30915

[paulmasson]

Branch: u/paulmasson/threejs-r122

[paulmasson]

comment:2

Joshua, should only need to do make build to test this. Downloading the file should be automatic.

---

New commits:

9ad4bdc

Upgrade to r122

[paulmasson]

Commit: 9ad4bdc

[jcamp0x2a]

comment:5

make build appears to have downloaded and extracted the files successfully. All the right stuff (OrbitControls and the "fat" line classes) seem to appear in the three.min.js file extracted into local/share/threejs/build and running a diff on it against the script I built from your repo produces no output, so it looks good.

Of course, the HTML plots produced are still looking for OrbitControls.js and/or directing to mrdoob's repo on the CDN, but that's going to be a separate ticket?

[paulmasson]

comment:6

Replying to @jcamp0x2a:

Of course, the HTML plots produced are still looking for OrbitControls.js and/or directing to mrdoob's repo on the CDN, but that's going to be a separate ticket?

No, I overlooked that before meeting a friend for dinner. Will fix tomorrow, but my friend pointed out another development: Mr.doob reversed himself and won’t be deprecating examples/js this year. I think I’m still in favor of this custom build rather than assembling a tarball because the entire process is smoother and the transition will happen at some point anyway. Feedback is welcome.

[jcamp0x2a]

comment:7

Replying to @paulmasson:

I think I’m still in favor of this custom build rather than assembling a tarball because the entire process is smoother and the transition will happen at some point anyway. Feedback is welcome.

I would agree. It also allows everything to be bundled up into a single javascript file instead of having to include OrbitControls and now all the line stuff separately. So I think it's a positive change even if examples/js were never deprecated.

[sagetrac-git]

Changed commit from 9ad4bdc to 63530d2

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

63530d2

Update script link generation

[paulmasson]

comment:10

The new commit updates the link generated internally, both for online and offline use. I changed the name of the internal function to recognize this. Doctests pass on the two modified files.

Matthias, part of the reason I wanted the repository moved to the SageMath organization is that the online links must point to my personal repository on the CDN. If someone creates notebooks with these changes in place for online use, they may fail to work in the future when the repository is moved. If you're fine with that possibility, then we can procede with the upgrade.

Joshua, before testing this ticket again let's hear back from Matthais.

[mkoeppe]

comment:11

Makes sense. Am I understanding correctly that jsdelivr.net refers to specific tags here, so when you update the repository, the old versions stick around and can continue to be referred to by plots that were generated earlier?

[paulmasson]

comment:12

Replying to @mkoeppe:

Makes sense. Am I understanding correctly that jsdelivr.net refers to specific tags here, so when you update the repository, the old versions stick around and can continue to be referred to by plots that were generated earlier?

Once a file is in the CDN it is always available for future use and is indeed referenced by tag. According to the documentation the files remain even when a repository is deleted, so presumably it can handle movement of the repository between organizations. If you want us to proceed with the current setup then we can do that without too much worry after all.

[mkoeppe]

comment:13

Thanks. And for the offline mode - does it make sense to make several versions available in SAGE_LOCAL?

[mkoeppe]

comment:14

Also, does the offline mode depend at all on the symbolic link threejs that is installed in $SAGE_LOCAL/share/jupyter/nbextensions?

(My guess is that it does not and we can remove this symbolic link. At least that's what I gather from a comment in #30476 - it seems that threejs offline graphics also works with system jupyter notebook, which does not have that symbolic link.)

[paulmasson]

comment:15

Replying to @mkoeppe:

Thanks. And for the offline mode - does it make sense to make several versions available in SAGE_LOCAL?

No, it does not. The Three.js template is specifically tailored to the release currently in use. Mr. Doob sometimes breaks things. Backwards compatibility is not assured.

[paulmasson]

comment:16

Replying to @mkoeppe:

Also, does the offline mode depend at all on the symbolic link threejs that is installed in $SAGE_LOCAL/share/jupyter/nbextensions?

The system Jupyter notebook needs to find the local files for our Three.js viewer to work. As far as I know the symbolic link is how that happens. If you have a better idea in mind for letting the notebook know where the files are, then go for it. I'm still not clear on this question.

[mkoeppe]

comment:17

Replying to @paulmasson:

Replying to @mkoeppe:

Also, does the offline mode depend at all on the symbolic link threejs that is installed in $SAGE_LOCAL/share/jupyter/nbextensions?

The system Jupyter notebook needs to find the local files for our Three.js viewer to work. As far as I know the symbolic link is how that happens. If you have a better idea in mind for letting the notebook know where the files are, then go for it. I'm still not clear on this question.

This seems to contradict what @EmmanuelCharpentier reported in #30476 regarding offline threejs in the system jupyter notebook.
@EmmanuelCharpentier, @enriqueartal, could you help clarify this?

[jcamp0x2a]

comment:18

Replying to @paulmasson:

Replying to @mkoeppe:

Thanks. And for the offline mode - does it make sense to make several versions available in SAGE_LOCAL?

No, it does not. The Three.js template is specifically tailored to the release currently in use. Mr. Doob sometimes breaks things. Backwards compatibility is not assured.

Hmm... perhaps it would be better to link the offline plots to a specific version.

For example, suppose I generate an offline plot using the template that works for r117. The plot links to /home/joshua/sage/local/share/threejs/build/three.min.js. Some time in the future I upgrade SageMath in-place. (Do normal users do that?) That old plot is now linking to three.js r122 despite the template it was generated from only being guaranteed to work with r117. Suddenly the plot may no longer work.

That's not an issue for the Jupyter notebook since it's a "live" environment and you can always just re-run the cell to get an up-to-date plot. It's also not an issue for the CDN scripts since those are tied to specific versions.

Although even if we fixed this, it'd still be fragile in that moving/deleting SageMath would also break old plots. To make a fully self-contained and isolated offline plot would probably require including the content of three.min.js inline instead of linking, but I imagine that might run afoul of the license.

[mkoeppe]

comment:19

In any case, so that we can make progress here, I have forked the repo at https://github.com/sagemath/threejs-sage

[mkoeppe]

comment:20

Replying to @jcamp0x2a:

Replying to @paulmasson:

Replying to @mkoeppe:

Thanks. And for the offline mode - does it make sense to make several versions available in SAGE_LOCAL?

No, it does not. The Three.js template is specifically tailored to the release currently in use. Mr. Doob sometimes breaks things. Backwards compatibility is not assured.

Hmm... perhaps it would be better to link the offline plots to a specific version.

For example, suppose I generate an offline plot using the template that works for r117. The plot links to /home/joshua/sage/local/share/threejs/build/three.min.js. Some time in the future I upgrade SageMath in-place. (Do normal users do that?)

Yes, that's a typical operation.

That old plot is now linking to three.js r122 despite the template it was generated from only being guaranteed to work with r117. Suddenly the plot may no longer work.

Right. So let's please do a versioned installation scheme to address this issue. Basically, upgrades should not break operation of older things.

That's not an issue for the Jupyter notebook since it's a "live" environment and you can always just re-run the cell to get an up-to-date plot.

Well, but isn't it an important part of the point of the notebook format that the outputs are also stored?

It's also not an issue for the CDN scripts since those are tied to specific versions.

Although even if we fixed this, it'd still be fragile in that moving/deleting SageMath would also break old plots.

(Moving the Sage tree is not supported.)

[mkoeppe]

comment:21

Replying to @mkoeppe:

That old plot is now linking to three.js r122 despite the template it was generated from only being guaranteed to work with r117. Suddenly the plot may no longer work.

Right. So let's please do a versioned installation scheme to address this issue. Basically, upgrades should not break operation of older things.

Again the key use case to consider is to have one ("system") installation of the Jupyter notebook or JupyterLab, which the user uses to access multiple installations of different versions of the Sage kernel.

[mkoeppe]

comment:22

Replying to @jcamp0x2a:

To make a fully self-contained and isolated offline plot would probably require including the content of three.min.js inline instead of linking,

Sounds like this would be a useful option.

but I imagine that might run afoul of the license.

Does a more restrictive license than MIT apply? https://github.com/sagemath/threejs-sage/blob/main/LICENSE

[mkoeppe]

comment:47

Replying to @paulmasson:

Are you planning on eliminating src/sage/ext_data?

No. The issue is about share/jupyter/nbextensions/threejs and the fact that there will be several unrelated copies of that.

Here is an example.

• Let's say system jupyter is installed in /usr, so the notebook server is accessing /usr/share/jupyter/nbextensions.
• Sage-9.x may be installed in /home/x/sage-9.x/local/ and may need threejs r122.
• Sage-9.y may be installed in /home/x/sage-9.y/local/ and may need threejs r155.
• Let's say r122 and r155 are mutually incompatible.

As @enriqueartal has reported above, our offline threejs graphics needs /usr/share/jupyter/nbextensions/threejs/. But the system does not provide it -- it must come from Sage.
If we create a symlink /usr/share/jupyter/nbextensions/threejs -> /home/x/sage-9.x/local/share/jupyter/nbextensions/threejs, then Sage 9.x will work with the system jupyter notebook; but Sage 9.y will not.

Do you agree?

So this means that we need a versioned installation scheme.
The details of this installation scheme is what I am trying to discuss with you, in particular because it is certainly related to that new repository.

[jcamp0x2a]

Changed author from Paul Masson to Paul Masson, Joshua Campbell

[jcamp0x2a]

Changed commit from 55449d8 to eed14d3

[jcamp0x2a]

comment:48

Replying to @paulmasson:

Joshua, ready for another review. Thanks!

Hi Paul, I found a failing doctest in display_manager.py due to the CDN URL change. I hope you don't mind, but I took the liberty of pushing a fix for it. I also went ahead and modified it to read from that new version file instead of relying on the REVISION= stuff, and I also renamed a few more occurrences of "scripts" to "script" as you had done in your commit.

If you are okay with the additional changes, I think you or I could mark it as positive review.

[jcamp0x2a]

Changed branch from u/paulmasson/threejs-r122 to u/gh-jcamp0x2a/threejs-r122

[jcamp0x2a]

comment:49

Replying to @mkoeppe:

So this means that we need a versioned installation scheme.
The details of this installation scheme is what I am trying to discuss with you, in particular because it is certainly related to that new repository.

Including the three.min.js inline for offline use (whether jupyter or command line) seems like it would clear up this headache for three.js at least. No more local file paths.

I'd like to get this ticket merged as-is so I can make some headway on my "fat" lines branch. So perhaps a follow-up ticket?

[paulmasson]

comment:50

Replying to @mkoeppe:

Replying to @paulmasson:

Are you planning on eliminating src/sage/ext_data?

No. The issue is about share/jupyter/nbextensions/threejs and the fact that there will be several unrelated copies of that.

Here is an example.

• Let's say system jupyter is installed in /usr, so the notebook server is accessing /usr/share/jupyter/nbextensions.
• Sage-9.x may be installed in /home/x/sage-9.x/local/ and may need threejs r122.
• Sage-9.y may be installed in /home/x/sage-9.y/local/ and may need threejs r155.
• Let's say r122 and r155 are mutually incompatible.

As @enriqueartal has reported above, our offline threejs graphics needs /usr/share/jupyter/nbextensions/threejs/. But the system does not provide it -- it must come from Sage.
If we create a symlink /usr/share/jupyter/nbextensions/threejs -> /home/x/sage-9.x/local/share/jupyter/nbextensions/threejs, then Sage 9.x will work with the system jupyter notebook; but Sage 9.y will not.

Do you agree?

So this means that we need a versioned installation scheme.
The details of this installation scheme is what I am trying to discuss with you, in particular because it is certainly related to that new repository.

Moving discussion to #30972

[paulmasson]

comment:51

Replying to @jcamp0x2a:

Replying to @paulmasson:

Joshua, ready for another review. Thanks!

Hi Paul, I found a failing doctest in display_manager.py due to the CDN URL change. I hope you don't mind, but I took the liberty of pushing a fix for it. I also went ahead and modified it to read from that new version file instead of relying on the REVISION= stuff, and I also renamed a few more occurrences of "scripts" to "script" as you had done in your commit.

If you are okay with the additional changes, I think you or I could mark it as positive review.

LGTM

[paulmasson]

Reviewer: Joshua Campbell, Paul Masson

[mkoeppe]

comment:52

Public methods such as threejs_offline_scripts should not be renamed without deprecation

[paulmasson]

comment:53

Joshua, would you just revert the method name? It's only used internally and I no longer care if the script name doesn't match the output.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

c097439

Revert threejs_scripts method name changes

[sagetrac-git]

Changed commit from eed14d3 to c097439

[jcamp0x2a]

comment:55

Replying to @paulmasson:

Joshua, would you just revert the method name? It's only used internally and I no longer care if the script name doesn't match the output.

Done. Did a quick sanity check and it still runs without issue in all four combinations of command-line/jupyter and online/offline.

[mkoeppe]

Changed reviewer from Joshua Campbell, Paul Masson to Joshua Campbell, Paul Masson, Matthias Koeppe

[mkoeppe]

comment:57

Joshua, by the way, have you tested if the offline mode works with JupyterLab too?

[jcamp0x2a]

comment:58

Replying to @mkoeppe:

Joshua, by the way, have you tested if the offline mode works with JupyterLab too?

Yes, sorry, that's actually what I meant by "jupyter". I've been using it in preference to the traditional notebook lately. I've gone back and tested with the traditional notebook and it works fine as well.

Oh, and I'm going to try to spend some time later today looking at your #30972.

[mkoeppe]

comment:59

Replying to @jcamp0x2a:

Replying to @mkoeppe:

Joshua, by the way, have you tested if the offline mode works with JupyterLab too?

Yes, sorry, that's actually what I meant by "jupyter". I've been using it in preference to the traditional notebook lately. I've gone back and tested with the traditional notebook and it works fine as well.

Great, thanks, I was not sure whether the JupyterLab server also serves the /nbextensions path.

[vbraun]

Changed branch from u/gh-jcamp0x2a/threejs-r122 to c097439