variable camera position, light, etc. for tachyon ray tracer

[dkrenn]

When using the tachyon ray tracer, at the moment the parameters for the camera, as well as for the light and for the background-plane are hard-coded in sage/plot/plot3d/base.pyx:

    def tachyon(self):
    ...
         camera
            zoom 1.0
            aspectratio 1.0
            antialiasing %s
            raydepth 8
            center  2.3 2.4 2.0
            viewdir  -2.3 -2.4 -2.0
            updir  0.0 0.0 1.0
         end_camera

      light center  4.0 3.0 2.0
            rad 0.2
            color  1.0 1.0 1.0

      plane
        center -2000 -1000 -500
        normal 2.3 2.4 2.0
        TEXTURE
            AMBIENT 1.0 DIFFUSE 1.0 SPECULAR 1.0 OPACITY 1.0
            COLOR 1.0 1.0 1.0
            TEXFUNC 0

Those things should be variable, i.e. it should be possible to pass those parameters by optional parameters (e.g. when calling show).

CC: @egourgoulhon @mjungmath @jplab

Component: graphics

Keywords: camera light tachyon

Author: Günter Rote

Branch/Commit: 20a7c00

Reviewer: Laith Rastanawi

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

[guenterrote]

Branch: u/guenterrote/variable_camera_position__light__etc__for_tachyon_ray_tracer

[sagetrac-git]

Commit: f674b14

[sagetrac-git]

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

eca2294	fix omitted javascript snippets
f674b14	renamed camera_center -> camera_position

[sagetrac-git]

Changed commit from f674b14 to dd63ab9

[sagetrac-git]

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

4e85f34	some fixed, still need to check documentation
dd63ab9	fixed the documentation

[guenterrote]

comment:8

I fixed the tachyon viewer. (I did not touch the other viewers, although there would be some room for improvements too)

[sagetrac-git]

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

b3c37a0

documentation one more sentence about different viewers

[sagetrac-git]

Changed commit from dd63ab9 to b3c37a0

[guenterrote]

Changed commit from b3c37a0 to none

[guenterrote]

Changed branch from u/guenterrote/variable_camera_position__light__etc__for_tachyon_ray_tracer to public/plot3d/variable_camera_position__light__etc__for_tachyon_ray_tracer

[guenterrote]

Changed branch from public/plot3d/variable_camera_position__light__etc__for_tachyon_ray_tracer to u/guenterrote/variable_camera_position__light__etc__for_tachyon_ray_tracer

[guenterrote]

Commit: b3c37a0

[guenterrote]

comment:11

I don't know what I'm doing (I wanted to change the branch to a public branch)

---

New commits:

5413afd	inserting camera parameters for tachyon and fixing other parameters
c29270a	option to retrieve camera position from threejs after rotation; requires still more post-processing
eca2294	fix omitted javascript snippets
f674b14	renamed camera_center -> camera_position
4e85f34	some fixed, still need to check documentation
dd63ab9	fixed the documentation
b3c37a0	documentation one more sentence about different viewers

[guenterrote]

comment:12

This is the long-awaited possibility to control many aspects of the tachyon viewer by
options of the show() method.
All available options are now properly documented.

(The tachyon viewer has now more available controls than the default threejs viewer, but
reorganizing the threejs options would be another project with a separate ticket.)

I also added an overview comparing the different viewer choices, and some warnings about some unexpected behaviour of certain viewers.
(There is also some code for a so-called "wavefront" viewer. Is that obsolete?)

[slel]

comment:13

Please add full name in author field.

[sagetrac-git]

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

82384b8	eliminated double word
000f272	grammar correction
18e8aa6	small grammar fix, eliminated references to ``self`` from the doc.

[sagetrac-git]

Changed commit from 71063b1 to 18e8aa6

[sagetrac-git]

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

1e01655	grammar fix
befe2d7	improve wording

[sagetrac-git]

Changed commit from 18e8aa6 to befe2d7

[LaisRast]

comment:22

All of these comments are related to the parameter camera_center:

• In plot/base.pyz, two examples still use the old parameter camera_center. However it will be ignored since it is not in tachyon_keywords:

            sage: p.show(viewer="tachyon",camera_center=(4,0,0))
            sage: p.show(viewer="tachyon",camera_center=(2,2,0.3),aspect_ratio=1)

• Instead of removing a parameter from a function directly, a DeprecationWarning should be raised for some time before removing the parameter entirely. This makes sure that old examples still behave the same and tells the user to switch to the new parameter.

• In threejs_template.html, when clicking on Get camera option, it copies ',camera_center=' + cam_position (the old parameter name). I also suggest to only copy the value of camera position, not with the whole sentence. This makes it similar to what Copy Viewpoint does.

-        textArea.textContent = ',camera_center=' + cam_position;
+        textArea.textContent = cam_position;

[guenterrote]

comment:23

Replying to @LaisRast:

• Instead of removing a parameter from a function directly, a DeprecationWarning should be raised for some time before removing the parameter entirely. This makes sure that old examples still behave the same and tells the user to switch to the new parameter.

The old name camera_center was the name I chose initially, following the convention of the Tachyon object.
https://doc.sagemath.org/html/en/reference/plot3d/sage/plot/plot3d/tachyon.html

I chose to rename it because "camera position" is what is commonly used in computer graphics, but it has never been a parameter of show() or the tachyon() function before I introduced it; so there is no need to deprecate it.

(But I have to fix the few places where I forgot to change it.)

[guenterrote]

comment:24

Replying to @LaisRast:

• In threejs_template.html, when clicking on Get camera option, it copies ',camera_center=' + cam_position (the old parameter name). I also suggest to only copy the value of camera position, not with the whole sentence. This makes it similar to what Copy Viewpoint does.

I found it annoying that I had to write the parameter name (and perhaps look it up) myself when I used Copy Viewpoint. There is hardly any conceivable use for Get camera than to insert the result into the parameter list.

[sagetrac-git]

Changed commit from befe2d7 to ddafd37

[sagetrac-git]

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

aac0134	eliminated remainders of camera_center
ddafd37	eliminated remainders of preliminary parameter name camera_center

[LaisRast]

Reviewer: Laith Rastanawi

[LaisRast]

comment:27

• An example in the docstring of sage.geometry.polyhedron.plot.Projection.render_wireframe_3d fails due to the changes of this ticket.
• In threejs_template.html: "Get camera" -> "Get Camera" to make the UI consistent. (Just a suggestion)
• Remove old lines of code instead of commenting them out.
• Comments indentation should match indentation of the block they are in. For instance, the heading before tachyon_keywords.
• Did you mean to include all the below under the warning?

         .. WARNING::
 
-         By default, the jmol and tachyon viewers perform
-         some non-uniform scaling of the axes.
-
-        If this is not desired, one can set ``aspect_ratio=1``::
-
-            sage: p = plot3d(lambda u,v:(cos(u)-cos(v)), (-0.2,0.2),(-0.2,0.2))
-            sage: p.show(viewer="threejs")
-            sage: p.show(viewer="jmol")
-            sage: p.show(viewer="jmol",aspect_ratio=1)
-            sage: p.show(viewer="tachyon",camera_position=(4,0,0))
-            sage: p.show(viewer="tachyon",camera_position=(2,2,0.3),aspect_ratio=1)
+            By default, the jmol and tachyon viewers perform
+            some non-uniform scaling of the axes.
+
+            If this is not desired, one can set ``aspect_ratio=1``::
+
+                sage: p = plot3d(lambda u,v:(cos(u)-cos(v)), (-0.2,0.2),(-0.2,0.2))
+                sage: p.show(viewer="threejs")
+                sage: p.show(viewer="jmol")
+                sage: p.show(viewer="jmol",aspect_ratio=1)
+                sage: p.show(viewer="tachyon",camera_position=(4,0,0))
+                sage: p.show(viewer="tachyon",camera_position=(2,2,0.3),aspect_ratio=1)

If not, just make the indentation to 4 spaces in the paragraph starting with "By default..."

• In the function _flip_orientation, did you mean Javascript (threejs) instead of Java, or you meant jmol? Also the first word in the docstring of this function should be capitalized.

Other than that, this ticket looks good to me.

[guenterrote]

comment:28

Replying to @LaisRast:

• An example in the docstring of sage.geometry.polyhedron.plot.Projection.render_wireframe_3d fails due to the changes of this ticket.

Fixed. (Anyway, what is the purpose of that test? Somebody adds a line to the beginning
of the tachyon output, then that test must be updated.)

• In threejs_template.html: "Get camera" -> "Get Camera" to make the UI consistent. (Just a suggestion)

I have changed "Get Viewpoint" to "Get viewpoint" for consistency.

         .. WARNING::
 
-         By default, the jmol and tachyon viewers perform
-         some non-uniform scaling of the axes.

If not, just make the indentation to 4 spaces in the paragraph starting with "By default..."

No, I indeed want only a short warning. On my system, the following text comes out separate from the warning in the html doc. I have now increased the indentation of the warning to 4 (extra) spaces. The result looks the same as before.

• In the function _flip_orientation, did you mean Javascript (threejs) instead of Java, or you meant jmol? Also the first word in the docstring of this function should be capitalized.

I factored this function out from some other part
of the program (copying the comment), but I don't understand why it is there.

[sagetrac-git]

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

33f6805	consistent capitalization
59e245d	fixed test output in polyhedron/plot.py example
20a7c00	fixes suggested by reviewer

[sagetrac-git]

Changed commit from ddafd37 to 20a7c00

[LaisRast]

comment:31

Replying to @guenterrote:

Fixed. (Anyway, what is the purpose of that test? Somebody adds a line to the beginning
of the tachyon output, then that test must be updated.)

I agree.

I have now increased the indentation of the warning to 4 (extra) spaces. The result looks the same as before.

Yes, the result looks the same. It is just a general convention to use 4 spaces.

The tickets looks go to me, and thus I will put it on positive review once the patchbot finishes.

[vbraun]

Changed branch from u/guenterrote/variable_camera_position__light__etc__for_tachyon_ray_tracer to 20a7c00