Apply flat shading when plotting 3d polyhedra with Three.js

[slel]

We should ensure flat shading is the default
when plotting 3d polyhedra with Three.js.

In this example, only the middle cube (at the origin)
has flat shading and looks correct:

sage: cc = [(0, 0, 0), (-1.125, 1.125, 0), (1.125, -1.125, 0)]
sage: cubes = sum(cube(c) for c in cc)
sage: cubes.show(frame=False)

The special case of Platonic solids (as purely graphics objects)
was done in #27836.

In this ticket we do the following:

• We fix a bug in plot3d.platonic:
  When translating such a platonic solid,
  the default behavior of flat shading is ignored
  as shown in the previous example.
  We fix that.

• We set flat shading to be the default behavior
  for plots of geometry.polyhedron.

CC: @egourgoulhon @jcamp0x2a @LaisRast @guenterrote @paulmasson @slel

Component: graphics

Keywords: threejs, flat shading, polyhedra

Author: Laith Rastanawi

Branch: 2125039

Reviewer: Matthias Koeppe

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

[mkoeppe]

comment:1

Sage development has entered the release candidate phase for 9.3. Setting a new milestone for this ticket based on a cursory review of ticket status, priority, and last modification date.

[slel]

comment:3

I thought changing the plot method
in src/sage/geometry/polyhedron/base.py
to set threejs_flat_shading to True
for the case of 3d polyhedra would do the trick.

I tried this change without success:

diff --git a/src/sage/geometry/polyhedron/base.py b/src/sage/geometry/polyhedron/base.py
index 843fe204cb..918ce178c9 100644
--- a/src/sage/geometry/polyhedron/base.py
+++ b/src/sage/geometry/polyhedron/base.py
@@ -815,6 +815,7 @@ class Polyhedron_base(Element):
              wireframe='blue', fill='green',
              position=None,
              orthonormal=True,  # whether to use orthonormal projections
+             threejs_flat_shading=True,  # flat shading for threejs plotting
              **kwds):
         """
         Return a graphical representation.
@@ -847,6 +848,9 @@ class Polyhedron_base(Element):
         - ``orthonormal`` -- Boolean (default: True); whether to use
           orthonormal projections.

+        - ``threejs_flat_shading`` -- boolean (default: True);
+          whether to apply flat shading when plotting with Three.js
+
         - ``**kwds`` -- optional keyword parameters that are passed to
           all graphics objects.

Ideas anyone?

[slel]

Attachment: polyhedron-flat-shading.png

Flat shading wanted for polyhedra

[LaisRast]

comment:5

I think there are two different things here:

• There is a bug in plot3d.platonic:
  As your example shows, when changing the center in one of these Platonic solids, the plot does not respect the default threejs_flat_shading=True.
  This is a bug which occurs in the function prep.
  To fix it, the assignment threejs_flat_shading=True should be set before applying the translation.

• For plots of geometry.polyhedron, one can set flat shading as follows:

sage: P = polytopes.cube()
sage: P.plot(polygon={"threejs_flat_shading": True})

I will set this as the default behavior.
However, I do not encourage putting threejs_flat_shading as a parameter of the function plot.
style-related parameters of plot should be passed through the parameters point, line and polygon.

[LaisRast]

Branch: public/31426

[sagetrac-git]

Commit: 2125039

[sagetrac-git]

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

2b988e7	ensure flat shading when applying translation
2125039	flat shading as default

[slel]

comment:7

Fill in the author field with your full name
and set to "needs review" if ready.

[LaisRast]

Author: Laith Rastanawi

[LaisRast]

Changed keywords from none to threejs, flat shading, polyhedra

[mkoeppe]

comment:9

Stalled in needs_review or needs_info; likely won't make it into Sage 9.5.

[mkoeppe]

comment:10

These are nice improvements! Thanks for working on this.

[mkoeppe]

Reviewer: Matthias Koeppe

[vbraun]

Changed branch from public/31426 to 2125039

[slel]

Changed commit from 2125039 to none

[slel]

comment:13

It seems the wrong shading sometimes persists.

Juanjo's answer to Ask Sage question 63031
seems to indicate the need for a follow-up.

[LaisRast]

comment:14

Replying to @slel:

It seems the wrong shading sometimes persists.

Juanjo's answer to Ask Sage question 63031
seems to indicate the need for a follow-up.

The situation is as follows:

• When calling cube() with frame_thickness > 0,
  the returned object is a Graphics3dGroup which contains two objects: a box and a frame.
  Otherwise, only the box is returned.

• To the returned object, the function prep() is applied,
  which among other things, sets flat shading for the returned object.

• When converting a Graphics3dGroup into json for threejs,
  the dictionary ._extra_kwds (in which flat shading is set) is ignored.
  Only the ._extra_kwds's in the child objects are considered.

I think the function prep() does not assume to get a Graphics3dGroup.
The only case when it receives one is the case above.
Thus, I suggest a workaround just for the cube() by applying the flat shading
on the box object at the beginning of cube():

    if 'threejs_flat_shading' not in kwds: 
        kwds['threejs_flat_shading'] = True

This kwds is passed to the box object.

By the way, the current implementations force the flat shading even if the user
explicitly says they do not want it. So
cube(threejs_flat_shading=False)
will produce flat shading.