Last Comment Bug 712029 - Implement a way to reset the modelview matrix in Tilt
: Implement a way to reset the modelview matrix in Tilt
Status: RESOLVED FIXED
[tilt][fixed-in-fx-team]
: dev-doc-complete
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 12
Assigned To: Victor Porof [:vporof][:vp]
:
Mentors:
Depends on:
Blocks: 712096
  Show dependency treegraph
 
Reported: 2011-12-19 09:20 PST by Victor Porof [:vporof][:vp]
Modified: 2012-04-23 10:33 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (19.49 KB, patch)
2012-01-13 13:18 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Review
v2 (21.18 KB, patch)
2012-01-15 04:10 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Review
v3 (27.28 KB, patch)
2012-01-15 23:10 PST, Victor Porof [:vporof][:vp]
rcampbell: review+
Details | Diff | Review

Description Victor Porof [:vporof][:vp] 2011-12-19 09:20:08 PST
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] 2012-01-13 13:18:27 PST
Created attachment 588507 [details] [diff] [review]
v1
Comment 2 Victor Porof [:vporof][:vp] 2012-01-14 12:38:58 PST
Comment on attachment 588507 [details] [diff] [review]
v1

Was rebased.
Comment 3 Victor Porof [:vporof][:vp] 2012-01-15 04:10:52 PST
Created attachment 588725 [details] [diff] [review]
v2
Comment 4 Victor Porof [:vporof][:vp] 2012-01-15 23:10:04 PST
Created attachment 588813 [details] [diff] [review]
v3

Cleaned up, fixed some oranges in try.
https://tbpl.mozilla.org/?tree=Try&rev=99f836e9fef1
Comment 5 Rob Campbell [:rc] (:robcee) 2012-01-19 12:10:39 PST
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) 2012-01-19 12:11:02 PST
Comment on attachment 588813 [details] [diff] [review]
v3

oh yeah, the R+ would help.
Comment 7 Victor Porof [:vporof][:vp] 2012-01-19 12:44:00 PST
(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] 2012-01-20 00:59:07 PST
Filed bug 719731 and bug 719732.
Comment 9 Rob Campbell [:rc] (:robcee) 2012-01-20 08:58:09 PST
great. Ready to land...
Comment 10 Rob Campbell [:rc] (:robcee) 2012-01-20 10:46:26 PST
https://hg.mozilla.org/integration/fx-team/rev/24cdcf5654fc
Comment 11 Rob Campbell [:rc] (:robcee) 2012-01-21 07:37:16 PST
https://hg.mozilla.org/mozilla-central/rev/24cdcf5654fc
Comment 12 Rob Campbell [:rc] (:robcee) 2012-01-27 07:52:32 PST
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] 2012-02-28 23:47:25 PST
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] 2012-04-23 10:33:04 PDT
Documentation updated:

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

Note You need to log in before you can comment on or make changes to this bug.