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

MR41: Add option to persist camera state to three.js viewer. #29192

Closed
sagetrac-gitlab-bot mannequin opened this issue Feb 13, 2020 · 33 comments
Closed

MR41: Add option to persist camera state to three.js viewer. #29192

sagetrac-gitlab-bot mannequin opened this issue Feb 13, 2020 · 33 comments

Comments

@sagetrac-gitlab-bot
Copy link
Mannequin

sagetrac-gitlab-bot mannequin commented Feb 13, 2020

Joshua Campbell (@jcamp0x2a) opened a merge request at https://gitlab.com/sagemath/sage/-/merge_requests/41:


At present, when using [interact](https://wiki.sagemath.org/interact) with three.js plots in the Jupyter notebook, the camera gets reset to its default position each time the plot is regenerated. If the default camera position isn't ideal for whatever is being plotted, it's cumbersome to have to re-adjust the camera after every change.

This MR adds a boolean `persist` viewing option for three.js that will persist the camera state between viewings of the plot using local storage. In a Jupyter notebook, it's keyed off of the current notebook and cell; otherwise the URL. It defaults to `False` so as not to interfere with the existing behavior that users may rely upon.

Without persist, if you don't like where you've moved the camera to, it's easy enough to reload the page or regenerate the plot to get back to the default. With persist, however, that becomes more difficult; so this MR also adds a "Reset Camera" feature to the bottom-right pop-up menu . (+ some minor hover highlighting and cursor fixes to the menu)

Depends on #28672
Depends on #29250

CC: @paulmasson

Component: graphics

Keywords: threejs camera persist

Author: Paul Masson

Branch/Commit: u/paulmasson/mrs/41/threejs-persist-camera @ 3b7fc0c

Reviewer: Joshua Campbell

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

@sagetrac-gitlab-bot sagetrac-gitlab-bot mannequin added this to the sage-9.1 milestone Feb 13, 2020
@jcamp0x2a

This comment has been minimized.

@jcamp0x2a
Copy link
Mannequin

jcamp0x2a mannequin commented Feb 13, 2020

Changed keywords from none to threejs camera persist

@jcamp0x2a jcamp0x2a mannequin added t: enhancement and removed p: major / 3 labels Feb 13, 2020
@novoselt
Copy link
Member

comment:3

I wonder if it would be better to actually change default behaviour to be persistent. If there is an easy to discover way to reset the camera, I don't see any drawbacks.

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Feb 13, 2020

Dependencies: #28672

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Feb 13, 2020

comment:4

Joshua, while I certainly appreciate all of the effort you put into this feature, there is a much simpler approach. #28672 already makes the camera viewpoint available on the clipboard. This can simply be passed into the plot through a new keyword named viewpoint which will keep the camera at that set position on regeneration. That's how I'm setting the camera in my own JavaScript library that embeds virtually the same viewer in web pages.

While this approach requires a manual step, it avoids the overhead of local storage and the dependence you note on the future structure of Jupyter notebooks. As soon as #28672 is merged, then either you or I can add the new keyword. Please be aware that I handle aspect multipliers a bit differently in my library. I left them more explicit in this viewer so that they don't get forgotten.

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

jcamp0x2a mannequin commented Feb 14, 2020

comment:5

Hi Paul!

Yes, a viewpoint option does seem like it would solve the use case I mentioned (keeping a fixed camera with interact).

Another reason I implemented this feature is because I've been experimenting with adding support for three.js-based animation to Sage: discrete keyframes like the existing animate function at present, with the goal of adding some interpolation or image morphing between the frames later on. I'd like to be able to persist the playback state of the animation as well, preferably tied to the same persist option.

Laying the foundation with a persistent camera first seemed the way to go. With the animation state changing on its own every frame, a manual approach similar to viewpoint would be more cumbersome.

I'm open to scrapping persistence for the camera and re-introducing it for the animation. Of course, that presumes that the animation stuff gets accepted later on; there may be better alternatives that I'm unaware of. Or perhaps a hybrid approach: if a viewpoint has been provided, then ignore persist as regards the initial camera state?

Thanks for taking a look at this. :)

@jcamp0x2a
Copy link
Mannequin

jcamp0x2a mannequin commented Feb 14, 2020

comment:6

Replying to @novoselt:

I wonder if it would be better to actually change default behaviour to be persistent. If there is an easy to discover way to reset the camera, I don't see any drawbacks.

I don't speed a lot of time in the Jupyter notebook, preferring the console, except recently in order to use interact, so I wasn't sure what the commons workflows in it might be and whether this change could interfere with them. So, I opted for the cautious route.

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Feb 14, 2020

comment:7

Joshua, just took a look at your animation branch on GitLab. Looks like you're deep into it! Are there any active examples online? And don't forget that Three.js has an animation system already, which you don't appear to be using.

Although keyframes are one way to approach the topic, keep in mind that we can manipulate data directly using JavaScript. We haven't yet added a basic spin animation, but that's easily done by updating the rotation parameters of the object. If the object is to be moved along a trajectory, we could pass in a line or function describing the location of its center over time. For more complex alterations, we could pass in a function describing the locations of all vertices over time. Lots of possibilities without thinking in terms of saving a movie frame by frame.

Please also try not to add anything to the viewer that will be dependent on a given environment, in this case a Jupyter notebook. I'd rather the viewer be as portable as possible.

@jcamp0x2a
Copy link
Mannequin

jcamp0x2a mannequin commented Feb 14, 2020

comment:8

Paul, I gathered the examples I've put in my documentation so far and thrown them up on GitHub pages:

https://jcamp0x2a.github.io/threejs-animation-example/

I opted not to use Three.js's animation system since I wanted to support multiple "times", one per variable being animated. Three.js would allow me to use multiple mixers to blend linearly between several sets of keyframes, but that wouldn't be appropriate for plotting a function f(x,y,...) that's not some linear combination c1f1(x)+c2f2(y)+...

Agreed that keyframes aren't necessarily the best approach, but it has two enourmous benefits:

  1. It was quite easy to work into the existing Graphics3d system. No need to, for example, go into the implicit_plot3d code and modify the algorithm (marching cubes I think) to produce a mesh suitable for morphing. Nor worrying about topological changes like holes forming or components pinching off. I love math, but I'm a software developer, not a mathematician! So that kind of stuff is beyond my paygrade. :) Instead, I just plot the keyframes, sum them all up into a single Graphics3d object, attach some metadata as to when each object should be visible, and voila!: animation.

  2. It's very general: it can animate a rigidbody moving along a trajectory up to a vector field representing fluid flow with the same amount of code on both the back- and front-ends.

Perhaps I should create a new ticket for the animation to continue the conversation there? I'll hold off on the persistence stuff then if it's not desired.

Thanks!

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Feb 14, 2020

comment:9

Joshua, the animations are great! Definitely new ticket for that. Will study the code more in the near future.

Rather than inspecting the notebook for a unique ID, why not generate a random large number? That's what I've done in my library and haven't had any problems yet, even with pages with lots of embedded content.

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Feb 27, 2020

Changed dependencies from #28672 to #28672 #29250

@mkoeppe
Copy link
Member

mkoeppe commented Apr 14, 2020

comment:11

Batch modifying tickets that will likely not be ready for 9.1, based on a review of the ticket title, branch/review status, and last modification date.

@mkoeppe mkoeppe modified the milestones: sage-9.1, sage-9.2 Apr 14, 2020
@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Jun 9, 2020

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Jun 10, 2020

Changed author from Joshua Campbell to Paul Masson

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Jun 10, 2020

comment:13

Joshua, here at long last is the viewpoint option we discussed above. Use the 'Get Viewpoint' menu option to obtain the value that is passed as a string in the graphic object.


New commits:

66d3471Add viewpoint option

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Jun 10, 2020

Changed commit from fcb6ed4 to 66d3471

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

jcamp0x2a mannequin commented Jun 11, 2020

comment:14

Hi Paul, I've verified that setting the viewpoint option to a string containing the output of Get Viewpoint correctly re-creates that camera orientation in the new plot.

I'm wary of passing structured data like this as a string parameter, though. It works fine for the Get Viewpoint use case, but one may imagine wanting to set the viewpoint to a specific axis-angle or have the axis-angle computed in some way. In the former case a typo (like forgetting a comma) would produce a blank plot instead of a syntax error, and in the latter case you'd have to take the extra step of building up a string.

As Get Viewpoint is currently implemented, we have to surround its output with something to avoid a syntax error, but why not parentheses instead of quotes? ([x,y,z],angle) opposed to "[x,y,z],angle". That way Python can validate the syntax, and we can validate the types/contents.

While testing, I forgot to surround the output with quotes a... non-zero... number of times. Ideally, and I apologize for not thinking of this when reviewing the code that introduced it, Get Viewpoint would do the surrounding for us to avoid this perhaps not uncommon occurrence among some subset of users ;)

Also, do we want the zoom / camera distance to be included as well? Perhaps by letting the length of the axis vector represent the zoom/distance?

Many thanks!

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Jun 11, 2020

comment:15

The Get Viewpoint feature was added to support how TikZ interfaces with Sage: see this tutorial for more information. See also the general rotate command in the base class for 3D objects. Adding anything else to the string here might require changes to other interfaces, so I don't think we should do that.

I agree that requiring a structured string for input is not ideal, and I like the idea of using parentheses or square brackets. That is a better way to structure the input for later handling and either can be used in Python. I'll put together a version for testing.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 12, 2020

Changed commit from 66d3471 to 37b0c87

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 12, 2020

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

37b0c87Update viewpoint option

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Jun 12, 2020

comment:17

Here's a more stable version, where viewpoint must be a list of the form [[x,y,z],angle] or it will raise a warning. Couple questions:

  1. Lots of Sage inputs are allowed to be either lists or tuples, but need to be converted to lists for the JSONing lines. Do we want to allow tuples here?

  2. The angle value for TikZ is in degrees, while the angle for the base-class rotate() is in radians. Not sure how to reconcile the two. Maybe just document what's happening?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 12, 2020

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

f96ccdcFurther refine viewpoint option

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 12, 2020

Changed commit from 37b0c87 to f96ccdc

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Jun 12, 2020

comment:19

Added support for tuples and clarified angle variable in documentation.

Let's add the zoom feature once we get some feedback from users.

@jcamp0x2a
Copy link
Mannequin

jcamp0x2a mannequin commented Jun 12, 2020

comment:20

I checked that the viewpoint is still set correctly, that lists/tuples are both supported, and that a helpful warning message is displayed if the viewpoint option is not of the correct form.

One very minor note: the documentation still states "string of the form...". Otherwise looks good and can give positive review.

Thank you for adding this option. Will make using interact in notebooks with three.js plots much more convenient.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 12, 2020

Changed commit from f96ccdc to 850c59a

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 12, 2020

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

850c59aUpdate documentation

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 12, 2020

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

3b7fc0cUpdate documentation

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 12, 2020

Changed commit from 850c59a to 3b7fc0c

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Jun 12, 2020

comment:23

Replying to @jcamp0x2a:

One very minor note: the documentation still states "string of the form...". Otherwise looks good and can give positive review.

Done!

@jcamp0x2a
Copy link
Mannequin

jcamp0x2a mannequin commented Jun 12, 2020

comment:24

Thanks!

@jcamp0x2a
Copy link
Mannequin

jcamp0x2a mannequin commented Jun 12, 2020

Reviewer: Joshua Campbell

@mkoeppe
Copy link
Member

mkoeppe commented Jul 8, 2020

comment:26

These have been merged into 9.2.beta4

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

2 participants