Closed Bug 831781 Opened 11 years ago Closed 11 years ago

Allow panning using gamepad d-pad/joystick

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 22

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(4 files, 5 obsolete files)

Currently it is hard/impossible to scroll down pages in Fennec on the ouya because we don't do anything with the d-pad/joystick input events. We should support those to scroll around the page.
This is just cleanup/generification; no functional changes. I should have realized before that mouse wheel code doesn't belong in TouchEventHandler since they're not touch events.
This allows scrolling around the page smoothly with the joystick. The InputDevice.MotionRange objects for the Ouya gamepad seems to be returning improper data so I hard-coded that bit for now. Also this doesn't do any sort of axis locking which we might want to do.
Attachment #703620 - Flags: review?(chrislord.net)
Comment on attachment 703621 [details] [diff] [review]
Part 2 - Support joystick scrolling

Btw, I believe the "else" clause of this hunk:

             mDisplacement += (mLastTouchPos - mTouchPos) * getEdgeResistance(false);
         else
-            mDisplacement += mVelocity;
+            mDisplacement += mVelocity * getEdgeResistance(false);
 
was previously unused, so this change shouldn't affect any previous behaviour. It is used in the autoscroll codepath though. Please let me know if you think it was used in some codepath previously because then this change might regress something.
Attachment #703621 - Flags: review?(chrislord.net)
Attachment #704031 - Flags: review?(chrislord.net)
Attached patch Part 4 - Use L2/R2 for zooming (obsolete) — Splinter Review
We'll probably want to deliver the events to content first and then only handle them if content doesn't use them, similarly to what we're doing in TouchEventHandler. However I think that a lot of this code will change as we merge PZC with B2G's APZC so I figure it's not too much of a big deal right now.
Attachment #704032 - Attachment is obsolete: true
Attachment #704543 - Flags: review?(chrislord.net)
Excuse me for taking a little longer on review than usual, I feel I'd like to try these out before r+'ing and, unfortunately, I don't have a suitable arm objdir atm to do a quick incremental build from.
No problem; I definitely want you to try out the patches and help me tune them if needed. In particular for the zooming one I'm not sure if we should be doing a linear bump in the zoom level (which is what the patch does) or a multiplicative increase/decrease. (e.g. R2 does zoomFactor *= 1.1 instead of zoomFactor += 0.2)
:Cwiiis, do you mind reviewing part 1 here? It's not ouya-specific but it will simplify some of my PZC unification work so I'd like to get that part in at least.
Actually, scratch that last comment. I'll need to rework it somewhat so I might as well rewrite it and incorporate it into the other bug.
Chris, ping. Seeing as we haven't really had much time to clean this up I'd like to get it reviewed and landed so that at least scrolling on the Ouya is possible. We can improve it in future bugs.
Comment on attachment 703620 [details] [diff] [review]
Part 1 - Move mouse wheel code out of TouchEventHandler

Review of attachment 703620 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine.
Attachment #703620 - Flags: review?(chrislord.net) → review+
Comment on attachment 703621 [details] [diff] [review]
Part 2 - Support joystick scrolling

Review of attachment 703621 [details] [diff] [review]:
-----------------------------------------------------------------

This looks ok too. Would be good to see the comment addressed.

::: mobile/android/base/ui/PanZoomController.java
@@ +446,5 @@
>          return false;
>      }
>  
> +    private float normalizeJoystick(float value, InputDevice.MotionRange range) {
> +        if (Math.abs(value) < 1e-2 /* this should really be range.getFlat() + range.getFuzz() */) {

And why isn't it? Maybe expand the comment?
Attachment #703621 - Flags: review?(chrislord.net) → review+
Comment on attachment 704031 [details] [diff] [review]
Part 3 - Extract a helper function

Review of attachment 704031 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.
Attachment #704031 - Flags: review?(chrislord.net) → review+
Comment on attachment 703620 [details] [diff] [review]
Part 1 - Move mouse wheel code out of TouchEventHandler

Err oops, forgot to obsolete this patch. I landed this code as part of bug 777468.
Attachment #703620 - Attachment is obsolete: true
Comment on attachment 704543 [details] [diff] [review]
Part 4 - Use L2/R2 for zooming

Review of attachment 704543 [details] [diff] [review]:
-----------------------------------------------------------------

r- because of the mapping, but I have nothing against the actual code - feel free to commit without hooking it up to those buttons, or hooking it up to keyboard zoom in/out buttons or the mouse-wheel.

::: mobile/android/base/gfx/LayerView.java
@@ +222,1 @@
>          if (mInputConnectionHandler != null)

It might be nice to change this if statement to match the one above, in case someone shuffles these around at some point and breaks it - javac should just optimise it out anyway, right?

::: mobile/android/base/ui/PanZoomController.java
@@ +209,5 @@
> +            switch (event.getKeyCode()) {
> +            case KeyEvent.KEYCODE_BUTTON_R2:
> +                return animatedScale(0.2f);
> +            case KeyEvent.KEYCODE_BUTTON_L2:
> +                return animatedScale(-0.2f);

I don't like this mapping - the Ouya's shoulder buttons are analog triggers, are are most console pad's L2/R2 buttons (certainly PS3, 360 and WiiU 'Pro' pads are) - They should be available on AXIS_LTRIGGER and AXIS_RTRIGGER apparently.
Attachment #704543 - Flags: review?(chrislord.net) → review-
Rebased, updated to address review comment (about getFlat() and getFuzz()). Carrying r+
Attachment #703621 - Attachment is obsolete: true
Attachment #718597 - Flags: review+
Rebased, carrying r+
Attachment #704031 - Attachment is obsolete: true
Attachment #718599 - Flags: review+
Rebased, and mapped zoom in/out to the KeyEvent.KEYCODE_ZOOM_IN and _OUT keys respectively. We never actually get these from the ouya gamepad though.
Attachment #704543 - Attachment is obsolete: true
Attachment #718600 - Flags: review?(chrislord.net)
This addresses your other review comment in comment 16, and also changes the rest of the onKeyXXXX functions to be consistent. Also I found some unnecessary mLayerClient.getPanZoomController() calls left over from previous code changes and lumped that cleanup in here too.
Attachment #718603 - Flags: review?(chrislord.net)
Comment on attachment 718600 [details] [diff] [review]
Part 3 - Hook up zooming

Review of attachment 718600 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the comment addressed.

::: mobile/android/base/gfx/JavaPanZoomController.java
@@ +220,5 @@
> +        if (Build.VERSION.SDK_INT <= 11) {
> +            return false;
> +        }
> +
> +        if ((event.getSource() & InputDevice.SOURCE_GAMEPAD) == InputDevice.SOURCE_GAMEPAD

This isn't quite what I meant, I'm not sure we'd ever get ZOOM_IN/OUT from a gamepad - Can we change this to SOURCE_KEYBOARD, or at least || with SOURCE_KEYBOARD?
Attachment #718600 - Flags: review?(chrislord.net) → review+
Comment on attachment 718603 [details] [diff] [review]
Part 4 - code cleanup

Review of attachment 718603 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.
Attachment #718603 - Flags: review?(chrislord.net) → review+
(In reply to Chris Lord [:cwiiis] from comment #21)
> This isn't quite what I meant, I'm not sure we'd ever get ZOOM_IN/OUT from a
> gamepad - Can we change this to SOURCE_KEYBOARD, or at least || with
> SOURCE_KEYBOARD?

Sure, I can change that. But I don't understand the point here - are there any keyboards that have dedicated "zoom in" and "zoom out" buttons? I don't know of any, and so this is dead code regardless. I guess what I'm wondering is, in an ideal world, what buttons on the Ouya controller would you like to map zooming to?

(In reply to Chris Lord [:cwiiis] from comment #16)
> I don't like this mapping - the Ouya's shoulder buttons are analog triggers,
> are are most console pad's L2/R2 buttons (certainly PS3, 360 and WiiU 'Pro'
> pads are) - They should be available on AXIS_LTRIGGER and AXIS_RTRIGGER
> apparently.

I admit I don't fully understand what you meant here. My interpretation of "shoulder buttons" are L1 and R1, which are not analog. My interpretation of "trigger buttons" are L2 and R2, which are usually analog but on the Ouya seem to not be (or at least I didn't see any analog data coming from them). I didn't find AXIS_LTRIGGER and AXIS_RTRIGGER anywhere, so I don't know what those are referring to.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #23)
> (In reply to Chris Lord [:cwiiis] from comment #21)
> > This isn't quite what I meant, I'm not sure we'd ever get ZOOM_IN/OUT from a
> > gamepad - Can we change this to SOURCE_KEYBOARD, or at least || with
> > SOURCE_KEYBOARD?
> 
> Sure, I can change that. But I don't understand the point here - are there
> any keyboards that have dedicated "zoom in" and "zoom out" buttons? I don't
> know of any, and so this is dead code regardless. I guess what I'm wondering
> is, in an ideal world, what buttons on the Ouya controller would you like to
> map zooming to?

I think my point is that we shouldn't map zoom to digital buttons when we have good analog alternatives (and apart from the WiiMote which has +/- buttons, it goes against convention) - I did like the code to handle this though, so it'd be nice to have it in there. I've had media keyboards with dedicated zoom buttons before, and we could use this code to handle mouse wheels too (which are usually implemented as button presses rather than an analog signal... sigh...)

So in an ideal world for Ouya, zoom would be mapped to the 2nd analog stick or the triggers. I quite like it on the 2nd analog stick, personally (this is what the WiiU does).

> (In reply to Chris Lord [:cwiiis] from comment #16)
> > I don't like this mapping - the Ouya's shoulder buttons are analog triggers,
> > are are most console pad's L2/R2 buttons (certainly PS3, 360 and WiiU 'Pro'
> > pads are) - They should be available on AXIS_LTRIGGER and AXIS_RTRIGGER
> > apparently.
> 
> I admit I don't fully understand what you meant here. My interpretation of
> "shoulder buttons" are L1 and R1, which are not analog. My interpretation of
> "trigger buttons" are L2 and R2, which are usually analog but on the Ouya
> seem to not be (or at least I didn't see any analog data coming from them).
> I didn't find AXIS_LTRIGGER and AXIS_RTRIGGER anywhere, so I don't know what
> those are referring to.

Sorry, I guess we need to establish terminology... I usually think of L1/R1 as hats and L2/R2 as triggers, but I'd call all of them buttons - I think your use of button/trigger is better, let's stick with that.

It's worth having a look at the ouya-controller-testapp sample in the Ouya development kit - it shows how to read all of the inputs on the gamepad. They have their own defines for the buttons, but they all map to standard Android equivalents - So for the shoulder buttons, you can listen to onGenericMotionEvent, and use event.getAxisValue(MotionEvent.AXIS_LTRIGGER), something like that - LTRIGGER is API level 12 though, so I guess we'll have to copy the numbers out...

Can we map zoom to the right stick though, rather than the triggers? It really feels so much better :)
(In reply to Chris Lord [:cwiiis] from comment #24)
> I think my point is that we shouldn't map zoom to digital buttons when we
> have good analog alternatives (and apart from the WiiMote which has +/-
> buttons, it goes against convention) - I did like the code to handle this
> though, so it'd be nice to have it in there. I've had media keyboards with
> dedicated zoom buttons before, and we could use this code to handle mouse
> wheels too (which are usually implemented as button presses rather than an
> analog signal... sigh...)

Agreed, it would be better to use an analog input for this feature rather than digital. I can land the patch as-is and then we can work with ibarlow to figure out the best mapping for everything.

> So in an ideal world for Ouya, zoom would be mapped to the 2nd analog stick
> or the triggers. I quite like it on the 2nd analog stick, personally (this
> is what the WiiU does).

I think the plan for the second analog stick was to make it do spatial navigation (link-to-link jumping). That needs a 4-way directional input, so I guess it could be mapped to the arrow buttons as well, but would feel more natural on the analog stick, I think.

> > (In reply to Chris Lord [:cwiiis] from comment #16)
> Sorry, I guess we need to establish terminology... I usually think of L1/R1
> as hats and L2/R2 as triggers, but I'd call all of them buttons - I think
> your use of button/trigger is better, let's stick with that.
> 
> It's worth having a look at the ouya-controller-testapp sample in the Ouya
> development kit - it shows how to read all of the inputs on the gamepad.

Hmm, I should look at that...

> They have their own defines for the buttons, but they all map to standard
> Android equivalents - So for the shoulder buttons, you can listen to
> onGenericMotionEvent, and use event.getAxisValue(MotionEvent.AXIS_LTRIGGER),
> something like that - LTRIGGER is API level 12 though, so I guess we'll have
> to copy the numbers out...

Ah, ok, that makes sense.

> Can we map zoom to the right stick though, rather than the triggers? It
> really feels so much better :)

I'll leave that decision up to ibarlow. I'm fine with it either way as long as we have something good to map spatial nav to.
Just booted up the lastest nightly.  Not sure if you guys decided to nix the d-pad for pan or not but its not working on my Ouya.
I think we wanted to use the d-pad for link-to-link navigation which landed at some point (bug 698437) but I don't remember the current status of that. It might be preffed off still. Does joystick-based panning work for you?
Joystick does still work yes.  Thank you for letting me know.  link to link is probably the best way to go about it but that doesn't seem to work either I'm sure they know already.
I looked through the code and actually it does seem to be hooked up and enabled. It might just be that the key mappings are wrong for the ouya. Link-to-link navigation works on my android phone which has a hardware keyboard and a d-pad. Probably best to file a new bug for this issue.
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: