712029 - Implement a way to reset the modelview matrix in Tilt

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Victor Porof [:vporof][:vp]]

It would be nice to be able to reset the translations/rotations applied to the visualization mesh via the arcball. Currently, only the zoom can be reset to 0.

Comment 1

[Victor Porof [:vporof][:vp]]

Attachment: v1

Comment 2

[Victor Porof [:vporof][:vp]]

Comment on attachment 588507 [details] [diff] [review]
v1

Was rebased.

Comment 3

[Victor Porof [:vporof][:vp]]

Attachment: v2

Comment 4

[Victor Porof [:vporof][:vp]]

Attachment: v3

Cleaned up, fixed some oranges in try.
https://tbpl.mozilla.org/?tree=Try&rev=99f836e9fef1

Comment 5

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 588813 [details] [diff] [review]
v3

in TiltUtils.jsm:

 /**
+ * Gets the most recent browser window.
+ *
+ * @return {Window} the window
+ */
+TiltUtils.getBrowserWindow = function TU_getBrowserWindow()
+{
+  return Cc["@mozilla.org/appshell/window-mediator;1"]
+    .getService(Ci.nsIWindowMediator)
+    .getMostRecentWindow("navigator:browser");
+};
+

why is this needed? We have a reference to the top-level window in Tilt.jsm.

Might make sense to pass in the chromeWindow when creating the TiltVisualizer and let that pick out all the various pieces it needs to refer to. contentWindow, InspectorUI, etc. Can do that in a follow-up if you prefer. 

We're trying to get away from using windowMediator's getMostRecentBrowserWindow function for targeting.

TiltVisualizer.jsm

+/*global TiltGL, TiltMath, EPSILON, vec3, mat4, quat4, TiltUtils */

EPSILON!

-const ARCBALL_SENSITIVITY = 0.3;
+const ARCBALL_SENSITIVITY = 0.5;

sensitivity boost?

    */
-  setupMesh: function TVP_setupMesh(aData)
+  setupMesh: function TVP_setupMesh(aData) /*global TiltVisualizerStyle */

why the comment? Are you referring to a TiltVisualizerStyle that is a global? Importing one?

I find these comments add more confusion than help, tbh.

rots and translations bits look good to me.

     // create an additional rotation based on the key events
-    quat4.fromEuler(deltaKeyRot[0], deltaKeyRot[1], 0, deltaRot);
+    quat4.fromEuler(deltaAdditionalRot[0], deltaAdditionalRot[1], 0, deltaRot);

Euler!

Comment 6

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 588813 [details] [diff] [review]
v3

oh yeah, the R+ would help.

Comment 7

[Victor Porof [:vporof][:vp]]

(In reply to Rob Campbell [:rc] (robcee) from comment #5)
> Comment on attachment 588813 [details] [diff] [review]
> v3
> 
> in TiltUtils.jsm:
> 
>  /**
> + * Gets the most recent browser window.
> + *
> + * @return {Window} the window
> + */
> +TiltUtils.getBrowserWindow = function TU_getBrowserWindow()
> +{
> +  return Cc["@mozilla.org/appshell/window-mediator;1"]
> +    .getService(Ci.nsIWindowMediator)
> +    .getMostRecentWindow("navigator:browser");
> +};
> +
> 
> why is this needed? We have a reference to the top-level window in Tilt.jsm.
> 
> Might make sense to pass in the chromeWindow when creating the
> TiltVisualizer and let that pick out all the various pieces it needs to
> refer to. contentWindow, InspectorUI, etc. Can do that in a follow-up if you
> prefer. 
> 
> We're trying to get away from using windowMediator's
> getMostRecentBrowserWindow function for targeting.
> 

Followup makes absolute sense. I would also prefer to get rid of this, but it sort of sticked..

> TiltVisualizer.jsm
> 
> +/*global TiltGL, TiltMath, EPSILON, vec3, mat4, quat4, TiltUtils */
> 
> EPSILON!
> 
> -const ARCBALL_SENSITIVITY = 0.3;
> +const ARCBALL_SENSITIVITY = 0.5;
> 
> sensitivity boost?
> 

Make dragging a bit more responsive.

>     */
> -  setupMesh: function TVP_setupMesh(aData)
> +  setupMesh: function TVP_setupMesh(aData) /*global TiltVisualizerStyle */
> 
> why the comment? Are you referring to a TiltVisualizerStyle that is a
> global? Importing one?
> 
> I find these comments add more confusion than help, tbh.
> 

One of my old annoying habits (js + globals + crockford). Followup bug to burn away all of these from tilt's codebase.

> rots and translations bits look good to me.
> 
>      // create an additional rotation based on the key events
> -    quat4.fromEuler(deltaKeyRot[0], deltaKeyRot[1], 0, deltaRot);
> +    quat4.fromEuler(deltaAdditionalRot[0], deltaAdditionalRot[1], 0,
> deltaRot);
> 
> Euler!

Birkhauser!

Comment 8

[Victor Porof [:vporof][:vp]]

Filed bug 719731 and bug 719732.

Comment 9

[Rob Campbell [:rc] (:robcee)]

great. Ready to land...

Comment 10

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/integration/fx-team/rev/24cdcf5654fc

Comment 11

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/24cdcf5654fc

Comment 12

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 588813 [details] [diff] [review]
v3

[Approval Request Comment]
Regression caused by (bug #): New feature
User impact if declined: User will not be able to reset their viewpoint in the 3d visualization.
Testing completed (on m-c, etc.): on m-c.
Risk to taking this patch (and alternatives if risky): Fairly low-risk. Possibly moderate. This patch has been quite well-tested on trunk and blocks a whole bunch of downstream patches. Despite the risk, it is a usability win.

Comment 13

[Victor Porof [:vporof][:vp]]

I think we'll need to add two more keys to the Tilt doc on MDN:
* R -> resets the translation, rotation and zoom
* 0 -> resets just the zoom

Comment 14

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/Tools/Page_Inspector/3D_view#Controlling_the_3D_view