Opened 4 years ago

Closed 22 months ago

#26410 closed defect (fixed)

Make threejs / three.js viewer respect thickness in parametric_plot3d

Reported by: Samuel Lelièvre Owned by:
Priority: major Milestone: sage-9.3
Component: graphics Keywords: threejs, three.js, parametric_plot3d, thickness, graphic options
Cc: Erik Bray, Joshua Campbell, Paul Masson, Samuel Lelièvre, Eric Gourgoulhon, Andrey Novoseltsev Merged in:
Authors: Joshua Campbell Reviewers: Eric Gourgoulhon
Report Upstream: N/A Work issues:
Branch: c90639a (Commits, GitHub, GitLab) Commit: c90639a2e61752644244a0c8edbeeca7add255ed
Dependencies: Stopgaps:

Status badges

Description (last modified by Samuel Lelièvre)

On Windows and macOS, WebGL line thickness support is absent or limited, so that the three.js / threejs viewer appears not to respect the thickness optional parameter in parametric_plot3d, while other viewers such as Jmol and Tachyon do.

For example, in Sage 8.3 or 8.7 on macOS 10.10.5, the thickness is not taken into account in the following -- whether launching it from the Sage REPL, the Jupyter notebook, or even SageCell online:

sage: pi = RDF.pi()
sage: tau = 2*pi
sage: a = (lambda t: cos(t), lambda t: sin(t), lambda t: 2*t/tau)
sage: b = (lambda t: cos(t), lambda t: sin(t), lambda t: 2*t/tau + 1)
sage: aa = parametric_plot(a, (-tau, tau), color='blue', thickness=1)
sage: bb = parametric_plot(b, (-3*pi, pi), color='red', thickness=10)
sage: p = aa + bb
sage: p.show(viewer='threejs')
Launched html viewer for Graphics3d Object
sage: p.show(viewer='jmol')
Launched jmol viewer for Graphics3d Object
sage: p.show(viewer='tachyon')
Launched png viewer for Graphics3d Object

Suggested workarounds:

  • use radius instead of thickness.
  • use THREE.Meshline

Attachments (1)

threejs-r120.tar.gz (167.7 KB) - added by Joshua Campbell 2 years ago.

Download all attachments as: .zip

Change History (32)

comment:1 Changed 4 years ago by Samuel Lelièvre

Description: modified (diff)
Keywords: threejs three.js parametric_plot3d thickness graphic options added

comment:2 Changed 4 years ago by Paul Masson

Samuel, I don't see what the problem is: the red line looks thicker to me than the blue in the Three.js viewer. What are your expecting?

comment:3 Changed 4 years ago by Samuel Lelièvre

Paul, thanks a lot for having a look. For me the red line looks thicker in jmol and tachyon, but not in threejs. Not only on my computer, but also on sagecell.

comment:4 Changed 4 years ago by Paul Masson

That's probably because you're using a Windows machine. WebGL line thickness isn't supported at all in Windows, and support for it in macOS has limitations. It looks like line thickness will be removed from future versions of WebGL entirely.

The way to visualize thick lines is to use the radius parameter listed in the documentation, which creates a cylinder along the path:

pi = RDF.pi()
tau = 2*pi
a = (lambda t: cos(t), lambda t: sin(t), lambda t: 2*t/tau)
b = (lambda t: cos(t), lambda t: sin(t), lambda t: 2*t/tau + 1)
aa = parametric_plot(a, (-tau, tau), color='blue', thickness=1)
bb = parametric_plot(b, (-3*pi, pi), color='red', radius=.1)
p = aa + bb
p.show(viewer='threejs')

Three.js has its own workaround for thick lines, but it's only in the examples, not the main library. I wouldn't implement it here until it's an official part of the main library.

comment:5 Changed 4 years ago by Samuel Lelièvre

Thanks for explaining and for the workaround.

This was under macOS 10.10.5, using either Firefox 66.0.2 (64-bit), Opera 58.0.3135.118, Safari 10.1.2 (10603.3.8), so surely illustrating the limitations you refer to.

Using radius as you suggest does the trick. A small enough radius works well. If the curve is long and the radius gets somewhat large it is worth increasing the number of plot points so that the (piecewise linear) cylinder still looks more curved than piecewise linear.

The following works well:

pi = RDF.pi()
tau = 2*pi
a = (lambda t: cos(t), lambda t: sin(t), lambda t: 2*t/tau)
b = (lambda t: cos(t), lambda t: sin(t), lambda t: 2*t/tau + 1)
aa = parametric_plot(a, (-tau, tau), color='blue', radius=.02, plot_points=400)
bb = parametric_plot(b, (-3*pi, pi), color='red', radius=.04, plot_points=400)
p = aa + bb
p.show(viewer='threejs')

I also notice that the cylinders have a polygonal base, with a number of sides depending on the radius; it seems to start 5-sided for small radius, and then the number of sides grows with the radius. Is there a way to control that number of sides?

The following example shows the number of sides starting at 5 then increasing to 6, 7, 8, 8, 9, 10, 11 (makes one want to guess the fomula for number_of_sides as a function of radius):

pp = parametric_plot
cyl = lambda x, y: (lambda t: x, lambda t: y, lambda t: t)
pcyl = lambda x, y, r, h: pp(cyl(x, y), (0, 1), radius=r, rgbcolor=hue(h))
param = (.03, .04, .05, .06, .07, .08, .09, .10, .11, .12, .13, .14, .15, .16)
np = RDF(len(param))
cylparam = lambda i, r: (0, 2.05*sum(param[:i])+r, r, (i+1)/(np+1))
p = sum((pcyl(*cylparam(i, r)) for i, r in enumerate(param)), Graphics())
p.show(frame=False, viewer='threejs')

comment:6 Changed 4 years ago by Erik Bray

I have encountered this problem myself, about a year ago, with my own threejs experiments outside of Sage. It's really too bad this is still a problem. IIRC the best workaround I found was to use the THREE.MeshLine plugin: https://github.com/spite/THREE.MeshLine

In particular, check out this cool demo: https://www.clicktorelease.com/code/THREE.MeshLine/demo/index.html

Perhaps we could work that into Sage for use in parametric plots?

comment:7 Changed 4 years ago by Samuel Lelièvre

Description: modified (diff)
Milestone: sage-8.4sage-wishlist

comment:8 Changed 4 years ago by Samuel Lelièvre

Thanks! Very cool demo indeed -- it would be very nice to work that into Sage.

comment:9 Changed 2 years ago by Samuel Lelièvre

Cc: Erik Bray Joshua Campbell Paul Masson added

comment:10 in reply to:  8 Changed 2 years ago by Joshua Campbell

Replying to slelievre:

Thanks! Very cool demo indeed -- it would be very nice to work that into Sage.

I was able to get MeshLine integrated into the Three.js viewer quite easily. It's nice being able to get thick lines on my Windows box that aren't cylinders :) I've also got a branch that uses MeshLine instead of sprites to add arrow heads. (Although I don't think I ever put my sprite arrow heads up for review. They make plot_vector_field3d look a lot nicer.)

The packaging is the only part I'm unsure of. Right now I'm just having Sage copy the MeshLine code directly into the generated pages similar to what I do for my animation files. I imagine we'd probably want to treat it similarly to threejs.min.js and OrbitControls.js? That is: include THREE.MeshLine.js in the threejs SPKG for offline use and use the jsDelivr CDN link for online use?

comment:11 Changed 2 years ago by Paul Masson

With respect to packaging, we can’t just add it to what we’re doing with Three.js because it has a different owner with a separate license. And remember that Three.js has its own implementation that looks better. We could package some of the files from the examples directory just like OrbitControls.js, but keep in mind that all of the simple non-module JavaScript files are disappearing at the end of the year...

comment:12 in reply to:  11 Changed 2 years ago by Joshua Campbell

Replying to paulmasson:

With respect to packaging, we can’t just add it to what we’re doing with Three.js because it has a different owner with a separate license.

Hmm, good point.

And remember that Three.js has its own implementation that looks better.

Nice! I was not aware of that example (missed your previous comment in this ticket), and I do agree it looks better. Also looks like we could use the wireframe example below it to spruce up the surface grid lines, too. One downside is that it doesn't seem to support varying width over the line like MeshLine does. I was using that to create arrow heads. Doesn't look like it would be difficult to add, though. (Don't quote me on that!)

We could package some of the files from the examples directory just like OrbitControls.js,

I will proceed along the same lines then.

but keep in mind that all of the simple non-module JavaScript files are disappearing at the end of the year...

Out of curiosity, do we know how we're going to handle this? As I understand, modules can't be loaded from the filesystem. So...

  • just drop support for running it purely locally, i.e. online=False and outside Jupyter?
  • spin up a simple server the first time a Three.js plot is shown per session for the sole purpose of serving the Three.js files? Would probably want to print a warning: "Hey! This plot won't work once you exit Sage!"
  • maintain our own non-module fork of the example files? I don't really have any interest in trying to keep such a fork up-to-date, even for only one file; heaven forbid the core Three.js ever goes module-only.
  • something else?

comment:13 Changed 2 years ago by Joshua Campbell

Authors: Joshua Campbell
Branch: u/gh-jcamp0x2a/26410-threejs-fat-lines
Commit: b3a444e0014187065db8712c8d7e67fc357338ed
Dependencies: 30462
Milestone: sage-wishlistsage-9.3
Status: newneeds_review

I've pushed a branch that is based on the webgl_lines_fat example that applies to both regular lines and surface grid lines. I opted not to follow the webgl_lines_fat_wireframe example for the surface grid since it would outline the generated triangles and not the original polygons coming from Sage.

Requires the following files from Three.js's examples/js/lines directory:

  • LineMaterial.js
  • LineSegmentsGeometry.js
  • LineGeometry.js
  • LineSegments2.js
  • Line2.js

They have all been added to the threejs SPKG. Since I had to modify the SPKG anyway, I went ahead and bumped Three.js up to the current version, r120. The new tarball is attached. If it'd be preferred to do the upgrade/repackaging in a separate ticket first, I can do that as well.

Whether these files and the new fat_lines.js script that uses them are included on the page depends on whether any lines or surface meshes with thickness > 1 are present in the scene so as not to slow down loading of plots that don't require this functionality.

I'm anticipating a merge conflict with #30462, but it should be trivial.


New commits:

b3a444eThree.js: update to r120 and support line thickness > 1 on all platforms

Changed 2 years ago by Joshua Campbell

Attachment: threejs-r120.tar.gz added

comment:14 Changed 2 years ago by Paul Masson

Milestone: sage-9.3sage-wishlist

Joshua, the version upgrade needs to be a separate ticket so the release manager knows to copy the attachment to the mirrors.

I haven’t looked at the Three.js example in detail. Are all of the files you copied actually used here?

comment:15 in reply to:  14 Changed 2 years ago by Joshua Campbell

Replying to paulmasson:

Joshua, the version upgrade needs to be a separate ticket so the release manager knows to copy the attachment to the mirrors.

Understood. Is there somewhere I would need to post the ticket number, like in sage-devel, in order for it to be picked up? Or would duplicating the structure of your r117 ticket, #29809, be enough? Please forgive me if I'm overstepping here and you'd prefer to manage the Three.js upgrades. I know the viewer is your creation, and I'm very grateful for it :)

I haven’t looked at the Three.js example in detail. Are all of the files you copied actually used here?

Yes, they're all used in fat_lines.js, but I could potentially get rid of Line2 and LineGeometry if we want to absolutely minimize the number of scripts fetched / included on the page. They are pretty minimal wrappers around LineSegments2 and LineSegmentsGeometry, respectively. Could easily turn the line strip into line segments ourselves.

comment:16 Changed 2 years ago by Paul Masson

I don't mind if you perform the upgrade. Just copy the last one, making sure "upgrade" is clearly stated. And make sure there are no errors in the template from features that might have changed. Test against a variety of graphics.

I've opened #30620 to discuss the possibilities for handling modular files. Considering that the change is coming in a couple months, you might want to minimize the number of files copied now. Or you may want to put this ticket on hold until we address the bigger issue. The change is a real pain...

comment:17 in reply to:  16 Changed 2 years ago by Joshua Campbell

Status: needs_reviewneeds_info

Replying to paulmasson:

I've opened #30620 to discuss the possibilities for handling modular files. Considering that the change is coming in a couple months, you might want to minimize the number of files copied now. Or you may want to put this ticket on hold until we address the bigger issue. The change is a real pain...

Going to put it on hold then until that's resolved.

comment:18 Changed 2 years ago by git

Commit: b3a444e0014187065db8712c8d7e67fc357338ede190d719856f2cdf7d8c919c4b4e2e8543892ee9

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

e190d71Three.js: update to r122 and support line thickness > 1 on all platforms

comment:19 Changed 2 years ago by git

Commit: e190d719856f2cdf7d8c919c4b4e2e8543892ee9a49eba0a074e9467c99313f5d85c31a3f48306c1

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

a49eba0Fix a test involving path to CDN script

comment:20 Changed 2 years ago by Matthias Köppe

Dependencies: 30462#30462

comment:21 Changed 2 years ago by git

Commit: a49eba0a074e9467c99313f5d85c31a3f48306c17575ea9c0f882671ac505da524694aa8606c8e13

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

7575ea9Three.js: support line thickness > 1 on all platforms using "fat" lines

comment:22 Changed 2 years ago by Joshua Campbell

Status: needs_infoneeds_review

Now that the Three.js r122 update has been merged in #30915, I've pushed an updated version of this branch.

comment:23 Changed 2 years ago by Joshua Campbell

Milestone: sage-wishlistsage-9.3

comment:24 Changed 2 years ago by Matthias Köppe

Cc: Eric Gourgoulhon Andrey Novoseltsev added

comment:25 in reply to:  22 ; Changed 2 years ago by Eric Gourgoulhon

Replying to gh-jcamp0x2a:

Now that the Three.js r122 update has been merged in #30915, I've pushed an updated version of this branch.

There is a merge conflict with Sage 9.3.beta5. Could you please fix this?

comment:26 Changed 2 years ago by git

Commit: 7575ea9c0f882671ac505da524694aa8606c8e13c90639a2e61752644244a0c8edbeeca7add255ed

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

c90639aMerge branch 'develop' into u/gh-jcamp0x2a/26410-threejs-fat-lines

comment:27 in reply to:  25 Changed 2 years ago by Joshua Campbell

Replying to egourgoulhon:

Replying to gh-jcamp0x2a:

Now that the Three.js r122 update has been merged in #30915, I've pushed an updated version of this branch.

There is a merge conflict with Sage 9.3.beta5. Could you please fix this?

Thanks, I've merged in the latest develop branch. Did a quick sanity check to make sure the feature still works, too.

comment:28 Changed 23 months ago by Matthias Köppe

Dependencies: #30462

comment:29 Changed 22 months ago by Eric Gourgoulhon

Reviewers: Eric Gourgoulhon
Status: needs_reviewpositive_review

Sorry the delay! LGTM. Thanks.

comment:30 in reply to:  29 Changed 22 months ago by Joshua Campbell

Replying to egourgoulhon:

Sorry the delay! LGTM. Thanks.

No worries. I haven't been able to do much Sage stuff recently myself. Thanks for the review :)

comment:31 Changed 22 months ago by Volker Braun

Branch: u/gh-jcamp0x2a/26410-threejs-fat-linesc90639a2e61752644244a0c8edbeeca7add255ed
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.