Plotting single text3d results in empty scene with Three.js viewer

[jcamp0x2a]

Attempting to plot a single piece of text using the three.js viewer results in an empty scene (besides the coordinate frame):

text3d("Hello world!", (1, 2, 3))

Inspecting the javascript in the generated HTML file, the texts array is empty: var texts = []

Adding any other Graphics3d object (even an empty one) to it fixes the problem:

from sage.plot.plot3d.base import Graphics3d
text3d("Hello world!", (1, 2, 3)) + Graphics3d()

I encountered this in my Ubuntu install based on 9.1.beta3 while working on #29194. I also tried it on my Windows install which is still 8.9, and the bug is present there as well.

CC: @egourgoulhon

Component: graphics

Keywords: threejs text text3d

Author: Joshua Campbell

Branch/Commit: e76dbdd

Reviewer: Paul Masson, Eric Gourgoulhon, Travis Scrimshaw

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

[paulmasson]

comment:1

Yeah, I remember coming across this when I first wrote the viewer. Something in the Python goes wrong somewhere. Need to run this down...

[jcamp0x2a]

comment:2

I believe it's because rich_repr_threejs is expecting the text to always be 2 levels deep after flattening: the for loop grabs the 1st level, and the hasattr(p.all[0], 'string') is looking at the 2nd level. For a single piece of text, it's only 1 level deep (below a TransformGroup). But when added to another Graphics3d object it becomes 2 levels deep (that TransformGroup is now below a Graphics3dGroup).

A similar bug can be produced going in the opposite direction (too many levels):

sage: t1 = text3d("t1", (0,0,1))
sage: t2 = text3d("t2", (0,1,0))
sage: t3 = text3d("t3", (1,0,0))
sage: g12 = t1 + t2
sage: g12 = g12.translate(-1,-1,-1)
sage: g123 = g12 + t3
sage: show(g123)

In this case, only "t3" gets displayed since its text is 2 levels deep but the text nodes for "t1" and "t2" are now 3 levels deep (Graphics3dGroup -> TransformGroup -> TransformGroup -> Text).

I think _rich_repr_threejs may need to be modified to walk the tree of groups/transforms to find points/lines/texts instead of relying on flattening.

[jcamp0x2a]

comment:3

I ran into a closely related issue earlier today due to the presence of a TransformGroup:

def revolved(phi):
    return parametric_plot3d([x, 0, sqrt(x)], (x, 0, 1)).rotateZ(phi)
show(revolved(0))

...works as expected and just shows a single curve, but...

show(revolved(0) + revolved(pi))

...produces a single 3D arrow instead of the expected two curves.

After flattening, each Line is under a TransformGroup, and both of those are under a single Graphics3dGroup. Thus, both lines fall into the case where hasattr(p, '_trans') and hasattr(p.all[0], 'points') are both True, resulting in an arrow.

[jcamp0x2a]

comment:4

Replying to @paulmasson:

Yeah, I remember coming across this when I first wrote the viewer. Something in the Python goes wrong somewhere. Need to run this down...

Hi Paul. Do you mind if I take a shot at fixing this? I wanted to check with you first to make sure there's no duplicated effort.

I'd like to try out walking the scene graph vs. flattening. I do have an ulterior motive: this would allow me to make my animation branch much more elegant by introducing a KeyframeAnimationGroup -- somewhat analogous to a TransformGroup -- that can be nested instead of the complicated combination of animation variables, keyframe indices, and the per-object mapping between them that I'm currently using.

[paulmasson]

comment:5

Replying to @jcamp0x2a:

Replying to @paulmasson:

Yeah, I remember coming across this when I first wrote the viewer. Something in the Python goes wrong somewhere. Need to run this down...

Hi Paul. Do you mind if I take a shot at fixing this? I wanted to check with you first to make sure there's no duplicated effort.

Go for it!

[jcamp0x2a]

comment:6

Will do. Thanks :)

[jcamp0x2a]

comment:7

I pushed a branch that resolves the problems mentioned in the issue description and comments. It extends the existing pattern that was used to collect surfaces from the scene graph (json_repr) to points, lines, and texts via new threejs_repr methods. For testing, I ran through as many of the example plots from the reference manual as I could find.

[jcamp0x2a]

Branch: u/gh-jcamp0x2a/29227-threejs_repr

[jcamp0x2a]

Commit: 0cc19c0

[jcamp0x2a]

Author: Joshua Campbell

[paulmasson]

comment:8

This is excellent! I've confirmed it builds and runs as expected. It also fixes #29206 and #29251.

Questions about design choice: Three.js is not the only way to implement WebGL, yet you have named all your new methods threejs_repr. I can understand that you would want to move my Three.js-specific modifications out of json_repr in index_face_set, but should we have separate json_reprs for all geometric objects? Or would that just make a mess of things, since we will only be using these new methods in Three.js for the foreseeable future?

Eric, would you like to chime in on this ticket?

[jcamp0x2a]

comment:9

Replying to @paulmasson:

Questions about design choice: Three.js is not the only way to implement WebGL, yet you have named all your new methods threejs_repr. I can understand that you would want to move my Three.js-specific modifications out of json_repr in index_face_set, but should we have separate json_reprs for all geometric objects? Or would that just make a mess of things, since we will only be using these new methods in Three.js for the foreseeable future?

For the name, I tried to follow the convention that I saw with some of the other viewers that have their own *_repr methods like jmol_repr and tachyon_repr. I'm not particularly attached to it, though, so I'd be okay with a webgl_repr or otherwise if that makes more sense.

I think json_repr is what the canvas3d viewer uses, too, _rich_repr_canvas3d. I don't have any experience using it, and I didn't want to possibly introduce a bug in one viewer whilst trying to fix one in another :) so I didn't make any modifications directly to those methods or try to introduce corresponding json_repr methods for points, lines, and text. I could revisit that decision if desired.

Thanks for taking a look at my changes. I know things are kinda hectic recently. I hope all is going well for you and yours.

[paulmasson]

comment:10

Yeah, I'm being too fussy about the method names. They're fine as is.

Eric, since I don't use Sage much, could you look over the code to check that it conforms to current standards for examples, documentation, etc.? Otherwise it looks good to go. Want to get this into the next beta!

[paulmasson]

comment:11

And hope as well that everyone is well in these trying times!

[egourgoulhon]

comment:12

Replying to @paulmasson:

Yeah, I'm being too fussy about the method names. They're fine as is.

Eric, since I don't use Sage much, could you look over the code to check that it conforms to current standards for examples, documentation, etc.? Otherwise it looks good to go. Want to get this into the next beta!

I am currently having a look...

[egourgoulhon]

comment:13

I gave a look to the code, checked the documentation and ran a few tests. In particular, I confirm #29206 is fixed by the current ticket. Everything looks good to me. The patchbot seems happy too.
Thanks a lot for this piece of work!

[egourgoulhon]

Reviewer: Paul Masson, Eric Gourgoulhon

[jcamp0x2a]

comment:14

Thanks for taking a look at this Eric :)

[vbraun]

comment:15

On 32-bit:

Doctesting 3819 files using 2 threads.
**********************************************************************
File "src/sage/plot/plot3d/implicit_surface.pyx", line 1156, in sage.plot.plot3d.implicit_surface.ImplicitSurface.threejs_repr
Failed example:
    G.threejs_repr(G.default_render_params())
Expected:
    [('surface',
      {'color': '#6666ff',
       'faces': [[0, 1, 2],
        ...
       'opacity': 1.0,
       'vertices': [{'x': 1.0,
         'y': -0.9743589743589743,
         'z': -0.02564102564102566},
        ...
        {'x': -1.0, 'y': 0.9487179487179487, 'z': 0.05128205128205132}]})]
Got:
    [('surface',
      {'color': '#6666ff',
       'faces': [[0, 1, 2],
[...]
        [20526, 20527, 20528],
        [20529, 20530, 20531]],
       'opacity': 1.0,
       'vertices': [{'x': 0.9999999999999999,
         'y': -0.9743589743589742,
         'z': -0.025641025641025675},
        {'x': 0.9999999999999999,
         'y': -0.9487179487179487,
         'z': -0.051282051282051135},
[...]
        {'x': -0.9743589743589743,
         'y': 0.9487179487179487,
         'z': 0.025641025641025605},
        {'x': -1.0, 'y': 0.9743589743589743, 'z': 0.025641025641025605},
        {'x': -1.0, 'y': 0.9487179487179487, 'z': 0.051282051282051246}]})]
**********************************************************************
1 item had failures:
   1 of   5 in sage.plot.plot3d.implicit_surface.ImplicitSurface.threejs_repr
    [101 tests, 1 failure, 12.17 s]
----------------------------------------------------------------------
sage -t --long src/sage/plot/plot3d/implicit_surface.pyx  # 1 doctest failed
----------------------------------------------------------------------

[tscrim]

comment:16

Here is a branch that makes the doctest compatible with 32-bit and 64-bit.

---

New commits:

e36459a	Merge branch 'u/gh-jcamp0x2a/29227-threejs_repr' of git://trac.sagemath.org/sage into u/gh-jcamp0x2a/29227-threejs_repr
e76dbdd	Fixing doctest for numerical noise on 32-bit.

[tscrim]

Changed reviewer from Paul Masson, Eric Gourgoulhon to Paul Masson, Eric Gourgoulhon, Travis Scrimshaw

[tscrim]

Changed branch from u/gh-jcamp0x2a/29227-threejs_repr to u/tscrim/threejs_repr-29227

[tscrim]

Changed commit from 0cc19c0 to e76dbdd

[egourgoulhon]

comment:17

Thank you Travis!

[jcamp0x2a]

comment:18

I'll second that! Many thanks, Travis.

I got Sage up and running on a new 32-bit VM, and I was able to verify that your branch builds and that the test case in question now passes on both my 32-bit and 64-bit systems.

I'll keep 32-bit in mind in any future Sage work I do.

[vbraun]

Changed branch from u/tscrim/threejs_repr-29227 to e76dbdd