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: sage-9.4
Component: graphics Keywords: camera light tachyon
Cc: egourgoulhon, gh-mjungmath, 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:

Status badges

Description

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

Change History (32)

comment:1 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:2 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:3 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:4 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:5 Changed 4 months ago by guenterrote

  • Branch set to u/guenterrote/variable_camera_position__light__etc__for_tachyon_ray_tracer

comment:6 Changed 3 months ago by git

  • Commit set to f674b1483dfd32e27807278c6e4f20d6321d1a2f

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

eca2294fix omitted javascript snippets
f674b14renamed camera_center -> camera_position

comment:7 Changed 3 months ago by git

  • Commit changed from f674b1483dfd32e27807278c6e4f20d6321d1a2f to dd63ab9f64f3924dafc57e735e5c679924c13d68

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

4e85f34some fixed, still need to check documentation
dd63ab9fixed the documentation

comment:8 Changed 3 months ago by guenterrote

  • 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 git

  • Commit changed from dd63ab9f64f3924dafc57e735e5c679924c13d68 to b3c37a0e313a31e0cc0002299701bd0febd68128

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

b3c37a0documentation one more sentence about different viewers

comment:10 Changed 3 months ago by guenterrote

  • 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 guenterrote

  • 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:

5413afdinserting camera parameters for tachyon and fixing other parameters
c29270aoption to retrieve camera position from threejs after rotation; requires still more post-processing
eca2294fix omitted javascript snippets
f674b14renamed camera_center -> camera_position
4e85f34some fixed, still need to check documentation
dd63ab9fixed the documentation
b3c37a0documentation one more sentence about different viewers

comment:12 Changed 3 months ago by guenterrote

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

Last edited 6 weeks ago by guenterrote (previous) (diff)

comment:13 Changed 6 weeks ago by slelievre

Please add full name in author field.

comment:14 Changed 6 weeks ago by guenterrote

  • Authors set to Günter Rote

comment:15 Changed 6 weeks ago by git

  • Commit changed from b3c37a0e313a31e0cc0002299701bd0febd68128 to 0de3ada2f12daf0c164a5d15516272b6f9e9737e

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

bfd3909typo in documentation
0de3adasimplify optimal_extra_kwds; added some tests

comment:16 Changed 6 weeks ago by git

  • Commit changed from 0de3ada2f12daf0c164a5d15516272b6f9e9737e to 71063b177b0f1b0e2b56b9d8b63738f99d173ff0

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

71063b1typos

comment:17 Changed 6 weeks ago by mkoeppe

  • Cc egourgoulhon gh-mjungmath jipilab added
  • Milestone changed from sage-6.4 to sage-9.3

comment:18 Changed 6 weeks ago by vdelecroix

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 mkoeppe

  • Milestone changed from sage-9.3 to sage-9.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 git

  • Commit changed from 71063b177b0f1b0e2b56b9d8b63738f99d173ff0 to 18e8aa635b74f92ab5605df8cebb3331b6ef53c8

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

82384b8eliminated double word
000f272grammar correction
18e8aa6small grammar fix, eliminated references to ``self`` from the doc.

comment:21 Changed 6 weeks ago by git

  • Commit changed from 18e8aa635b74f92ab5605df8cebb3331b6ef53c8 to befe2d7c8c5bb6729f305fae52e808c75865ad52

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

1e01655grammar fix
befe2d7improve wording

comment:22 follow-ups: Changed 6 weeks ago by gh-LaisRast

  • 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 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;
    

comment:23 in reply to: ↑ 22 Changed 6 weeks ago by guenterrote

Replying to gh-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.)

comment:24 in reply to: ↑ 22 Changed 6 weeks ago by guenterrote

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

comment:25 Changed 6 weeks ago by git

  • Commit changed from befe2d7c8c5bb6729f305fae52e808c75865ad52 to ddafd377ca37d9bc3c3b13586bf11a8bbfa7724e

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

aac0134eliminated remainders of camera_center
ddafd37eliminated remainders of preliminary parameter name camera_center

comment:26 Changed 6 weeks ago by guenterrote

  • Status changed from needs_work to needs_review

comment:27 follow-up: Changed 6 weeks ago by gh-LaisRast

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

Last edited 6 weeks ago by gh-LaisRast (previous) (diff)

comment:28 in reply to: ↑ 27 ; follow-up: Changed 6 weeks ago by guenterrote

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

comment:29 Changed 6 weeks ago by git

  • Commit changed from ddafd377ca37d9bc3c3b13586bf11a8bbfa7724e to 20a7c0015598da05be5f344bc31d1489b71758fb

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

33f6805consistent capitalization
59e245dfixed test output in polyhedron/plot.py example
20a7c00fixes suggested by reviewer

comment:30 Changed 6 weeks ago by guenterrote

  • Status changed from needs_work to needs_review

comment:31 in reply to: ↑ 28 Changed 6 weeks ago by gh-LaisRast

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 gh-LaisRast

  • Status changed from needs_review to positive_review
Note: See TracTickets for help on using tickets.