Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Three.js: Cleanup in preparation for adding camera viewpoint option #29250

Closed
paulmasson mannequin opened this issue Feb 27, 2020 · 14 comments
Closed

Three.js: Cleanup in preparation for adding camera viewpoint option #29250

paulmasson mannequin opened this issue Feb 27, 2020 · 14 comments

Comments

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Feb 27, 2020

Before commandeering #29192 to add a new option to set the camera viewpoint, there are several points of cleanup that should be addressed:

  1. In the file plot3d/base.pyx the dictionary js_options is created a number of lines distant from where options are actually set. This struck me as double maintenance of lists, so I moved creation of this dictionary up in the file and set its entries immediately. This also makes explicit in one spot which options are being sent to the Three.js template.

  2. The variable names aspect_ratio and axes_labels are currently carried over into the Three.js template. Since camel case is more common for JavaScript people, I renamed these in the template.

  3. A couple CSS improvements inspired by the current branch of MR41: Add option to persist camera state to three.js viewer. #29192 are included here

  4. The data for the viewpoint is overly precise: no one needs ten decimal places to set orientations in a graphic. The numerical values have been rounded to sufficient values.

This cleanup should be merged separately from #29192 for clarity.

CC: @egourgoulhon @jcamp0x2a

Component: graphics

Keywords: threejs

Author: Paul Masson

Branch/Commit: bc4c440

Reviewer: Joshua Campbell

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

@paulmasson paulmasson mannequin added this to the sage-9.1 milestone Feb 27, 2020
@paulmasson paulmasson mannequin added c: graphics labels Feb 27, 2020
@paulmasson
Copy link
Mannequin Author

paulmasson mannequin commented Feb 27, 2020

Branch: u/paulmasson/29192-prep

@paulmasson
Copy link
Mannequin Author

paulmasson mannequin commented Feb 27, 2020

Commit: bfa9a30

@paulmasson
Copy link
Mannequin Author

paulmasson mannequin commented Feb 27, 2020

comment:2

Eric, if you have a notebook that stress-test overall options, then please test this ticket with it. I'm quite sure I caught all the options, but a second set of eyes is always useful.

Joshua, after this ticket is merged I'll push a branch to #29192 to add the camera viewpoint setting. Please make sure you have all of your original code before I overwrite the public branch!


New commits:

bfa9a30Cleanup in preparation for #29192

@paulmasson paulmasson mannequin added the s: needs review label Feb 27, 2020
@jcamp0x2a
Copy link
Mannequin

jcamp0x2a mannequin commented Feb 27, 2020

comment:3

Much appreciated, Paul; the viewpoint option will be quite nice to have. I've got the original code for the branch so no worries. I'm almost 100% sure I'll have a conflict in plot3d/base.pyx and perhaps elsewhere with my animation work, so I'll rebase off your work before requesting the animation stuff get merged in.

@jcamp0x2a
Copy link
Mannequin

jcamp0x2a mannequin commented Mar 2, 2020

comment:4

Just finished testing these changes myself without issue so you have my vote. :)

One question I do have, though: you added some rounding with the "get viewpoint" data and also removed a space in the resulting string. I'm wondering if maybe someone is feeding this info into a script, program, or database and the formatting changes might break that?

@paulmasson
Copy link
Mannequin Author

paulmasson mannequin commented Mar 2, 2020

comment:5

Replying to @jcamp0x2a:

One question I do have, though: you added some rounding with the "get viewpoint" data and also removed a space in the resulting string. I'm wondering if maybe someone is feeding this info into a script, program, or database and the formatting changes might break that?

The format of the viewpoint data is set by this tutorial using tikz. It's all copy and paste right now.

@paulmasson
Copy link
Mannequin Author

paulmasson mannequin commented Mar 2, 2020

comment:6

Need to redo ticket thanks to #21785

@paulmasson paulmasson mannequin added s: needs work and removed s: needs review labels Mar 2, 2020
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 3, 2020

Changed commit from bfa9a30 to bc4c440

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 3, 2020

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

bc4c440Cleanup in preparation for #29192

@paulmasson
Copy link
Mannequin Author

paulmasson mannequin commented Mar 3, 2020

comment:8

Should now be good to go, gentlemen. Please review!

@paulmasson paulmasson mannequin added s: needs review and removed s: needs work labels Mar 3, 2020
@jcamp0x2a
Copy link
Mannequin

jcamp0x2a mannequin commented Mar 3, 2020

comment:9

I went through and tested the various viewer options and the "get viewpoint" menu button again and didn't run into any issues.

@paulmasson
Copy link
Mannequin Author

paulmasson mannequin commented Mar 3, 2020

comment:10

Joshua, if you are satisfied with this ticket then enter your full name as a reviewer and set it to positive review.

@jcamp0x2a
Copy link
Mannequin

jcamp0x2a mannequin commented Mar 3, 2020

Reviewer: Joshua Campbell

@vbraun
Copy link
Member

vbraun commented Mar 5, 2020

Changed branch from u/paulmasson/29192-prep to bc4c440

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant