Opened 2 years ago

Closed 2 years ago

#30462 closed enhancement (fixed)

Dark theme for Three.js viewer

Reported by: Joshua Campbell Owned by:
Priority: minor Milestone: sage-9.3
Component: graphics Keywords: threejs dark theme colors background jupyterlab
Cc: Merged in:
Authors: Joshua Campbell Reviewers: Paul Masson
Report Upstream: N/A Work issues:
Branch: b310bfb (Commits, GitHub, GitLab) Commit: b310bfb1ddfd7de18356e068a8716eb7dd449d97
Dependencies: #30246 Stopgaps:

Status badges

Description

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.

Change History (14)

comment:1 Changed 2 years ago by Joshua Campbell

Authors: Joshua Campbell
Branch: u/gh-jcamp0x2a/30462-threejs-dark-theme
Commit: c48a12d832aaa8f7ad534ffe06f7e2d2b1180818
Dependencies: 30246#30246
Status: newneeds_review

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

comment:2 Changed 2 years ago by Paul Masson

Reviewers: Paul Masson

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

comment:3 Changed 2 years ago by git

Commit: c48a12d832aaa8f7ad534ffe06f7e2d2b1180818642ea69a54a5b3a2573a19ed1061eee7ddae050e

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

642ea69Remove unnecessary parentheses in ternary condition

comment:4 in reply to:  2 Changed 2 years ago by Joshua Campbell

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.

comment:5 Changed 2 years ago by git

Commit: 642ea69a54a5b3a2573a19ed1061eee7ddae050ec8a45134dfe919f89656ee902ff19593ebc04cb5

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

c8a4513Minor whitespace fix

comment:6 Changed 2 years ago by Paul Masson

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

comment:7 in reply to:  6 ; Changed 2 years ago by Joshua Campbell

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

comment:8 in reply to:  7 ; Changed 2 years ago by Paul Masson

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

comment:9 Changed 2 years ago by git

Commit: c8a45134dfe919f89656ee902ff19593ebc04cb5b310bfb1ddfd7de18356e068a8716eb7dd449d97

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

b310bfbOnly check for 'black' and '#000000'

comment:10 in reply to:  8 Changed 2 years ago by Joshua Campbell

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.

comment:11 Changed 2 years ago by Paul Masson

Status: needs_reviewpositive_review

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.

comment:12 in reply to:  11 Changed 2 years ago by Joshua Campbell

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.

comment:13 Changed 2 years ago by Matthias Köppe

Milestone: sage-9.2sage-9.3

comment:14 Changed 2 years ago by Volker Braun

Branch: u/gh-jcamp0x2a/30462-threejs-dark-themeb310bfb1ddfd7de18356e068a8716eb7dd449d97
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.