glotzerlab / plato

Efficient visualization of particle data supporting several rendering engines.
https://plato-draw.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
12 stars 4 forks source link

pythreejs backend not replicating properties #11

Open bdice opened 5 years ago

bdice commented 5 years ago

When using the pythreejs backend, only the first shape of a primitive is drawn, and it's solid black. I saw this issue with @k-l-wang the other day and it's occurring on my machine too.

Version information:

Code to reproduce, in a Jupyter notebook:

import plato.draw.fresnel as draw
import numpy as np
positions = np.zeros((10, 3))
positions[:, 0] = np.arange(10)
positions -= np.mean(positions, axis=0)
spheres = draw.Spheres(positions=positions)
scene = draw.Scene(spheres)
scene.show()

fresnel backend output: image

pythreejs backend output (it appears to be drawing either the first or the last shape - the one on the right side is drawn in this case). image

bdice commented 5 years ago

The shape attributes aren't being replicated properly, and the lights are disabled unless users specifically request lights. This code snippet solves both problems, but I think both behaviors are bugs.

import plato.draw.pythreejs as draw
import numpy as np
positions = np.zeros((10, 3))
positions[:, 0] = np.arange(10)
positions -= np.mean(positions, axis=0)
# Add full arrays for colors and radii or else only one particle shows up
colors = np.array([[1, 0, 0, 1]]*len(positions))
radii = np.array([0.5]*len(positions))
spheres = draw.Spheres(positions=positions, colors=colors, radii=radii)
scene = draw.Scene(spheres)
# Add lights
#scene.enable('ambient_light', 0.0)
scene.enable('directional_light')
scene.show()

Result: image

The replication problem can be seen by investigating these arrays. They should be (Nparticles * Nvertices, width) where width is 4 for RGBA colors and 3 for normal vectors. In this case, with 10 particles and 64 vertices in the sphere mesh, the shapes should be (640, 4) and (640, 3). Since the values are not being replicated, they are (64, 4) and (64, 3) instead.

color = spheres.threejs_primitive.geometry.attributes['color'].array
normal = spheres.threejs_primitive.geometry.attributes['normal'].array

Some lights should be enabled by default, as they are for other backends.

bdice commented 5 years ago

It seems like the failure to replicate comes from mesh.unfoldProperties. @klarh It was my understanding that if a single value was provided for colors or radii, it would be replicated automatically. At least, several backends act that way. Do inputs need to be pre-processed so that len(colors) = len(radii) = len(positions) before calling mesh.unfoldProperties?

klarh commented 5 years ago

@bdice using the size of the smallest array you're given is the expected behavior for mesh.unfoldProperties at the moment. I would guess that any backends in which the default values get replicated automatically are getting lucky in the way they were written with respect to numpy broadcasting rules.

I think it would be a great feature to make things like colors, orientations, and diameters be automatically broadcast (see glotzerlab/platoviz#44), but it's a little more complicated because properties can be set (or not) in any order, often they should not be checked until the next draw call is triggered, and sometimes people do want to draw just one thing (in which case we wouldn't want to broadcast).

As a separate issue, I'm all for setting up a default lighting scheme for the pythreejs backend in the name of getting something decent drawn on the screen with a minimal amount of code (so the light feature isn't necessary in this case). Default lights were added in the vispy backend in 33d7b8164c1055a9339de83d248840bcc43d81ce if you want to use the same directions and relative intensities.

bdice commented 5 years ago

@klarh I've spent about 4 hours trying (and failing) to find a reasonable way to make non-position properties replicate/tile out to the number of shapes by default. I can't figure out a good way to do it without duplicating a lot of code and/or breaking the existing patterns for data storage and class inheritance. I would welcome your thoughts.

klarh commented 5 years ago

@bdice I don't think it would be particularly straightforward, but maybe setting properties with dimension D-1 from what they are expecting would indicate that it should be automatically broadcast? The value would need to be immediately dispatched in the setter function and specially dealt with in the update_arrays (vispy)/render (povray)/equivalent functions in the various backends.