Opened 2 years ago

Closed 2 years ago

#29227 closed defect (fixed)

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

Reported by: gh-jcamp0x2a Owned by:
Priority: minor Milestone: sage-9.1
Component: graphics Keywords: threejs text text3d
Cc: egourgoulhon Merged in:
Authors: Joshua Campbell Reviewers: Paul Masson, Eric Gourgoulhon, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: e76dbdd (Commits, GitHub, GitLab) Commit: e76dbdda30e241a4c29dcee4afee5a38e24ca4bd
Dependencies: Stopgaps:

Status badges

Description

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.

Change History (19)

comment:1 follow-up: Changed 2 years ago by 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...

comment:2 Changed 2 years ago by gh-jcamp0x2a

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.

comment:3 Changed 2 years ago by gh-jcamp0x2a

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.

comment:4 in reply to: ↑ 1 ; follow-up: Changed 2 years ago by gh-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.

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.

comment:5 in reply to: ↑ 4 Changed 2 years ago by paulmasson

Replying to gh-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!

comment:6 Changed 2 years ago by gh-jcamp0x2a

Will do. Thanks :)

comment:7 Changed 2 years ago by gh-jcamp0x2a

  • Authors set to Joshua Campbell
  • Branch set to u/gh-jcamp0x2a/29227-threejs_repr
  • Commit set to 0cc19c0188890fda05798105d67735e08f21a6a5
  • Status changed from new to needs_review

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.

comment:8 follow-up: Changed 2 years ago by paulmasson

  • Cc egourgoulhon added

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?

comment:9 in reply to: ↑ 8 Changed 2 years ago by gh-jcamp0x2a

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.

comment:10 follow-up: Changed 2 years ago by 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!

comment:11 Changed 2 years ago by paulmasson

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

comment:12 in reply to: ↑ 10 Changed 2 years ago by egourgoulhon

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...

comment:13 Changed 2 years ago by egourgoulhon

  • Reviewers set to Paul Masson, Eric Gourgoulhon
  • Status changed from needs_review to positive_review

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!

comment:14 Changed 2 years ago by gh-jcamp0x2a

Thanks for taking a look at this Eric :)

comment:15 Changed 2 years ago by vbraun

  • Status changed from positive_review to needs_work

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
----------------------------------------------------------------------

comment:16 Changed 2 years ago by tscrim

  • Branch changed from u/gh-jcamp0x2a/29227-threejs_repr to u/tscrim/threejs_repr-29227
  • Commit changed from 0cc19c0188890fda05798105d67735e08f21a6a5 to e76dbdda30e241a4c29dcee4afee5a38e24ca4bd
  • Reviewers changed from Paul Masson, Eric Gourgoulhon to Paul Masson, Eric Gourgoulhon, Travis Scrimshaw
  • Status changed from needs_work to needs_review

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


New commits:

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

comment:17 Changed 2 years ago by egourgoulhon

  • Status changed from needs_review to positive_review

Thank you Travis!

comment:18 Changed 2 years ago by gh-jcamp0x2a

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.

comment:19 Changed 2 years ago by vbraun

  • Branch changed from u/tscrim/threejs_repr-29227 to e76dbdda30e241a4c29dcee4afee5a38e24ca4bd
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.