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

Dark theme for Three.js viewer #30462

Closed
jcamp0x2a mannequin opened this issue Aug 28, 2020 · 21 comments
Closed

Dark theme for Three.js viewer #30462

jcamp0x2a mannequin opened this issue Aug 28, 2020 · 21 comments

Comments

@jcamp0x2a
Copy link
Mannequin

jcamp0x2a mannequin commented Aug 28, 2020

This ticket proposes introducing a dark theme to the Three.js viewer. The new theme viewer option will be used to control whether the original (and default) 'light' theme or the new 'dark' theme is used.

Aside from appeasing users -- myself included -- who generally prefer dark UIs, the main justification for this feature is that JupyterLab, unlike the Jupyter notebook, allows you to use a dark theme for its UI. When using that theme, however, the contrast between the dark UI and the bright background of Three.js plots can be quite harsh.

Depends on #30246

Component: graphics

Keywords: threejs dark theme colors background jupyterlab

Author: Joshua Campbell

Branch/Commit: b310bfb

Reviewer: Paul Masson

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

@jcamp0x2a jcamp0x2a mannequin added this to the sage-9.2 milestone Aug 28, 2020
@jcamp0x2a jcamp0x2a mannequin added c: graphics labels Aug 28, 2020
@jcamp0x2a
Copy link
Mannequin Author

jcamp0x2a mannequin commented Aug 29, 2020

comment:1

Here are a couple examples:

Points, lines, surfaces, and texts

surface = dodecahedron(mesh=True, color='blue').scale(0.5)
curve = parametric_plot3d([sin(x), cos(x), x/pi], (x, -pi, pi), color='red')
points = point3d([random_vector(RR, 3) for i in range(0, 100)], color='green')
text = text3d("Hello world!", (1, 1, 1), color='yellow')
show(surface + curve + points + text, theme='dark')

Animation controls

animate([dodecahedron().rotateZ(t*pi/32) for t in range(0, 64)]).interactive(delay=5, theme='dark')

@jcamp0x2a
Copy link
Mannequin Author

jcamp0x2a mannequin commented Aug 29, 2020

Commit: c48a12d

@jcamp0x2a
Copy link
Mannequin Author

jcamp0x2a mannequin commented Aug 29, 2020

@jcamp0x2a
Copy link
Mannequin Author

jcamp0x2a mannequin commented Aug 29, 2020

Changed dependencies from 30246 to #30246

@jcamp0x2a
Copy link
Mannequin Author

jcamp0x2a mannequin commented Aug 29, 2020

Author: Joshua Campbell

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

paulmasson mannequin commented Aug 29, 2020

comment:2

Cool idea! Minor quibble: you don't need parentheses around the condition during variable assignment via the ternary operator.

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Aug 29, 2020

Reviewer: Paul Masson

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 29, 2020

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

642ea69Remove unnecessary parentheses in ternary condition

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 29, 2020

Changed commit from c48a12d to 642ea69

@jcamp0x2a
Copy link
Mannequin Author

jcamp0x2a mannequin commented Aug 29, 2020

comment:4

Replying to @paulmasson:

Cool idea! Minor quibble: you don't need parentheses around the condition during variable assignment via the ternary operator.

Good point. I have removed those unnecessary parentheses. Thanks for taking a look at this.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 29, 2020

Changed commit from 642ea69 to c8a4513

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 29, 2020

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

c8a4513Minor whitespace fix

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Aug 31, 2020

comment:6

I can confirm that things work as expected, so almost ready to go.

I don't quite like the code inside addLabel that tests for the color black, since you haven't captured all possible ways to create that color via CSS. A foolproof method would be to create a DIV, set its color with the incoming CSS string and then check the numeric values of its computed style. That's a bit much, however, since anyone trying to use black text in a dark theme is obviously not going to see anything. We could just test for the string 'black', since it's being set internally as the default value, and see how much feedback we get on usage.

Also, why are the lines for testing viewpoint values highlighted? Can't see any changes...

@jcamp0x2a
Copy link
Mannequin Author

jcamp0x2a mannequin commented Aug 31, 2020

comment:7

Replying to @paulmasson:

That's a bit much, however, since anyone trying to use black text in a dark theme is obviously not going to see anything. We could just test for the string 'black', since it's being set internally as the default value, and see how much feedback we get on usage.

Although 'black' is the default in the javascript, I was seeing '#000000' coming in from text3d when not specifying a color, so that's why that's there. The '#000' was a "hey why not?" addition.

I'm not particularly happy with it either in hindsight. I think that if the user explicitly sets the color to black, we should respect that. Same if they set it to white in the normal theme.

Maybe it would make more sense then to default the text3d and javascript colors to None/null and then choose whether to turn that into black/white based on the theme?

Also, why are the lines for testing viewpoint values highlighted? Can't see any changes...

Sorry. It was probably vscode automatically cleaning up whitespace at the end of the line. I enabled all that auto-format stuff at one point, didn't like all the useless changes I was seeing in diffs, and thought I'd disabled it all. Guess not :)

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Sep 1, 2020

comment:8

Replying to @jcamp0x2a:

Replying to @paulmasson:

That's a bit much, however, since anyone trying to use black text in a dark theme is obviously not going to see anything. We could just test for the string 'black', since it's being set internally as the default value, and see how much feedback we get on usage.

Although 'black' is the default in the javascript, I was seeing '#000000' coming in from text3d when not specifying a color, so that's why that's there. The '#000' was a "hey why not?" addition.

I'm not particularly happy with it either in hindsight. I think that if the user explicitly sets the color to black, we should respect that. Same if they set it to white in the normal theme.

Maybe it would make more sense then to default the text3d and javascript colors to None/null and then choose whether to turn that into black/white based on the theme?

I don't think we want to change defaults in the Python code: might affect the current user experience in other viewers. Perhaps we should just check for the defaults we know are being set, 'black' and '!#000000', and let the other formats exist as a way to override this behavior. If you agree then make that minor change and I'll set the ticket to positive.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 1, 2020

Changed commit from c8a4513 to b310bfb

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 1, 2020

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

b310bfbOnly check for 'black' and '#000000'

@jcamp0x2a
Copy link
Mannequin Author

jcamp0x2a mannequin commented Sep 1, 2020

comment:10

Replying to @paulmasson:

I don't think we want to change defaults in the Python code: might affect the current user experience in other viewers. Perhaps we should just check for the defaults we know are being set, 'black' and '!#000000', and let the other formats exist as a way to override this behavior. If you agree then make that minor change and I'll set the ticket to positive.

Agreed...and done! Many thanks.

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Sep 1, 2020

comment:11

Thanks! All set now.

You really seem to be getting into the nuts and bolts of every part of Sage, even the build system. I'm curious as to what motivates such interest. My GitHub profile has an email address if you're willing to share information about your Sage-related projects.

@jcamp0x2a
Copy link
Mannequin Author

jcamp0x2a mannequin commented Sep 2, 2020

comment:12

Replying to @paulmasson:

Thanks! All set now.

You really seem to be getting into the nuts and bolts of every part of Sage, even the build system. I'm curious as to what motivates such interest. My GitHub profile has an email address if you're willing to share information about your Sage-related projects.

Thanks Paul. I appreciate your time in reviewing this ticket.

No particular projects underway. I've been using Sage in my own hobbyist adventures into math & physics, particularly for trying to visualize things and of course to cheat on the integrals in my books' exercise questions :) Figured it'd be nice to contribute back to the project in some way.

@mkoeppe mkoeppe modified the milestones: sage-9.2, sage-9.3 Oct 24, 2020
@vbraun
Copy link
Member

vbraun commented Nov 7, 2020

Changed branch from u/gh-jcamp0x2a/30462-threejs-dark-theme to b310bfb

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