search

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

Fix/pythreejs rotation #28

Closed bdice closed 5 years ago

bdice commented 5 years ago

This PR aims to resolve #26. Currently the scenes are drawn with the correct rotation, but querying scene.rotation gives a wrong answer. This will need to be resolved before merging. I'm quite confused by this issue and would appreciate help.

bdice commented 5 years ago

On 2b0cf5f, I tried to test setting the scene translation to [3, 5, -50] for one of the axis scenes but it doesn't appear to do anything unless scene._update_camera() is called. However, calling scene._update_camera() also changes the camera position and thus changes the output of the getter for scene.rotation.

(Copied from Slack): When _update_camera is called, it does the following:

  1. Compute the scene size and figure out a distance dz for the camera.
  2. Rotates [0, 0, 1 + dz/2] by rowan.conjugate(self.rotation) to compute translation
  3. Shifts translation by the scene translation: translation -= self.translation
  4. Moves the camera to this new position, sets rendering bounds and up vector

BUT then the getter self.rotation relies on this new camera position to compute its rotation quaternion. This is wrong because the camera position has been translated by self.translation already.

Subsequent calls to _update_camera then get the wrong value for self.rotation because they rely on a post-translated camera position.

Possible solution: Use camera.position + self.translation in the getter for self.rotation. However, I don't know how to account for changes introduced by the user panning the camera -- those don't affect self.translation to my knowledge.

Seems like we'll need to reintroduce the line self._update_camera() in the right place, and adjust some math to account for this.

klarh commented 5 years ago

It looks like the target attribute of the controls object contains the translation state of the scene when you control-grab to drag around the view (which is applied before the camera rotation). I think we should be able to use that to properly determine both the orientation and translation of the scene.

bdice commented 5 years ago

@klarh I think this is ready for a final review and merge.