Feature : added new PlanarControls #454

EmmanuelSchmuck · 2017-08-25T12:45:42Z

PlanarControls provides ergonomic camera controls for the planar mode : drag, pan, orbit,
zoom and animated expression to smoothly move and orient the camera.

Modified planar example to use these controls
Update contributors

peppsac

Thanks for the PR and welcome to iTowns project :)

I did a very quick first review.
I (or another reviewer) will have do a more in depth one before merging but the code is looking good mostly and is well documented, nice!

peppsac · 2017-08-25T12:50:02Z

src/Renderer/ThreeExtended/PlanarControls.js

+// time management
+let deltaTime = 0;
+let lastElapsedTime = 0;
+const clock = new THREE.Clock();


How about storing all these inside the PlanarControls instance so a user can instanciate several controls?

peppsac · 2017-08-25T12:51:42Z

src/Renderer/ThreeExtended/PlanarControls.js

+        dragDelta.set(0, 0, 0);
+    };
+
+    /** =========================================================================================================================


I don't mind the ========= separator but this is something that should be done everywhere or nowhere (as all other stylistic choices).
So I'd suggest removing them from this PR and open a specific PR to convert itowns to this style (but I'm not sure it would be merged...)

peppsac · 2017-08-25T12:53:19Z

src/Renderer/ThreeExtended/PlanarControls.js

+* @param {number} value : the value to be smoothed, between 0 and 1
+* @returns {number}
+*/
+var clamp01 = function clamp01(value) {


You should directly use THREE.Math.clamp(value, 0, 1) instead of adding a one liner function.

peppsac · 2017-08-25T12:55:27Z

src/Renderer/ThreeExtended/PlanarControls.js

+
+// event keycode
+const keys = { CTRL: 17, SPACE: 32, S: 83, T: 84 };
+const mouseButtons = { LEFTCLICK: THREE.MOUSE.LEFT, MIDDLECLICK: THREE.MOUSE.MIDDLE, RIGHTCLICK: THREE.MOUSE.RIGHT };


Minor style comment:

const mouseButtons = { LEFTCLICK: THREE.MOUSE.LEFT, MIDDLECLICK: THREE.MOUSE.MIDDLE, RIGHTCLICK: THREE.MOUSE.RIGHT, };

is prefered.

EmmanuelSchmuck · 2017-08-25T14:44:06Z

Thank you for the review @peppsac , I made the modifications you suggested.

PlanarControls offers ergonomic camera controls for the planar mode : drag, pan, orbit, zoom and animated expression to smoothly move and orient the camera. Modified planar example to use these controls Update contributors Modifications following peppsac review : -all external variables are now within the class, this allows to instanciate more than one controller. -clamp01 function removed, using THREE clamp instead. -various style modifications (separator removed, const array format...)

peppsac

I tested this controls quickly and it seems to be working good.

I left a few comments and then it'll be ready for merging.

peppsac · 2017-08-25T14:59:48Z

src/Renderer/ThreeExtended/PlanarControls.js

+    this.view = view;
+    this.camera = view.camera.camera3D;
+    this.domElement = view.mainLoop.gfxEngine.renderer.domElement;
+    this.position = this.camera.position;


this.position isn't really useful: let's use this.camera.position directly instead.

peppsac · 2017-08-25T15:01:03Z

src/Renderer/ThreeExtended/PlanarControls.js

+    * Updates the view and camera if needed, and handles the animated travel
+    */
+    this.update = function update() {
+        this.deltaTime = this.clock.getElapsedTime() - this.lastElapsedTime;


Why don't you use the dt provided as an argument to your update function (see https://github.com/iTowns/itowns/blob/master/src/Core/MainLoop.js#L63)?

peppsac · 2017-08-25T15:01:47Z

src/Renderer/ThreeExtended/PlanarControls.js

+            this.handleTravel(this.deltaTime);
+        }
+        if (this.state !== STATE.NONE) {
+            this.view.camera.update(window.innerWidth, window.innerHeight);


You don't need to call this: it's done for you in view's update process (https://github.com/iTowns/itowns/blob/master/src/Core/MainLoop.js#L96)

update() uses mainloop's deltatime, no longer call camera.update() this.camera.position replaces this.position

EmmanuelSchmuck · 2017-08-25T17:35:56Z

@peppsac : modified again following your suggestions. Also, my inputs no longer call update() but notifyChange() instead, much cleaner.

…d of S (used for picking)

autra

Taking over this, as @peppsac is off. Sorry for the inconvenience to have another reviewer with different expectations stepping in :-/

On overall I really like it! The ergonomy (and the code) is good :-) I made a lot of comments, but most are minor.

autra · 2017-08-29T08:16:39Z

src/Renderer/ThreeExtended/PlanarControls.js

+    // time management
+    this.deltaTime = 0;
+    this.lastElapsedTime = 0;
+    this.clock = new THREE.Clock();


this.clock is not used.

Actually, all three "time management" vars are not used, if I'm not mistaken.

autra · 2017-08-29T08:21:08Z

src/Renderer/ThreeExtended/PlanarControls.js

+    this._handlerOnMouseDown = onMouseDown.bind(this);
+    this._handlerOnMouseUp = onMouseUp.bind(this);
+    this._handlerOnMouseMove = onMouseMove.bind(this);
+    this._handlerOnMouseWheel = onMouseWheel.bind(this);


As everything is defined in the constructor, you don't really need to attach these handlers on this. They can be local variables. That's better because they are truly isolated then (and being able to really isolate things is a very good reason to do this way, and not to use prototype or ES6 classes :-) )

This is also true for every local state you don't want to expose. I'm thinking about:

this.view

this.camera

all travelFoo vars

all drag vars

this.mousePosition, this.lastMousePosition and this.deltaMousePosition

isCtrlDown

I'm pretty sure these should be hidden.

In contrast, I believe keeping all the option properties visible is perfect :-)

For this.state, I have no strong opinion about it.

autra · 2017-08-29T08:22:40Z

src/Renderer/ThreeExtended/PlanarControls.js

+    * Updates the view and camera if needed, and handles the animated travel
+    * @param {number} dt : deltatime given by mainLoop
+    * @param {boolean} updateLoopRestarted : given by mainLoop
+    */


Not sure we want to document this. It's just the implementation of a FrameRequester. It's not meant to be called by the outside world. Better not have it in documentation.

autra · 2017-08-29T08:50:34Z

src/Renderer/ThreeExtended/PlanarControls.js

+            dt = 16;
+        }
+        if (this.state === STATE.TRAVEL) {
+            this.handleTravel(dt / 1000);


What about reasoning in ms everywhere?

autra · 2017-08-29T09:47:37Z

src/Renderer/ThreeExtended/PlanarControls.js

+* @returns {number}
+*/
+var smooth = function smooth(value) {
+    // p between 1.0 and 1.5


autra · 2017-08-29T12:45:11Z

src/Renderer/ThreeExtended/PlanarControls.js

+        // setup the end rotation :
+
+        // case where targetOrientation is a quaternion
+        if (typeof targetOrientation.w !== 'undefined') {


Then the test should be if (targetOrientation instanceof THREE.Quaternion)?

(it would also work if targetOrientation is undefined this way)

autra · 2017-08-29T12:47:54Z

src/Renderer/ThreeExtended/PlanarControls.js

+        // update cursor
+        this.updateMouseCursorType();
+
+        this.travelUseRotation = !(targetOrientation === 'none');


I would test targetOrientation instanceOf THREE.Quaternion || targetOrientation instanceOf THREE.Vector3, it would also cover undefined, null, whatever string and invalid objects the caller gave you. Here the caller needs to remember to pass 'none' to explicitly disable rotation.

Not using isVector3 because this would throw an error if undefined or null. (I don't know why Vector3 implementors felt the need to have this boolean, it's useless imho).

(I assume you don't want a rotation to the default value of travelEndRot if targetOrientation is invalid)

autra · 2017-08-29T12:53:10Z

src/Renderer/ThreeExtended/PlanarControls.js

+        if (this.instantTravel) {
+            this.travelDuration = 0;
+        }
+


nits: remove empty line (a reader could miss the else if below)

autra · 2017-08-29T12:54:18Z

src/Renderer/ThreeExtended/PlanarControls.js

+        // case where travelTime is set to 'auto' : travelDuration will be a value between autoTravelTimeMin and autoTravelTimeMax
+        // depending on travel distance and travel angular difference
+        else if (travelTime === 'auto') {
+            // a value between 0 and 1 according to the travel distance. Adjusted by autoTravelTimeDist parameter


then autoTravelTimeDist is a factor, not a real distance? Maybe we could rename it then.

autra · 2017-08-29T13:00:26Z

src/Renderer/ThreeExtended/PlanarControls.js

+        const screenCenter = new THREE.Vector2(0.5 * window.innerWidth, 0.5 * window.innerHeight);
+
+        topViewPos.copy(this.getWorldPointAtScreenXY(screenCenter));
+        topViewPos.z += Math.min(this.maxAltitude, this.camera.position.distanceTo(topViewPos));


Can you explain this line to me? Why isn't it always maxAltitude?

autra

Nearly there :-)

autra · 2017-08-30T07:54:23Z

src/Renderer/ThreeExtended/PlanarControls.js

-        return pointUnderCursor;
-    };
+        let dir = new THREE.Vector3();
+        let pointUnderCursor = new THREE.Vector3();


It's not a big deal but, dir does not need to be declared here. You can declare it when you first use it. Anyway, dir and vector will store the same pointer, because unproject, sub, normalize and multiplyScalar all modify the vector3 inplace and returns it (see for instance how sub is implemented).

On the other hand, pointUnderCursor must not be declared here, otherwise we would always return the same pointer, which is dangerous. We need to create a new instance each time, which is done by the clone anyway. So in short, just

return this.camera.clone().add(dir.multiplyScalar(distance));

instead of line 538-540 is fine (that being said, you can keep pointUnderCursor if you find it clearer, just declare it with const on line 538).

And it's totally my fault, because I was the one to ask you about pointUnderCursor. Sorry :-/

autra · 2017-08-30T07:55:45Z

src/Renderer/ThreeExtended/PlanarControls.js

        // the returned value
        const pointUnderCursor = new THREE.Vector3();
+        return (posXY) => {
+            // check if there is valid geometry under cursor
+            if (typeof this.view.getPickingPositionFromDepth(posXY) !== 'undefined') {


Sorry I missed that in my previous review: let's put this.view.getPickingPositionFromDepth(posXY) in a variable to avoid calling it 2 times.

autra · 2017-08-30T08:20:27Z

src/Renderer/ThreeExtended/PlanarControls.js

@@ -212,10 +223,10 @@ function PlanarControls(view, options = {}) {
        const panSpeed = THREE.Math.lerp(this.minPanSpeed, this.maxPanSpeed, distToGround);

        // lateral movement (local x axis)
-        this.camera.position.copy(this.camera.localToWorld(new THREE.Vector3(panSpeed * -1 * this.deltaMousePosition.x, 0, 0)));
+        this.camera.position.copy(this.camera.localToWorld(new THREE.Vector3(panSpeed * -1 * deltaMousePosition.x, 0, 0)));


this.handlePanMovement = (() => { const tmp = new THREE.Vector3(); return () => { // .... tmp.set(panSpeed * -1 * deltaMousePosition.x, 0, 0); this.camera.position.copy(this.camera.localToWorld(tmp)); // ... } })();

should prevent one instanciation.

autra · 2017-08-30T08:26:19Z

src/Renderer/ThreeExtended/PlanarControls.js

+            const offset = this.camera.position.clone().sub(centerPoint);
+
+            quat.setFromUnitVectors(this.camera.up, new THREE.Vector3(0, 0, 1));
+            quatInverse = quat.clone().inverse();


what about quat.setFromUnitVectors(this.camera.up, new THREE.Vector3(0, 0, 1)).inverse() and using quat instead of quatInverse below? It saves a clone().

autra · 2017-08-30T08:26:26Z

src/Renderer/ThreeExtended/PlanarControls.js

-                offset.applyQuaternion(rotationZQuaternion);
+    this.handleRotation = (() => {
+        const quat = new THREE.Quaternion();
+        let quatInverse = new THREE.Quaternion();


Please "cache" here:

new THREE.Vector3(0, 0, 1) on line 268

rotationXQuaternion and vector on line 279-280

rotationZQuaternion on line 290

autra · 2017-08-30T08:42:58Z

src/Renderer/ThreeExtended/PlanarControls.js

+            else {
+                pointUnderCursor.copy(this.getWorldPointFromMathPlaneAtScreenXY(posXY, this.groundLevel));
+            }
+            return pointUnderCursor;


same thing as above. Sorry, my fault :-/

autra · 2017-08-30T08:57:54Z

src/Renderer/ThreeExtended/PlanarControls.js

+    };
+
+    PlanarControls.prototype = Object.create(THREE.EventDispatcher.prototype);
+    PlanarControls.prototype.constructor = PlanarControls;


You're not using dispatchEvent, so you don't need the 2 lines above.

autra · 2017-08-30T10:04:11Z

Yay thanks \o/

autra · 2017-08-30T10:11:09Z

Nice first contrib btw !

With the miniGlobe, getZoom used to always return 0. Feature : added new PlanarControls (iTowns#454) * Feature : added new PlanarControls PlanarControls offers ergonomic camera controls for the planar mode : drag, pan, orbit, zoom and animated expression to smoothly move and orient the camera. Modified planar example to use these controls Update contributors Feature : added new PlanarControls PlanarControls offers ergonomic camera controls for the planar mode : drag, pan, orbit, zoom and animated expression to smoothly move and orient the camera. Modified planar example to use these controls Update contributors Modifications following peppsac review : -all external variables are now within the class, this allows to instanciate more than one controller. -clamp01 function removed, using THREE clamp instead. -various style modifications (separator removed, const array format...) planarcontrols modifications following peppsac review : update() uses mainloop's deltatime, no longer call camera.update() this.camera.position replaces this.position removed options.startposition and startlook, start view with Y instead of S (used for picking) many modifications following review by Autra, see PR

EmmanuelSchmuck · 2017-08-30T11:39:47Z

Thank you :)

NikoSaul requested review from Jeremy-Gaillard, peppsac and NikoSaul August 25, 2017 12:46

peppsac reviewed Aug 25, 2017

View reviewed changes

EmmanuelSchmuck force-pushed the planar_control branch 2 times, most recently from bff620c to 80f4a40 Compare August 25, 2017 14:41

EmmanuelSchmuck force-pushed the planar_control branch from 80f4a40 to 91eafa1 Compare August 25, 2017 14:46

peppsac reviewed Aug 25, 2017

View reviewed changes

planarcontrols modifications following peppsac review :

1a61694

update() uses mainloop's deltatime, no longer call camera.update() this.camera.position replaces this.position

removed options.startposition and startlook, start view with Y instea…

d7ae5da

…d of S (used for picking)

autra suggested changes Aug 29, 2017

View reviewed changes

many modifications following review by Autra, see PR

851c223

EmmanuelSchmuck force-pushed the planar_control branch from 7edea28 to 851c223 Compare August 29, 2017 17:20

autra suggested changes Aug 30, 2017

View reviewed changes

additional changes following @autra second review (opti, cleanup)

957e365

autra approved these changes Aug 30, 2017

View reviewed changes

autra merged commit 92f404d into iTowns:master Aug 30, 2017

Feature : added new PlanarControls #454

Feature : added new PlanarControls #454

Conversation

EmmanuelSchmuck commented Aug 25, 2017

peppsac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EmmanuelSchmuck commented Aug 25, 2017

peppsac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EmmanuelSchmuck commented Aug 25, 2017

autra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

autra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

autra commented Aug 30, 2017

autra commented Aug 30, 2017

EmmanuelSchmuck commented Aug 30, 2017