Addition of Field-of-View Hook and switched to CombinedCamera #40
Conversation
not yet implemented switching of combined camera to orthographic
so that it applies for any angle (not just initial z-direction)
js/src/scatter.js
Outdated
| @@ -20,6 +20,7 @@ var ScatterView = widgets.WidgetView.extend( { | |||
|
|
|||
| this.geo_diamond = new THREE.SphereGeometry(1, 2, 2) | |||
| this.geo_sphere = new THREE.SphereGeometry(1, 12, 12) | |||
| this.geo_sphere_hres = new THREE.SphereGeometry(1, 48, 48) | |||
There was a problem hiding this comment.
Can you remove this, to keep this PR cleaner?
js/src/scatter.js
Outdated
| @@ -53,6 +54,7 @@ var ScatterView = widgets.WidgetView.extend( { | |||
| box: this.geo_box, | |||
| arrow: this.geo_arrow, | |||
| sphere: this.geo_sphere, | |||
| sphere_hres:this.geo_sphere_hres, | |||
| @@ -142,7 +150,8 @@ var FigureView = widgets.DOMWidgetView.extend( { | |||
| this.wire_box.add(make_line(-0.5, -0.5+1, -0.5, -0.5, -0.5+1, -0.5+1, this.axes_material)) | |||
| this.wire_box.add(make_line(-0.5+1, -0.5+1, -0.5, -0.5+1, -0.5+1, -0.5+1, this.axes_material)) | |||
|
|
|||
| this.camera.position.z = 2 | |||
| // set a good intial z for any fov angle | |||
| this.camera.position.z = 2 * this.getTanDeg(45/2) / this.getTanDeg(this.model.get("camera_fov")/2) | |||
There was a problem hiding this comment.
What's the idea behind this? Can you link to sth or provide a hint?
There was a problem hiding this comment.
Just some 'back of an envelope' trigonometry:
Since the camera is intialized at (0,0,z) (i.e. a=z) and we know that theta=45 and z=2 provides a good initial view (as is currently hard coded), hence the new code.
Make sense?
There was a problem hiding this comment.
perfect, maybe put a link to this PR.
There was a problem hiding this comment.
(Updated) Maybe add 45 as a constant INITIAL_VIEW_ANGLE (or more descriptive) to avoid magic numbers?
There was a problem hiding this comment.
well, it's not really a fov anymore... it sets the initial distance. I'll think about it.
There was a problem hiding this comment.
Hey vidarf, I'll leave your suggestion to maarten. But, for future knowledge, what do you mean by magic numbers?
There was a problem hiding this comment.
Ah, by magic numbers I mean any number that appears in code without explanation (e.g. 3.14...). These should typically be replaced by constant vars, or at least commented.
There was a problem hiding this comment.
ah ok ta, just wanted to check it wasn't some obscure javascript issue lol
|
Btw, what about the camera license, I guess it's the same as three, since it is in the threejs repo: https://github.com/mrdoob/three.js/blob/master/examples/js/cameras/CombinedCamera.js Also, we could make pythreejs a hard dependency, so we expose the cameras object as pythreejs objects instead. What do you think about that? That would mean that you always need to install pythreejs, which would be an extra possible point of failure, which I don't like. |
|
I copied it directly from there, so I assume its covered by the three.js license, since there is no sub licences in any of the directory path. For the pythreejs integration, I think its a good long term goal. But, I guess, hold off on that until it is closer to a version 1.0 |
|
Yup, I agree. If you remove the high res spheres, happy to merge. |
This reverts commit 5c97a0a.
|
cheers |
|
Thanks for you first contribution! 👍 |
|
Yeh no problem, |
…ti#40) added hook for camera fov, changed camera from perspective to combined not yet implemented switching of combined camera to orthographic
Hows this for a first stab?
camera_fovtrait and also some math so that, when changing fov, you maintain the same zoom level.