Implement a way to reset the modelview matrix in Tilt

RESOLVED FIXED in Firefox 12

Status

()

Firefox
Developer Tools: Inspector
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: vporof, Assigned: vporof)

Tracking

({dev-doc-complete})

unspecified
Firefox 12
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
Whiteboard: [tilt]
(Assignee)

Updated

6 years ago
Assignee: nobody → vporof
Status: NEW → ASSIGNED
(Assignee)

Comment 1

6 years ago
Created attachment 588507 [details] [diff] [review]
v1
Attachment #588507 - Flags: review?(rcampbell)
(Assignee)

Comment 2

6 years ago
Comment on attachment 588507 [details] [diff] [review]
v1

Was rebased.
Attachment #588507 - Flags: review?(rcampbell)
(Assignee)

Comment 3

6 years ago
Created attachment 588725 [details] [diff] [review]
v2
Attachment #588507 - Attachment is obsolete: true
Attachment #588725 - Flags: review?(rcampbell)
(Assignee)

Comment 4

6 years ago
Created attachment 588813 [details] [diff] [review]
v3

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+
(Assignee)

Comment 7

6 years ago
(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!
Blocks: 712096
(Assignee)

Comment 8

6 years ago
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
Last Resolved: 6 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?
tracking-firefox11: --- → ?
Attachment #588813 - Flags: approval-mozilla-aurora?
tracking-firefox11: ? → ---
(Assignee)

Comment 13

6 years ago
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
Documentation updated:

https://developer.mozilla.org/en/Tools/Page_Inspector/3D_view#Controlling_the_3D_view
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.