add fitToSphere support for orthographic camera #159
Conversation
| ( this._camera as THREE.PerspectiveCamera ).isPerspectiveCamera ? ACTION.DOLLY : | ||
| ( this._camera as THREE.OrthographicCamera ).isOrthographicCamera ? ACTION.ZOOM : | ||
| isPerspectiveCamera( this._camera ) ? ACTION.DOLLY : | ||
| isOrthographicCamera( this._camera ) ? ACTION.ZOOM : |
There was a problem hiding this comment.
Function call could be slightly costy. Could we just use the property?
There was a problem hiding this comment.
I believe that using the type guards instead of type casting improves code clarity and helps to avoid errors. I'm not quite sure about the function call overhead. https://jsbench.me/lyklgbaglm/1 this extremely simplistic benchmark reports the property being actually 17% slower than function call on Firefox, and both as being equal on Chrome (for me). There is some conflicting information online, apparently function calls are slower on Internet Explorer.
I have rebased the PR such that the commit "add type guards for camera type" is last. Feel free to exclude it if you want to keep using the property.
There was a problem hiding this comment.
Thanks for understanding, and the test😊
improves code clarity and helps to avoid errors
You are right! However, three.js tends to avoid using functions such as mrdoob/three.js#19997 (comment)
therefore, let me revert a9a53c9 ("add type guards for camera type") at this moment.
4c1120c to
9815b4d
Compare
|
Merged. |
|
thanks @ervinoro ! |
Really a very simple change to use zoom instead of dolly in fitToSphere in the case of orthographic camera.
Additionally, it includes a bit of refactoring: