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:  sage9.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: 
Description (last modified by )
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 ofthickness
.  use THREE.Meshline
Attachments (1)
Change History (32)
comment:1 Changed 4 years ago by
Description:  modified (diff) 

Keywords:  threejs three.js parametric_plot3d thickness graphic options added 
comment:2 Changed 4 years ago by
comment:3 Changed 4 years ago by
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
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
Thanks for explaining and for the workaround.
This was under macOS 10.10.5, using either Firefox 66.0.2 (64bit), 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 5sided 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
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
Description:  modified (diff) 

Milestone:  sage8.4 → sagewishlist 
comment:8 followup: 10 Changed 4 years ago by
Thanks! Very cool demo indeed  it would be very nice to work that into Sage.
comment:9 Changed 2 years ago by
Cc:  Erik Bray Joshua Campbell Paul Masson added 

comment:10 Changed 2 years ago by
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 followup: 12 Changed 2 years ago by
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 nonmodule JavaScript files are disappearing at the end of the year...
comment:12 Changed 2 years ago by
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 nonmodule 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 nonmodule fork of the example files? I don't really have any interest in trying to keep such a fork uptodate, even for only one file; heaven forbid the core Three.js ever goes moduleonly.
 something else?
comment:13 Changed 2 years ago by
Authors:  → Joshua Campbell 

Branch:  → u/ghjcamp0x2a/26410threejsfatlines 
Commit:  → b3a444e0014187065db8712c8d7e67fc357338ed 
Dependencies:  → 30462 
Milestone:  sagewishlist → sage9.3 
Status:  new → needs_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:
b3a444e  Three.js: update to r120 and support line thickness > 1 on all platforms

Changed 2 years ago by
Attachment:  threejsr120.tar.gz added 

comment:14 followup: 15 Changed 2 years ago by
Milestone:  sage9.3 → sagewishlist 

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 Changed 2 years ago by
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 sagedevel, 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 followup: 17 Changed 2 years ago by
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 Changed 2 years ago by
Status:  needs_review → needs_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
Commit:  b3a444e0014187065db8712c8d7e67fc357338ed → e190d719856f2cdf7d8c919c4b4e2e8543892ee9 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
e190d71  Three.js: update to r122 and support line thickness > 1 on all platforms

comment:19 Changed 2 years ago by
Commit:  e190d719856f2cdf7d8c919c4b4e2e8543892ee9 → a49eba0a074e9467c99313f5d85c31a3f48306c1 

Branch pushed to git repo; I updated commit sha1. New commits:
a49eba0  Fix a test involving path to CDN script

comment:20 Changed 2 years ago by
Dependencies:  30462 → #30462 

comment:21 Changed 2 years ago by
Commit:  a49eba0a074e9467c99313f5d85c31a3f48306c1 → 7575ea9c0f882671ac505da524694aa8606c8e13 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
7575ea9  Three.js: support line thickness > 1 on all platforms using "fat" lines

comment:22 followup: 25 Changed 2 years ago by
Status:  needs_info → needs_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
Milestone:  sagewishlist → sage9.3 

comment:24 Changed 2 years ago by
Cc:  Eric Gourgoulhon Andrey Novoseltsev added 

comment:25 followup: 27 Changed 2 years ago by
Replying to ghjcamp0x2a:
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
Commit:  7575ea9c0f882671ac505da524694aa8606c8e13 → c90639a2e61752644244a0c8edbeeca7add255ed 

Branch pushed to git repo; I updated commit sha1. New commits:
c90639a  Merge branch 'develop' into u/ghjcamp0x2a/26410threejsfatlines

comment:27 Changed 2 years ago by
Replying to egourgoulhon:
Replying to ghjcamp0x2a:
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
Dependencies:  #30462 

comment:29 followup: 30 Changed 22 months ago by
Reviewers:  → Eric Gourgoulhon 

Status:  needs_review → positive_review 
Sorry the delay! LGTM. Thanks.
comment:30 Changed 22 months ago by
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
Branch:  u/ghjcamp0x2a/26410threejsfatlines → c90639a2e61752644244a0c8edbeeca7add255ed 

Resolution:  → fixed 
Status:  positive_review → closed 
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?