jupyter-widgets / pythreejs

A Jupyter - Three.js bridge
https://pythreejs.readthedocs.io
Other
942 stars 188 forks source link

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

Closed maartenbreddels closed 6 years ago

maartenbreddels commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

The generated python code for Scene gives me:

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

is that expected?

vidartf commented 6 years ago

is that expected

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

vidartf commented 6 years ago

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 commented 6 years ago

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

vidartf commented 6 years ago

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

maartenbreddels commented 6 years ago

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 commented 6 years ago

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

vidartf commented 6 years ago

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

maartenbreddels commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

I would say transparent is a bad default:

vidartf commented 6 years ago

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

vidartf commented 6 years ago

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 commented 6 years ago

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

maartenbreddels commented 6 years ago

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 commented 6 years ago

Are you saying this isn't enough? 😅 http://pythreejs.readthedocs.io/en/latest/api/scenes/Scene_autogen.html#pythreejs.Scene.background

maartenbreddels commented 6 years ago

Ok, that will do :)