Make Three.js work offline redux #22670

paulmasson · 2017-03-21T23:50:15Z

The Three.js scripts used by various backends for offline viewing should be generated by methods in the backends, not in plot3d/base.pyx.

Continuation of discussion after #22488 was closed.

CC: @novoselt @egourgoulhon

Component: graphics

Author: Paul Masson, Andrey Novoseltsev

Branch: 668eeee

Reviewer: Andrey Novoseltsev, Paul Masson

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

The text was updated successfully, but these errors were encountered:

novoselt · 2017-03-22T02:37:16Z

comment:2

Some more thoughts: we should avoid code duplication as much as feasible here so that we don't have to chase all places where the version of the threejs is used when we want to change it. Perhaps something like this will work:

generic method returns CDN scripts with online=True and throws an exception with online=False, these scripts include the version number
backend-specific implementation provide appropriate paths to Sage package (which does not include the version number), and if they have to include a fall-back CDN section, they do it by invoking the parent method with online=True and embed that string in appropriate places

This way when upgrading threejs package one has also to bump version number in the generic method (and perhaps it should be mentioned in SPKG.txt), but backend ones will likely never require tweaking.

paulmasson · 2017-03-31T22:38:36Z

Branch: u/paulmasson/22670

paulmasson · 2017-03-31T22:46:04Z

comment:4

Andrey, please have a look at this. I'm using the internal installed_packages table to read the version number where needed, so we'll only have to change it once in the package itself during upgrades. I didn't include an error test to make sure the package is installed since it's now standard and should always be present. The IPython notebook can't call the parent method as you'd like because that fallback is in the JavaScript, so it needs to look up the version as well, but then there will be no tweaking for upgrades.

There is currently a failing doctest for something called "BackendDoctest". I'll fix that once I know we're on the right track.

New commits:

`1d27bd0`	`Add methods for offline scripts`

paulmasson · 2017-03-31T22:46:04Z

Commit: 1d27bd0

novoselt · 2017-04-01T03:49:34Z

comment:5

Automatic version matching is great, thanks for figuring it out!

It still seems to me that in base.pyx it would be better to have just

scripts = backend.threejs_scripts(options['online'])

with the base backend class handling True case and more specific classes worrying about False. That way BackendIPythonNotebook could call parent method for extra bits instead of repeating the logic. Also, everything would be more in one place rather than the same thing split between plot and repl. But overall this design is already a nice improvement over current version.

novoselt · 2017-04-01T03:49:52Z

Reviewer: Andrey Novoseltsev

sagetrac-git · 2017-04-02T00:58:56Z

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

`5c1c232`	`Move all script generation to backend`

sagetrac-git · 2017-04-02T00:58:56Z

Changed commit from 1d27bd0 to 5c1c232

paulmasson · 2017-04-02T01:07:52Z

comment:8

Hopefully this is closer to what you had in mind.

I've included an error test in the base method, but this causes a doctest in plot3d/base.pyx to fail even with the keyword argument set explicitly. Do you know why the doctesting process ignores the keyword argument?

A workaround is to have the base method return an empty string when online=False but that's hardly desirable.

novoselt · 2017-04-02T18:45:52Z

Changed branch from u/paulmasson/22670 to u/novoselt/22670

novoselt · 2017-04-02T18:48:52Z

Changed commit from 5c1c232 to 6a4bdec

novoselt · 2017-04-02T18:48:52Z

Work Issues: option handling

novoselt · 2017-04-02T18:48:52Z

comment:10

Tweaked a bit backend code.

Regarding option ignoring - that's because you ignore them instead of doing opts = self._process_viewing_options(kwds) as other methods ;-) I'm looking at how things work and hope to post a solution shortly, will be useful for my understanding of option handling.

New commits:

`6a4bdec`	`Use a single copy of CDN threejs scripts`

sagetrac-git · 2017-04-02T19:33:09Z

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

`b73ce25`	`Pick up constructor options for threejs rendering`

sagetrac-git · 2017-04-02T19:33:09Z

Changed commit from 6a4bdec to b73ce25

novoselt · 2017-04-02T19:35:15Z

comment:12

Improved option handing, but now the error is

Failed example:
    sphere(online=True)._rich_repr_threejs()
Exception raised:
    Traceback (most recent call last):
      File "/home/novoselt/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 498, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/novoselt/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 861, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.plot.plot3d.base.Graphics3d._rich_repr_threejs[0]>", line 1, in <module>
        sphere(online=True)._rich_repr_threejs()
      File "sage/plot/plot3d/base.pyx", line 424, in sage.plot.plot3d.base.Graphics3d._rich_repr_threejs (build/cythonized/sage/plot/plot3d/base.c:10615)
        html = html.replace('SAGE_OPTIONS', json.dumps(options))
      File "/home/novoselt/sage/local/lib/python/json/__init__.py", line 244, in dumps
        return _default_encoder.encode(obj)
      File "/home/novoselt/sage/local/lib/python/json/encoder.py", line 207, in encode
        chunks = self.iterencode(o, _one_shot=True)
      File "/home/novoselt/sage/local/lib/python/json/encoder.py", line 270, in iterencode
        return _iterencode(o, 0)
      File "/home/novoselt/sage/local/lib/python/json/encoder.py", line 184, in default
        raise TypeError(repr(o) + " is not JSON serializable")
    TypeError: Texture(texture42, 6666ff) is not JSON serializable

which makes me wonder why these options are sent to the output at all?..

novoselt · 2017-04-02T19:38:30Z

comment:13

OK, looking at the template I see how they are used, but I am not sure what's the correct way to proceed: fix representation of texture option, drop if from threejs output, or send only options which are known to be used in JavaScript?

paulmasson · 2017-04-02T20:36:26Z

comment:14

I don't see how invoking _process_viewing_options(kwds) fixes anything: all it appears to do is add a bunch of options that won't be used in JavaScript, at least not yet. Where is online actually getting passed on to the doctest backend with your changes?

All the other changes look good. And FYI decimals is Three.js specific.

paulmasson · 2017-04-02T20:44:04Z

comment:15

FYI the options explicitly set by me are the only ones used in the template, which is why I just created my own dictionary.

novoselt · 2017-04-02T20:45:20Z

comment:16

online=True passed to the constructor is saved in the object, _process_viewing_options assembles default options, options passed to constructor, and options passed directly in kwds, it also does some normalization of them, so I am pretty sure it is "the right thing" to call it here as well. I've tried to mark decimals as Three.js specific - they are set there to a default if not passed via other means and then another section is dealing with type conversion.

novoselt · 2017-04-02T20:47:40Z

comment:17

Replying to @paulmasson:

FYI the options explicitly set by me are the only ones used in the template, which is why I just created my own dictionary.

But you are using some of the options that are "standard" like axes and the problem was that it was not possible to pass threejs-specific options via standard means. So I am still convinced that my changes are on the right track. And it seems to me now that the next step is to keep only whitelisted options in this dictionary before sending them to template.

paulmasson · 2017-04-02T20:57:50Z

comment:18

Replying to @novoselt:

Replying to @paulmasson:

FYI the options explicitly set by me are the only ones used in the template, which is why I just created my own dictionary.

But you are using some of the options that are "standard" like axes and the problem was that it was not possible to pass threejs-specific options via standard means. So I am still convinced that my changes are on the right track. And it seems to me now that the next step is to keep only whitelisted options in this dictionary before sending them to template.

If it gets things to work then good, but I'm not yet seeing the standard means. Sorry about the decimals mix up: was editing my statement when you responded. And one correction: online isn't used in the template so doesn't need to be whitelisted.

novoselt · 2017-04-02T22:16:19Z

Changed work issues from option handling to none

novoselt · 2017-04-02T22:18:29Z

comment:24

Regarding CDN releases - in principle the version matters, although the problems are presumably more frequent when using old instead of new. So we may try to omit the version and use the master branch of threejs.

paulmasson · 2017-04-02T23:46:27Z

comment:25

Replying to @novoselt:

Regarding CDN releases - in principle the version matters, although the problems are presumably more frequent when using old instead of new. So we may try to omit the version and use the master branch of threejs.

With Three.js the version number can matter greatly, unfortunately. Mr. Doob has made radical changes in the past that render the code not backward compatible. That doesn't happen too often but it will be an ongoing concern.

novoselt · 2017-04-02T23:51:42Z

comment:26

OK, possible solutions:

Have hard-coded version number and add some comment to SPKG.txt as I originally proposed.
Pick up the version number from the library file itself, presumably this is possible.
Put an extra file together with the library into share/threejs
=2+3: Pick up the version number from the library file and then write it into some file for speed. Or just some variable to avoid disk operations and access issues...

kiwifb · 2017-04-02T23:59:18Z

comment:27

Options 2,3 and 4 rely on theejs to be installed locally, a distro may very decide to make that optional (if at all - properly packaging this kind of stuff is a pain) and to rely on online availability by default.

Alternative to (1) declare a variable in sage.env.py it could be migrated to a separate file installed by the spkg once we work out #22652. That method would be ok to me as I can supply an arbitrary extra file for my distro.

paulmasson · 2017-04-03T00:09:14Z

comment:28

Replying to @novoselt:

It still bugged me that _backend of display manager was accessed directly

Why is this a bad thing to do?

It is somewhat ugly that they call that CDN method when handling offline case, but it is kind of due to things not done completely properly.

Only the IPython notebook does this, to provide a fallback requested by Eric. How would you do this more properly?

Why SageNB is using CDN for offline case? Doesn't it have access to Sage installation which includes three.js?

The server directory to Three.js files doesn't exist in the current version of the notebook. A pull request was merged to provide that and the method will be altered when a newer version of the notebook is available.

Couple minor things:

In display manager documentation, "Three.js" should be capitalized and "offline" is one word.
I may want to reuse the options dictionary for handling future features, like setting iframe size from figsize, so it shouldn't be completely overwritten. Please introduce a different name on the next commit.

paulmasson · 2017-04-03T00:23:27Z

comment:29

Andrey, scenes render for all three backends and both on/offline, but you've introduced a problem with IPython offline. The closing script tags need to be written as <\/script> with the forward slash escaped to prevent the remainer of that line of code, the ');, from printing in the cell above the scene. Please do a string replace before returning.

novoselt · 2017-04-03T00:53:33Z

comment:30

Replying to @kiwifb:

Options 2,3 and 4 rely on theejs to be installed locally, a distro may very decide to make that optional (if at all - properly packaging this kind of stuff is a pain) and to rely on online availability by default.

Alternative to (1) declare a variable in sage.env.py it could be migrated to a separate file installed by the spkg once we work out #22652. That method would be ok to me as I can supply an arbitrary extra file for my distro.

Our three.js package contrains only two files that we use instead of the whole repository, so it does not seem a big deal to me to keep it installed as part of Sage. In any case I think that for now (i.e. this ticket) we should keep it as is and modify to something else once (and if) it becomes a problem.

kiwifb · 2017-04-03T01:05:38Z

comment:31

For sage on its own, in its corner playing all alone, by itself, there is no problem. There is never a problem, you are self contained. I don't suggest you change the status standard package for threejs either. I am encouraging you to think of a bigger picture regarding package management when you are not on your little island.

It will be a problem for anyone doing sage-on-distro/conda as soon as it is merged. I probably should publicise this ticket on the sage-packaging mailing list and see what everyone think of the threejs business as it is now, since the current situation is hardly ideal either.

sagetrac-git · 2017-04-03T01:08:01Z

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

`8d1b155`	`Little fixes to threejs scripts handling`

sagetrac-git · 2017-04-03T01:08:01Z

Changed commit from e81038e to 8d1b155

novoselt · 2017-04-03T01:09:39Z

comment:33

Replying to @paulmasson:

Replying to @novoselt:

It still bugged me that _backend of display manager was accessed directly

Why is this a bad thing to do?

Because backends are supposed to be isolated! It was really, really painful to have a lot of places spread over Sage that were doing different things depending on the way it was launched and I would prefer none of them coming back ;-)

It is somewhat ugly that they call that CDN method when handling offline case, but it is kind of due to things not done completely properly.

Only the IPython notebook does this, to provide a fallback requested by Eric. How would you do this more properly?

No how, it is just a bit unfortunate that this problem arises in the first place.

Why SageNB is using CDN for offline case? Doesn't it have access to Sage installation which includes three.js?

The server directory to Three.js files doesn't exist in the current version of the notebook. A pull request was merged to provide that and the method will be altered when a newer version of the notebook is available.

Great!

Couple minor things:

In display manager documentation, "Three.js" should be capitalized and "offline" is one word.

Fixed.

I may want to reuse the options dictionary for handling future features, like setting iframe size from figsize, so it shouldn't be completely overwritten. Please introduce a different name on the next commit.

That's why I put this code right next to template filling, but different name works just as well, done.

novoselt · 2017-04-03T01:23:45Z

comment:34

Replying to @kiwifb:

For sage on its own, in its corner playing all alone, by itself, there is no problem. There is never a problem, you are self contained. I don't suggest you change the status standard package for threejs either. I am encouraging you to think of a bigger picture regarding package management when you are not on your little island.

It will be a problem for anyone doing sage-on-distro/conda as soon as it is merged. I probably should publicise this ticket on the sage-packaging mailing list and see what everyone think of the threejs business as it is now, since the current situation is hardly ideal either.

I am not trying to stay put on my island and I am all for isolation etc. But if use of sage.misc.package is prohibited, then perhaps it should not be exposed and look like an accessible functionality. How should we know that you disapprove of it? Please improve the current non-ideal situation, but if depends on a new ticket I would like to merge this one for the moment since it does improve threejs situation and we hope to make it default viewer in 8.0.

kiwifb · 2017-04-03T01:38:04Z

comment:35

Replying to @novoselt:

Replying to @kiwifb:

For sage on its own, in its corner playing all alone, by itself, there is no problem. There is never a problem, you are self contained. I don't suggest you change the status standard package for threejs either. I am encouraging you to think of a bigger picture regarding package management when you are not on your little island.

It will be a problem for anyone doing sage-on-distro/conda as soon as it is merged. I probably should publicise this ticket on the sage-packaging mailing list and see what everyone think of the threejs business as it is now, since the current situation is hardly ideal either.

I am not trying to stay put on my island and I am all for isolation etc. But if use of sage.misc.package is prohibited, then perhaps it should not be exposed and look like an accessible functionality. How should we know that you disapprove of it? Please improve the current non-ideal situation, but if depends on a new ticket I would like to merge this one for the moment since it does improve threejs situation and we hope to make it default viewer in 8.0.

Yes, you are right there should be a higher level policy on this. sage.misc.package is an artefact of sage being its own packaging system and it has unwelcome tentacles all over the place. I am trying to convince people not create any more tentacles - like I killed most uses of SAGE_ROOT before. I shall bring it on sage-devel.

paulmasson · 2017-04-03T02:05:49Z

comment:36

Documentation won't build. Looks like some extra whitespace somewhere:

[repl     ] /Users/Masson/Downloads/GitHub/sage/local/lib/python2.7/site-packages/sage/repl/rich_output/backend_ipython.py:docstring of sage.repl.rich_output.backend_ipython.BackendIPythonNotebook.threejs_offline_scripts:13: WARNING: Block quote ends without a blank line; unexpected unindent.
[repl     ] /Users/Masson/Downloads/GitHub/sage/local/lib/python2.7/site-packages/sage/repl/rich_output/display_manager.py:docstring of sage.repl.rich_output.display_manager.DisplayManager.threejs_scripts:22: WARNING: Block quote ends without a blank line; unexpected unindent.
Error building the documentation.

paulmasson · 2017-04-03T21:16:17Z

Changed branch from u/novoselt/22670 to u/paulmasson/22670

paulmasson · 2017-04-03T21:20:58Z

comment:38

Figured out it was the \n in two doctests. Replaced with ... and documentation now builds and doctests still pass.

Setting to positive review unless someone else wants to test it further.

New commits:

`668eeee`	`Allow documentation to build`

paulmasson · 2017-04-03T21:20:58Z

Changed reviewer from Andrey Novoseltsev to Andrey Novoseltsev, Paul Masson

paulmasson · 2017-04-03T21:20:58Z

Changed author from Paul Masson to Paul Masson, Andrey Novoseltsev

paulmasson · 2017-04-03T21:20:58Z

Changed commit from 8d1b155 to 668eeee

tscrim · 2017-04-03T21:24:10Z

comment:39

I suspect it might have been fixed if you made the string raw, i.e., use r""".

vbraun · 2017-04-07T22:24:02Z

Changed branch from u/paulmasson/22670 to 668eeee

jdemeyer · 2018-06-26T11:55:41Z

Changed commit from 668eeee to none

jdemeyer · 2018-06-26T11:55:41Z

comment:41

See #25665 for a follow-up

paulmasson mannequin added this to the sage-8.0 milestone Mar 21, 2017

paulmasson mannequin added c: graphics labels Mar 21, 2017

This comment has been minimized.

Sign in to view

paulmasson mannequin added the t: bug label Mar 22, 2017

paulmasson mannequin added the s: needs review label Mar 31, 2017

novoselt added s: needs work and removed s: needs review labels Apr 2, 2017

novoselt added s: needs review and removed s: needs work labels Apr 2, 2017

paulmasson mannequin added s: positive review and removed s: needs review labels Apr 3, 2017

vbraun removed the s: positive review label Apr 7, 2017

vbraun closed this as completed in e399a53 Apr 7, 2017

Make Three.js work offline redux #22670

Make Three.js work offline redux #22670

Comments

paulmasson mannequin commented Mar 21, 2017

This comment has been minimized.

novoselt commented Mar 22, 2017

paulmasson mannequin commented Mar 31, 2017

paulmasson mannequin commented Mar 31, 2017

paulmasson mannequin commented Mar 31, 2017

novoselt commented Apr 1, 2017

novoselt commented Apr 1, 2017

sagetrac-git mannequin commented Apr 2, 2017

sagetrac-git mannequin commented Apr 2, 2017

paulmasson mannequin commented Apr 2, 2017

novoselt commented Apr 2, 2017

novoselt commented Apr 2, 2017

novoselt commented Apr 2, 2017

novoselt commented Apr 2, 2017

sagetrac-git mannequin commented Apr 2, 2017

sagetrac-git mannequin commented Apr 2, 2017

novoselt commented Apr 2, 2017

novoselt commented Apr 2, 2017

paulmasson mannequin commented Apr 2, 2017

paulmasson mannequin commented Apr 2, 2017

novoselt commented Apr 2, 2017

novoselt commented Apr 2, 2017

paulmasson mannequin commented Apr 2, 2017

novoselt commented Apr 2, 2017

novoselt commented Apr 2, 2017

paulmasson mannequin commented Apr 2, 2017

novoselt commented Apr 2, 2017

kiwifb commented Apr 2, 2017

paulmasson mannequin commented Apr 3, 2017

paulmasson mannequin commented Apr 3, 2017

novoselt commented Apr 3, 2017

kiwifb commented Apr 3, 2017

sagetrac-git mannequin commented Apr 3, 2017

sagetrac-git mannequin commented Apr 3, 2017

novoselt commented Apr 3, 2017

novoselt commented Apr 3, 2017

kiwifb commented Apr 3, 2017

paulmasson mannequin commented Apr 3, 2017

paulmasson mannequin commented Apr 3, 2017

paulmasson mannequin commented Apr 3, 2017

paulmasson mannequin commented Apr 3, 2017

paulmasson mannequin commented Apr 3, 2017

paulmasson mannequin commented Apr 3, 2017

tscrim commented Apr 3, 2017

vbraun commented Apr 7, 2017

jdemeyer commented Jun 26, 2018

jdemeyer commented Jun 26, 2018