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

Scene.background default not null/None and None does not translate to null on the frontend #176

Closed
maartenbreddels opened this issue Apr 11, 2018 · 19 comments

Comments

@maartenbreddels
Copy link
Member

According to the docs the default background for a Scene should be null. However, when I explicitly set it to None, it translates to sth non-null on the frontend:
screen shot 2018-04-11 at 10 37 14

@vidartf
Copy link
Member

vidartf commented Apr 11, 2018

The current definition of Scene.background is simply a color. It is currently marked as nullable, but I think that only causes the default of THREE.Color() to be used.

@maartenbreddels
Copy link
Member Author

maartenbreddels commented Apr 11, 2018

The generated python code for Scene gives me:

background = Unicode("#ffffff", allow_none=True).tag(sync=True)

is that expected?

@vidartf
Copy link
Member

vidartf commented Apr 11, 2018

is that expected

Yes, but the JS side is wrong. I'm working on a fix.

@vidartf
Copy link
Member

vidartf commented Apr 11, 2018

PS: It is a Unicode trait in order to support all the different strings for THREE.Color: https://threejs.org/docs/#api/math/Color

@vidartf
Copy link
Member

vidartf commented Apr 11, 2018

By the way, what are you trying to achieve by setting the background to None?

@vidartf
Copy link
Member

vidartf commented Apr 11, 2018

If you are trying to achieve a transparent background, you will also need to set the clearOpacity of the renderer to 0.

@maartenbreddels
Copy link
Member Author

Well, in ipyvolume I created my own scene, now that I'm using a scene created from pythreejs, it has a background color, which causes (I think) in one of my render passes to clear the framebuffer with a white color. Before it didn't do that, so that's why I noticed.

@vidartf
Copy link
Member

vidartf commented Apr 11, 2018

Oh, you're using your own renderer, so then the pythreejs renderer is not relevant.

@vidartf
Copy link
Member

vidartf commented Apr 11, 2018

@maartenbreddels Let me know if #177 fixed the problem or not!

@maartenbreddels
Copy link
Member Author

Yes, background is null now, but the generated code still has background's default set to '#ffffff', but you said that is expected, I don't understand why? And if the threejs docs says null is the default, shouldn't the default be None for the Python side?

@vidartf
Copy link
Member

vidartf commented Apr 12, 2018

The generated code is expected based on the configuration linked.

The default was set to white before I got null support, but was kept as a jupyter specific change as that is the default background color of notebooks. When set to null, the default background would be the clear color which defaults to black. Does the default white make sense, or do you think it should be set back to None?

@maartenbreddels
Copy link
Member Author

I'd say, we should have a default that is transparant, so it uses the background of whatever the theme is. That would mean we'd have to have a non-default alpha=True for the Renderer .. What do you think?

@vidartf
Copy link
Member

vidartf commented Apr 12, 2018

I would say transparent is a bad default:

  • Adding the alpha channel has a (small?) performance penalty
  • Depending on the theme, the background might combine poorly with the colors in the scene, e.g. black text/grids on a black background.

@vidartf
Copy link
Member

vidartf commented Apr 12, 2018

Regarding the last point, see the discussions on jupyterlab repo about how MPL figures clash with lab themes.

@vidartf
Copy link
Member

vidartf commented Apr 12, 2018

PS: If you want to use theme colors in widgets, I would recommend setting up a library that defines color constants based on CSS vars. These can then be dynamically calculated on the JS side. E.g.:

jslink((scene, 'background'), (themecolors, '--jp_layout_color0'))

@vidartf
Copy link
Member

vidartf commented Apr 12, 2018

(I'm not sure if that syntax is realizable or not)

@maartenbreddels
Copy link
Member Author

I'd say what seems like a good default for now, but I think that jslink example is something to think about. Maybe mention the white default in the docs, as deviating from the defaults in threejs?

@vidartf
Copy link
Member

vidartf commented Apr 12, 2018

@maartenbreddels
Copy link
Member Author

Ok, that will do :)

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

No branches or pull requests

2 participants