Implement a way to reset the modelview matrix in Tilt

RESOLVED FIXED in Firefox 12

Status

defect
RESOLVED FIXED
8 years ago
Last year

People

(Reporter: vporof, Assigned: vporof)

Tracking

({dev-doc-complete})

unspecified
Firefox 12
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tilt][fixed-in-fx-team])

Attachments

(1 attachment, 2 obsolete attachments)

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.
Whiteboard: [tilt]
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Posted patch v1 (obsolete) — Splinter Review
Attachment #588507 - Flags: review?(rcampbell)
Comment on attachment 588507 [details] [diff] [review]
v1

Was rebased.
Attachment #588507 - Flags: review?(rcampbell)
Posted patch v2 (obsolete) — Splinter Review
Attachment #588507 - Attachment is obsolete: true
Attachment #588725 - Flags: review?(rcampbell)
Posted patch v3Splinter Review
Cleaned up, fixed some oranges in try.
https://tbpl.mozilla.org/?tree=Try&rev=99f836e9fef1
Attachment #588725 - Attachment is obsolete: true
Attachment #588725 - Flags: review?(rcampbell)
Attachment #588813 - Flags: review?(rcampbell)
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 on attachment 588813 [details] [diff] [review]
v3

oh yeah, the R+ would help.
Attachment #588813 - Flags: review?(rcampbell) → review+
(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!
Filed bug 719731 and bug 719732.
great. Ready to land...
Whiteboard: [tilt] → [tilt][land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/24cdcf5654fc
Whiteboard: [tilt][land-in-fx-team] → [tilt][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/24cdcf5654fc
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
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.
Attachment #588813 - Flags: approval-mozilla-aurora?
Attachment #588813 - Flags: approval-mozilla-aurora?
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
Keywords: dev-doc-needed
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.