Opened 9 years ago
Last modified 6 weeks ago
#13111 positive_review enhancement
variable camera position, light, etc. for tachyon ray tracer
Reported by:  dkrenn  Owned by:  jason, was 

Priority:  major  Milestone:  sage9.4 
Component:  graphics  Keywords:  camera light tachyon 
Cc:  egourgoulhon, ghmjungmath, jipilab  Merged in:  
Authors:  Günter Rote  Reviewers:  Laith Rastanawi 
Report Upstream:  N/A  Work issues:  
Branch:  u/guenterrote/variable_camera_position__light__etc__for_tachyon_ray_tracer (Commits, GitHub, GitLab)  Commit:  20a7c0015598da05be5f344bc31d1489b71758fb 
Dependencies:  Stopgaps: 
Description
When using the tachyon ray tracer, at the moment the parameters for the camera, as well as for the light and for the backgroundplane are hardcoded 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
).
Change History (32)
comment:1 Changed 8 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:2 Changed 7 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:3 Changed 7 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:4 Changed 7 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:5 Changed 4 months ago by
 Branch set to u/guenterrote/variable_camera_position__light__etc__for_tachyon_ray_tracer
comment:6 Changed 3 months ago by
 Commit set to f674b1483dfd32e27807278c6e4f20d6321d1a2f
comment:7 Changed 3 months ago by
 Commit changed from f674b1483dfd32e27807278c6e4f20d6321d1a2f to dd63ab9f64f3924dafc57e735e5c679924c13d68
comment:8 Changed 3 months ago by
 Status changed from new to needs_review
I fixed the tachyon viewer. (I did not touch the other viewers, although there would be some room for improvements too)
comment:9 Changed 3 months ago by
 Commit changed from dd63ab9f64f3924dafc57e735e5c679924c13d68 to b3c37a0e313a31e0cc0002299701bd0febd68128
Branch pushed to git repo; I updated commit sha1. New commits:
b3c37a0  documentation one more sentence about different viewers

comment:10 Changed 3 months ago by
 Branch changed from u/guenterrote/variable_camera_position__light__etc__for_tachyon_ray_tracer to public/plot3d/variable_camera_position__light__etc__for_tachyon_ray_tracer
 Commit b3c37a0e313a31e0cc0002299701bd0febd68128 deleted
comment:11 Changed 3 months ago by
 Branch changed from public/plot3d/variable_camera_position__light__etc__for_tachyon_ray_tracer to u/guenterrote/variable_camera_position__light__etc__for_tachyon_ray_tracer
 Commit set to b3c37a0e313a31e0cc0002299701bd0febd68128
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 postprocessing

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

comment:12 Changed 3 months ago by
This is the longawaited 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 socalled "wavefront"
viewer. Is that obsolete?)
comment:13 Changed 6 weeks ago by
Please add full name in author field.
comment:14 Changed 6 weeks ago by
comment:15 Changed 6 weeks ago by
 Commit changed from b3c37a0e313a31e0cc0002299701bd0febd68128 to 0de3ada2f12daf0c164a5d15516272b6f9e9737e
comment:16 Changed 6 weeks ago by
 Commit changed from 0de3ada2f12daf0c164a5d15516272b6f9e9737e to 71063b177b0f1b0e2b56b9d8b63738f99d173ff0
Branch pushed to git repo; I updated commit sha1. New commits:
71063b1  typos

comment:17 Changed 6 weeks ago by
 Cc egourgoulhon ghmjungmath jipilab added
 Milestone changed from sage6.4 to sage9.3
comment:18 Changed 6 weeks ago by
This is not proper english
The most important facts about these classes are hat you can add simply add graphics objects together
comment:19 Changed 6 weeks ago by
 Milestone changed from sage9.3 to sage9.4
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.
comment:20 Changed 6 weeks ago by
 Commit changed from 71063b177b0f1b0e2b56b9d8b63738f99d173ff0 to 18e8aa635b74f92ab5605df8cebb3331b6ef53c8
comment:21 Changed 6 weeks ago by
 Commit changed from 18e8aa635b74f92ab5605df8cebb3331b6ef53c8 to befe2d7c8c5bb6729f305fae52e808c75865ad52
comment:22 followups: ↓ 23 ↓ 24 Changed 6 weeks ago by
 Status changed from needs_review to needs_work
All of these comments are related to the parameter camera_center
:
 In
plot/base.pyz
, two examples still use the old parametercamera_center
. However it will be ignored since it is not intachyon_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 onGet 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 whatCopy Viewpoint
does. textArea.textContent = ',camera_center=' + cam_position; + textArea.textContent = cam_position;
comment:23 in reply to: ↑ 22 Changed 6 weeks ago by
Replying to ghLaisRast:
 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.)
comment:24 in reply to: ↑ 22 Changed 6 weeks ago by
Replying to ghLaisRast:
 In
threejs_template.html
, when clicking onGet 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 whatCopy 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.
comment:25 Changed 6 weeks ago by
 Commit changed from befe2d7c8c5bb6729f305fae52e808c75865ad52 to ddafd377ca37d9bc3c3b13586bf11a8bbfa7724e
comment:26 Changed 6 weeks ago by
 Status changed from needs_work to needs_review
comment:27 followup: ↓ 28 Changed 6 weeks ago by
 Reviewers set to Laith Rastanawi
 Status changed from needs_review to needs_work
 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 nonuniform 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 nonuniform 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.
comment:28 in reply to: ↑ 27 ; followup: ↓ 31 Changed 6 weeks ago by
Replying to ghLaisRast:
 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 nonuniform 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.
comment:29 Changed 6 weeks ago by
 Commit changed from ddafd377ca37d9bc3c3b13586bf11a8bbfa7724e to 20a7c0015598da05be5f344bc31d1489b71758fb
comment:30 Changed 6 weeks ago by
 Status changed from needs_work to needs_review
comment:31 in reply to: ↑ 28 Changed 6 weeks ago by
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.
comment:32 Changed 6 weeks ago by
 Status changed from needs_review to positive_review
Branch pushed to git repo; I updated commit sha1. New commits:
fix omitted javascript snippets
renamed camera_center > camera_position