Make threejs / three.js viewer respect thickness in parametric_plot3d

slel commented 5 years ago

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

CC: @embray @jcamp0x2a @paulmasson @slel @egourgoulhon @novoselt

Component: graphics

Keywords: threejs, three.js, parametric_plot3d, thickness, graphic options

Author: Joshua Campbell

Branch/Commit: c90639a

Reviewer: Eric Gourgoulhon

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

slel commented 5 years ago

Changed keywords from none to threejs, three.js, parametric_plot3d, thickness, graphic options

slel commented 5 years ago

Description changed:

--- 
+++ 
@@ -1,5 +1,5 @@
-In Sage 8.3, the Jmol viewer respects the thickness optional parameter in  parametric_plot3d,
-but the threejs / three.js viewer does not.
+In Sage 8.3, the three.js / threejs viewer does not respects the thickness optional
+parameter in parametric_plot3d, while other viewers such as Jmol and Tachyon do.

sage: pi = RDF.pi() @@ -9,8 +9,10 @@ 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='threejs') -Launched html viewer for Graphics3d Object +sage: p.show(viewer='tachyon') +Launched png viewer for Graphics3d Object

53959995-fd20-47f5-85e6-5e769b863d1f commented 5 years ago

comment:2

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?

slel commented 5 years ago

comment:3

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.

53959995-fd20-47f5-85e6-5e769b863d1f commented 5 years ago

comment:4

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.

slel commented 5 years ago

comment:5

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

embray commented 5 years ago

comment:6

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?

slel commented 5 years ago

Description changed:

--- 
+++ 
@@ -1,5 +1,11 @@
-In Sage 8.3, the three.js / threejs viewer does not respects the thickness optional
-parameter in parametric_plot3d, while other viewers such as Jmol and Tachyon do.
+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() @@ -16,3 +22,7 @@ sage: p.show(viewer='tachyon') Launched png viewer for Graphics3d Object

+
+Suggested workarounds:
+- use `radius` instead of `thickness`.
+- use THREE.Meshline

slel commented 5 years ago

comment:8

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

21c3cf6e-6c50-4e8b-9831-5a03f9a599c1 commented 3 years ago

comment:10

Replying to @slel:

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?

53959995-fd20-47f5-85e6-5e769b863d1f commented 3 years ago

comment:11

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

21c3cf6e-6c50-4e8b-9831-5a03f9a599c1 commented 3 years ago

comment:12

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?

21c3cf6e-6c50-4e8b-9831-5a03f9a599c1 commented 3 years ago

Branch: u/gh-jcamp0x2a/26410-threejs-fat-lines

21c3cf6e-6c50-4e8b-9831-5a03f9a599c1 commented 3 years ago

Dependencies: 30462

21c3cf6e-6c50-4e8b-9831-5a03f9a599c1 commented 3 years ago

Commit: b3a444e

21c3cf6e-6c50-4e8b-9831-5a03f9a599c1 commented 3 years ago

Author: Joshua Campbell

21c3cf6e-6c50-4e8b-9831-5a03f9a599c1 commented 3 years ago

comment:13

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

21c3cf6e-6c50-4e8b-9831-5a03f9a599c1 commented 3 years ago

Attachment: threejs-r120.tar.gz

53959995-fd20-47f5-85e6-5e769b863d1f commented 3 years ago

comment:14

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?

21c3cf6e-6c50-4e8b-9831-5a03f9a599c1 commented 3 years ago

comment:15

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.

53959995-fd20-47f5-85e6-5e769b863d1f commented 3 years ago

comment:16

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

21c3cf6e-6c50-4e8b-9831-5a03f9a599c1 commented 3 years ago

comment:17

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.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from b3a444e to e190d71

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

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

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

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

a49eba0 Fix a test involving path to CDN script

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from e190d71 to a49eba0

mkoeppe commented 3 years ago

Changed dependencies from 30462 to #30462

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

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

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from a49eba0 to 7575ea9

21c3cf6e-6c50-4e8b-9831-5a03f9a599c1 commented 3 years ago

comment:22

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

egourgoulhon commented 3 years ago

comment:25

Replying to @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?

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 7575ea9 to c90639a

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

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

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

21c3cf6e-6c50-4e8b-9831-5a03f9a599c1 commented 3 years ago

comment:27

Replying to @egourgoulhon:

Replying to @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.

mkoeppe commented 3 years ago

Changed dependencies from #30462 to none

egourgoulhon commented 3 years ago

Reviewer: Eric Gourgoulhon

egourgoulhon commented 3 years ago

comment:29

Sorry the delay! LGTM. Thanks.

21c3cf6e-6c50-4e8b-9831-5a03f9a599c1 commented 3 years ago

comment:30

Replying to @egourgoulhon: